Split regular histograms from date histograms. #19551

Currently both aggregations really share the same implementation. This commit
splits the implementations so that regular histograms can support decimal
intervals/offsets and compute correct buckets for negative decimal values.

However the response API is still the same. So for intance both regular
histograms and date histograms will produce an
`org.elasticsearch.search.aggregations.bucket.histogram.Histogram`
aggregation.

The optimization to compute an identifier of the rounded value and the
rounded value itself has been removed since it was only used by regular
histograms, which now do the rounding themselves instead of relying on the
Rounding abstraction.

Closes #8082
Closes #4847

Author: jpountz

core/src/main/java/org/elasticsearch/common/rounding/Rounding.java

@@ -35,24 +35,9 @@ public abstract class Rounding implements Streamable {
     public abstract byte id();
 
     /**
-     * Given a value, compute a key that uniquely identifies the rounded value although it is not necessarily equal to the rounding value itself.
+     * Rounds the given value.
      */
-    public abstract long roundKey(long value);
-
-    /**
-     * Compute the rounded value given the key that identifies it.
-     */
-    public abstract long valueForKey(long key);
-
-    /**
-     * Rounds the given value, equivalent to calling <code>roundValue(roundKey(value))</code>.
-     *
-     * @param value The value to round.
-     * @return      The rounded value.
-     */
-    public final long round(long value) {
-        return valueForKey(roundKey(value));
-    }
+    public abstract long round(long value);
 
     /**
      * Given the rounded value (which was potentially generated by {@link #round(long)}, returns the next rounding value. For example, with
@@ -112,13 +97,8 @@ public static long roundValue(long key, long interval) {
         }
 
         @Override
-        public long roundKey(long value) {
-            return roundKey(value, interval);
-        }
-
-        @Override
-        public long valueForKey(long key) {
-            return key * interval;
+        public long round(long value) {
+            return roundKey(value, interval) * interval;
         }
 
         @Override
@@ -179,13 +159,8 @@ public byte id() {
         }
 
         @Override
-        public long roundKey(long utcMillis) {
-            return rounding.roundKey((long) (factor * utcMillis));
-        }
-
-        @Override
-        public long valueForKey(long key) {
-            return rounding.valueForKey(key);
+        public long round(long utcMillis) {
+            return rounding.round((long) (factor * utcMillis));
         }
 
         @Override
@@ -248,13 +223,8 @@ public byte id() {
         }
 
         @Override
-        public long roundKey(long value) {
-            return rounding.roundKey(value - offset);
-        }
-
-        @Override
-        public long valueForKey(long key) {
-            return offset + rounding.valueForKey(key);
+        public long round(long value) {
+            return rounding.round(value - offset) + offset;
         }
 
         @Override

core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java

@@ -31,6 +31,9 @@
 import java.util.Objects;
 
 /**
+ * A rounding strategy for dates. It is typically used to group together dates
+ * that are part of the same hour/day/month, taking into account time zones and
+ * daylight saving times.
  */
 public abstract class TimeZoneRounding extends Rounding {
     public static final ParseField INTERVAL_FIELD = new ParseField("interval");
@@ -125,7 +128,7 @@ public byte id() {
         }
 
         @Override
-        public long roundKey(long utcMillis) {
+        public long round(long utcMillis) {
             long rounded = field.roundFloor(utcMillis);
             if (timeZone.isFixed() == false && timeZone.getOffset(utcMillis) != timeZone.getOffset(rounded)) {
                 // in this case, we crossed a time zone transition. In some edge cases this will
@@ -138,20 +141,14 @@ public long roundKey(long utcMillis) {
             return rounded;
         }
 
-        @Override
-        public long valueForKey(long time) {
-            assert roundKey(time) == time;
-            return time;
-        }
-
         @Override
         public long nextRoundingValue(long utcMillis) {
-            long floor = roundKey(utcMillis);
+            long floor = round(utcMillis);
             // add one unit and round to get to next rounded value
-            long next = roundKey(field.add(floor, 1));
+            long next = round(field.add(floor, 1));
             if (next == floor) {
                 // in rare case we need to add more than one unit
-                next = roundKey(field.add(floor, 2));
+                next = round(field.add(floor, 2));
             }
             return next;
         }
@@ -216,7 +213,7 @@ public byte id() {
         }
 
         @Override
-        public long roundKey(long utcMillis) {
+        public long round(long utcMillis) {
             long timeLocal = timeZone.convertUTCToLocal(utcMillis);
             long rounded = Rounding.Interval.roundValue(Rounding.Interval.roundKey(timeLocal, interval), interval);
             long roundedUTC;
@@ -225,7 +222,7 @@ public long roundKey(long utcMillis) {
                 // check if we crossed DST transition, in this case we want the last rounded value before the transition
                 long transition = timeZone.previousTransition(utcMillis);
                 if (transition != utcMillis && transition > roundedUTC) {
-                    roundedUTC = roundKey(transition - 1);
+                    roundedUTC = round(transition - 1);
                 }
             } else {
                 /*
@@ -276,12 +273,6 @@ private boolean isInDSTGap(long instantLocal) {
             return false;
         }
 
-        @Override
-        public long valueForKey(long time) {
-            assert roundKey(time) == time;
-            return time;
-        }
-
         @Override
         public long nextRoundingValue(long time) {
             long timeLocal = time;

core/src/main/java/org/elasticsearch/search/SearchModule.java

@@ -116,6 +116,7 @@
 import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramParser;
 import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.histogram.HistogramParser;
+import org.elasticsearch.search.aggregations.bucket.histogram.InternalDateHistogram;
 import org.elasticsearch.search.aggregations.bucket.histogram.InternalHistogram;
 import org.elasticsearch.search.aggregations.bucket.missing.InternalMissing;
 import org.elasticsearch.search.aggregations.bucket.missing.MissingAggregationBuilder;
@@ -546,7 +547,7 @@ private void registerBuiltinAggregations() {
         registerAggregation(new AggregationSpec(HistogramAggregationBuilder::new, new HistogramParser(),
                 HistogramAggregationBuilder.AGGREGATION_NAME_FIELD).addResultReader(InternalHistogram::new));
         registerAggregation(new AggregationSpec(DateHistogramAggregationBuilder::new, new DateHistogramParser(),
-                DateHistogramAggregationBuilder.AGGREGATION_NAME_FIELD));
+                DateHistogramAggregationBuilder.AGGREGATION_NAME_FIELD).addResultReader(InternalDateHistogram::new));
         registerAggregation(new AggregationSpec(GeoDistanceAggregationBuilder::new, new GeoDistanceParser(),
                 GeoDistanceAggregationBuilder.AGGREGATION_NAME_FIELD).addResultReader(InternalGeoDistance::new));
         registerAggregation(new AggregationSpec(GeoGridAggregationBuilder::new, new GeoHashGridParser(),