Refactor extendedBounds to use DoubleBounds (#60556) (#60681)

Refactors extendedBounds to use DoubleBounds instead
of 2 variables.

This is a follow up for #59175

server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AbstractHistogramAggregator.java

@@ -37,6 +37,9 @@ import java.util.Collections;
 import java.util.Map;
 import java.util.function.BiConsumer;
 
+import static org.elasticsearch.search.aggregations.bucket.histogram.DoubleBounds.getEffectiveMax;
+import static org.elasticsearch.search.aggregations.bucket.histogram.DoubleBounds.getEffectiveMin;
+
 /**
  * Base class for functionality shared between aggregators for this
  * {@code histogram} aggregation.
@@ -48,8 +51,7 @@ public abstract class AbstractHistogramAggregator extends BucketsAggregator {
     protected final BucketOrder order;
     protected final boolean keyed;
     protected final long minDocCount;
-    protected final double minBound;
+    protected final DoubleBounds extendedBounds;
-    protected final double maxBound;
     protected final DoubleBounds hardBounds;
     protected final LongKeyedBucketOrds bucketOrds;
 
@@ -61,8 +63,7 @@ public abstract class AbstractHistogramAggregator extends BucketsAggregator {
         BucketOrder order,
         boolean keyed,
         long minDocCount,
-        double minBound,
+        DoubleBounds extendedBounds,
-        double maxBound,
         DoubleBounds hardBounds,
         DocValueFormat formatter,
         SearchContext context,
@@ -80,8 +81,7 @@ public abstract class AbstractHistogramAggregator extends BucketsAggregator {
         order.validate(this);
         this.keyed = keyed;
         this.minDocCount = minDocCount;
-        this.minBound = minBound;
+        this.extendedBounds = extendedBounds;
-        this.maxBound = maxBound;
         this.hardBounds = hardBounds;
         this.formatter = formatter;
         bucketOrds = LongKeyedBucketOrds.build(context.bigArrays(), cardinalityUpperBound);
@@ -100,7 +100,8 @@ public abstract class AbstractHistogramAggregator extends BucketsAggregator {
 
                 EmptyBucketInfo emptyBucketInfo = null;
                 if (minDocCount == 0) {
-                    emptyBucketInfo = new EmptyBucketInfo(interval, offset, minBound, maxBound, buildEmptySubAggregations());
+                    emptyBucketInfo = new EmptyBucketInfo(interval, offset, getEffectiveMin(extendedBounds),
+                        getEffectiveMax(extendedBounds), buildEmptySubAggregations());
                 }
                 return new InternalHistogram(name, buckets, order, minDocCount, emptyBucketInfo, formatter, keyed, metadata());
             });
@@ -110,7 +111,8 @@ public abstract class AbstractHistogramAggregator extends BucketsAggregator {
     public InternalAggregation buildEmptyAggregation() {
         InternalHistogram.EmptyBucketInfo emptyBucketInfo = null;
         if (minDocCount == 0) {
-            emptyBucketInfo = new InternalHistogram.EmptyBucketInfo(interval, offset, minBound, maxBound, buildEmptySubAggregations());
+            emptyBucketInfo = new InternalHistogram.EmptyBucketInfo(interval, offset, getEffectiveMin(extendedBounds),
+                getEffectiveMax(extendedBounds), buildEmptySubAggregations());
         }
         return new InternalHistogram(name, Collections.emptyList(), order, minDocCount, emptyBucketInfo, formatter, keyed, metadata());
     }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DoubleBounds.java

@@ -70,6 +70,15 @@ public class DoubleBounds implements ToXContentFragment, Writeable {
      * Construct with bounds.
      */
     public DoubleBounds(Double min, Double max) {
+        if (min != null && Double.isFinite(min) == false) {
+            throw new IllegalArgumentException("min bound must be finite, got: " + min);
+        }
+        if (max != null && Double.isFinite(max) == false) {
+            throw new IllegalArgumentException("max bound must be finite, got: " + max);
+        }
+        if (max != null && min != null && max < min) {
+            throw new IllegalArgumentException("max bound [" + max + "] must be greater than min bound [" + min + "]");
+        }
         this.min = min;
         this.max = max;
     }
@@ -125,6 +134,20 @@ public class DoubleBounds implements ToXContentFragment, Writeable {
         return max;
     }
 
+    /**
+     * returns bounds min if it is defined or POSITIVE_INFINITY otherwise
+     */
+    public static double getEffectiveMin(DoubleBounds bounds) {
+        return bounds == null || bounds.min == null ? Double.POSITIVE_INFINITY : bounds.min;
+    }
+
+    /**
+     * returns bounds max if it is defined or NEGATIVE_INFINITY otherwise
+     */
+    public static Double getEffectiveMax(DoubleBounds bounds) {
+        return bounds == null || bounds.max == null ? Double.NEGATIVE_INFINITY : bounds.max;
+    }
+
     public boolean contain(double value) {
         if (max != null && value > max) {
             return false;

server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java

@@ -72,9 +72,8 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
 
         PARSER.declareLong(HistogramAggregationBuilder::minDocCount, Histogram.MIN_DOC_COUNT_FIELD);
 
-        PARSER.declareField((histogram, extendedBounds) -> {
+        PARSER.declareField(HistogramAggregationBuilder::extendedBounds, parser -> DoubleBounds.PARSER.apply(parser, null),
-            histogram.extendedBounds(extendedBounds[0], extendedBounds[1]);
+            Histogram.EXTENDED_BOUNDS_FIELD, ObjectParser.ValueType.OBJECT);
-        }, parser -> EXTENDED_BOUNDS_PARSER.apply(parser, null), Histogram.EXTENDED_BOUNDS_FIELD, ObjectParser.ValueType.OBJECT);
 
         PARSER.declareField(HistogramAggregationBuilder::hardBounds, parser -> DoubleBounds.PARSER.apply(parser, null),
             Histogram.HARD_BOUNDS_FIELD, ObjectParser.ValueType.OBJECT);
@@ -89,9 +88,7 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
 
     private double interval;
     private double offset = 0;
-    //TODO: Replace with DoubleBounds
+    private DoubleBounds extendedBounds;
-    private double minBound = Double.POSITIVE_INFINITY;
-    private double maxBound = Double.NEGATIVE_INFINITY;
     private DoubleBounds hardBounds;
     private BucketOrder order = BucketOrder.key(true);
     private boolean keyed = false;
@@ -113,8 +110,7 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
         super(clone, factoriesBuilder, metadata);
         this.interval = clone.interval;
         this.offset = clone.offset;
-        this.minBound = clone.minBound;
+        this.extendedBounds = clone.extendedBounds;
-        this.maxBound = clone.maxBound;
         this.hardBounds = clone.hardBounds;
         this.order = clone.order;
         this.keyed = clone.keyed;
@@ -134,10 +130,17 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
         minDocCount = in.readVLong();
         interval = in.readDouble();
         offset = in.readDouble();
-        minBound = in.readDouble();
-        maxBound = in.readDouble();
         if (in.getVersion().onOrAfter(Version.V_7_10_0)) {
+            extendedBounds = in.readOptionalWriteable(DoubleBounds::new);
             hardBounds = in.readOptionalWriteable(DoubleBounds::new);
+        } else {
+            double minBound = in.readDouble();
+            double maxBound = in.readDouble();
+            if (minBound ==  Double.POSITIVE_INFINITY && maxBound == Double.NEGATIVE_INFINITY) {
+                extendedBounds = null;
+            } else {
+                extendedBounds = new DoubleBounds(minBound, maxBound);
+            }
         }
     }
 
@@ -148,10 +151,17 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
         out.writeVLong(minDocCount);
         out.writeDouble(interval);
         out.writeDouble(offset);
-        out.writeDouble(minBound);
-        out.writeDouble(maxBound);
         if (out.getVersion().onOrAfter(Version.V_7_10_0)) {
+            out.writeOptionalWriteable(extendedBounds);
             out.writeOptionalWriteable(hardBounds);
+        } else {
+            if (extendedBounds != null) {
+                out.writeDouble(extendedBounds.getMin());
+                out.writeDouble(extendedBounds.getMax());
+            } else {
+                out.writeDouble(Double.POSITIVE_INFINITY);
+                out.writeDouble(Double.NEGATIVE_INFINITY);
+            }
         }
     }
 
@@ -182,12 +192,16 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
 
     /** Get the current minimum bound that is set on this builder. */
     public double minBound() {
-        return minBound;
+        return extendedBounds.getMin();
     }
 
     /** Get the current maximum bound that is set on this builder. */
     public double maxBound() {
-        return maxBound;
+        return extendedBounds.getMax();
+    }
+
+    protected DoubleBounds extendedBounds() {
+        return extendedBounds;
     }
 
     /**
@@ -200,17 +214,23 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
      *             are not finite.
      */
     public HistogramAggregationBuilder extendedBounds(double minBound, double maxBound) {
-        if (Double.isFinite(minBound) == false) {
+        return extendedBounds(new DoubleBounds(minBound, maxBound));
-            throw new IllegalArgumentException("minBound must be finite, got: " + minBound);
     }
-        if (Double.isFinite(maxBound) == false) {
+
-            throw new IllegalArgumentException("maxBound must be finite, got: " + maxBound);
+    /**
+     * Set extended bounds on this builder: buckets between {@code minBound} and
+     * {@code maxBound} will be created even if no documents fell into these
+     * buckets.
+     *
+     * @throws IllegalArgumentException
+     *             if maxBound is less that minBound, or if either of the bounds
+     *             are not finite.
+     */
+    public HistogramAggregationBuilder extendedBounds(DoubleBounds extendedBounds) {
+        if (extendedBounds == null) {
+            throw new IllegalArgumentException("[extended_bounds] must not be null: [" + name + "]");
         }
-        if (maxBound < minBound) {
+        this.extendedBounds = extendedBounds;
-            throw new IllegalArgumentException("maxBound [" + maxBound + "] must be greater than minBound [" + minBound + "]");
-        }
-        this.minBound = minBound;
-        this.maxBound = maxBound;
         return this;
     }
 
@@ -307,14 +327,15 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
 
         builder.field(Histogram.MIN_DOC_COUNT_FIELD.getPreferredName(), minDocCount);
 
-        if (Double.isFinite(minBound) || Double.isFinite(maxBound)) {
+        if (extendedBounds != null) {
             builder.startObject(Histogram.EXTENDED_BOUNDS_FIELD.getPreferredName());
-            if (Double.isFinite(minBound)) {
+            extendedBounds.toXContent(builder, params);
-                builder.field("min", minBound);
+            builder.endObject();
-            }
-            if (Double.isFinite(maxBound)) {
-                builder.field("max", maxBound);
         }
+
+        if (hardBounds != null) {
+            builder.startObject(Histogram.HARD_BOUNDS_FIELD.getPreferredName());
+            hardBounds.toXContent(builder, params);
             builder.endObject();
         }
 
@@ -332,23 +353,23 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
                                                        AggregatorFactory parent,
                                                        AggregatorFactories.Builder subFactoriesBuilder) throws IOException {
 
-        if (hardBounds != null) {
+        if (hardBounds != null && extendedBounds != null) {
-            if (hardBounds.getMax() != null && hardBounds.getMax() < maxBound) {
+            if (hardBounds.getMax() != null && extendedBounds.getMax() != null && hardBounds.getMax() < extendedBounds.getMax()) {
                 throw new IllegalArgumentException("Extended bounds have to be inside hard bounds, hard bounds: [" +
-                    hardBounds + "], extended bounds: [" + minBound + "--" + maxBound + "]");
+                    hardBounds + "], extended bounds: [" + extendedBounds.getMin() + "--" + extendedBounds.getMax() + "]");
             }
-            if (hardBounds.getMin() != null && hardBounds.getMin() > minBound) {
+            if (hardBounds.getMin() != null && extendedBounds.getMin() != null && hardBounds.getMin() > extendedBounds.getMin()) {
                 throw new IllegalArgumentException("Extended bounds have to be inside hard bounds, hard bounds: [" +
-                    hardBounds + "], extended bounds: [" + minBound + "--" + maxBound + "]");
+                    hardBounds + "], extended bounds: [" + extendedBounds.getMin() + "--" + extendedBounds.getMax() + "]");
             }
         }
-        return new HistogramAggregatorFactory(name, config, interval, offset, order, keyed, minDocCount, minBound, maxBound,
+        return new HistogramAggregatorFactory(name, config, interval, offset, order, keyed, minDocCount, extendedBounds,
             hardBounds, queryShardContext, parent, subFactoriesBuilder, metadata);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(super.hashCode(), order, keyed, minDocCount, interval, offset, minBound, maxBound, hardBounds);
+        return Objects.hash(super.hashCode(), order, keyed, minDocCount, interval, offset, extendedBounds, hardBounds);
     }
 
     @Override
@@ -362,8 +383,7 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
             && Objects.equals(minDocCount, other.minDocCount)
             && Objects.equals(interval, other.interval)
             && Objects.equals(offset, other.offset)
-            && Objects.equals(minBound, other.minBound)
+            && Objects.equals(extendedBounds, other.extendedBounds)
-            && Objects.equals(maxBound, other.maxBound)
             && Objects.equals(hardBounds, other.hardBounds);
     }
 }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java

@@ -47,7 +47,7 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact
     private final BucketOrder order;
     private final boolean keyed;
     private final long minDocCount;
-    private final double minBound, maxBound;
+    private final DoubleBounds extendedBounds;
     private final DoubleBounds hardBounds;
 
     static void registerAggregators(ValuesSourceRegistry.Builder builder) {
@@ -66,8 +66,7 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact
                                         BucketOrder order,
                                         boolean keyed,
                                         long minDocCount,
-                                        double minBound,
+                                        DoubleBounds extendedBounds,
-                                        double maxBound,
                                         DoubleBounds hardBounds,
                                         QueryShardContext queryShardContext,
                                         AggregatorFactory parent,
@@ -79,8 +78,7 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact
         this.order = order;
         this.keyed = keyed;
         this.minDocCount = minDocCount;
-        this.minBound = minBound;
+        this.extendedBounds = extendedBounds;
-        this.maxBound = maxBound;
         this.hardBounds = hardBounds;
     }
 
@@ -100,7 +98,7 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact
                 aggregatorSupplier.getClass().toString() + "]");
         }
         HistogramAggregatorSupplier histogramAggregatorSupplier = (HistogramAggregatorSupplier) aggregatorSupplier;
-        return histogramAggregatorSupplier.build(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound,
+        return histogramAggregatorSupplier.build(name, factories, interval, offset, order, keyed, minDocCount, extendedBounds,
             hardBounds, config, searchContext, parent, cardinality, metadata);
     }
 
@@ -108,7 +106,7 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact
     protected Aggregator createUnmapped(SearchContext searchContext,
                                             Aggregator parent,
                                             Map<String, Object> metadata) throws IOException {
-        return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound,
+        return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, extendedBounds,
             hardBounds, config, searchContext, parent, CardinalityUpperBound.NONE, metadata);
     }
 }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorSupplier.java

@@ -38,8 +38,7 @@ public interface HistogramAggregatorSupplier extends AggregatorSupplier {
         BucketOrder order,
         boolean keyed,
         long minDocCount,
-        double minBound,
+        DoubleBounds extendedBounds,
-        double maxBound,
         DoubleBounds hardBounds,
         ValuesSourceConfig valuesSourceConfig,
         SearchContext context,

server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java

@@ -52,8 +52,7 @@ public class NumericHistogramAggregator extends AbstractHistogramAggregator {
         BucketOrder order,
         boolean keyed,
         long minDocCount,
-        double minBound,
+        DoubleBounds extendedBounds,
-        double maxBound,
         DoubleBounds hardBounds,
         ValuesSourceConfig valuesSourceConfig,
         SearchContext context,
@@ -69,8 +68,7 @@ public class NumericHistogramAggregator extends AbstractHistogramAggregator {
             order,
             keyed,
             minDocCount,
-            minBound,
+            extendedBounds,
-            maxBound,
             hardBounds,
             valuesSourceConfig.format(),
             context,

server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java

@@ -49,8 +49,7 @@ public class RangeHistogramAggregator extends AbstractHistogramAggregator {
         BucketOrder order,
         boolean keyed,
         long minDocCount,
-        double minBound,
+        DoubleBounds extendedBounds,
-        double maxBound,
         DoubleBounds hardBounds,
         ValuesSourceConfig valuesSourceConfig,
         SearchContext context,
@@ -66,8 +65,7 @@ public class RangeHistogramAggregator extends AbstractHistogramAggregator {
             order,
             keyed,
             minDocCount,
-            minBound,
+            extendedBounds,
-            maxBound,
             hardBounds,
             valuesSourceConfig.format(),
             context,

server/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java

@@ -73,21 +73,21 @@ public class HistogramTests extends BaseAggregationTestCase<HistogramAggregation
         factory.interval(randomDouble() * 1000);
 
         IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(Double.NaN, 1.0); });
-        assertThat(ex.getMessage(), startsWith("minBound must be finite, got: "));
+        assertThat(ex.getMessage(), startsWith("min bound must be finite, got: "));
         ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(Double.POSITIVE_INFINITY, 1.0); });
-        assertThat(ex.getMessage(), startsWith("minBound must be finite, got: "));
+        assertThat(ex.getMessage(), startsWith("min bound must be finite, got: "));
         ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(Double.NEGATIVE_INFINITY, 1.0); });
-        assertThat(ex.getMessage(), startsWith("minBound must be finite, got: "));
+        assertThat(ex.getMessage(), startsWith("min bound must be finite, got: "));
 
         ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(0.0, Double.NaN); });
-        assertThat(ex.getMessage(), startsWith("maxBound must be finite, got: "));
+        assertThat(ex.getMessage(), startsWith("max bound must be finite, got: "));
         ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(0.0, Double.POSITIVE_INFINITY); });
-        assertThat(ex.getMessage(), startsWith("maxBound must be finite, got: "));
+        assertThat(ex.getMessage(), startsWith("max bound must be finite, got: "));
         ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(0.0, Double.NEGATIVE_INFINITY); });
-        assertThat(ex.getMessage(), startsWith("maxBound must be finite, got: "));
+        assertThat(ex.getMessage(), startsWith("max bound must be finite, got: "));
 
         ex = expectThrows(IllegalArgumentException.class, () -> { factory.extendedBounds(0.5, 0.4); });
-        assertThat(ex.getMessage(), equalTo("maxBound [0.4] must be greater than minBound [0.5]"));
+        assertThat(ex.getMessage(), equalTo("max bound [0.4] must be greater than min bound [0.5]"));
     }
 
     private List<BucketOrder> randomOrder() {

x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/aggregations/bucket/histogram/HistoBackedHistogramAggregator.java

@@ -36,15 +36,14 @@ public class HistoBackedHistogramAggregator extends AbstractHistogramAggregator
         BucketOrder order,
         boolean keyed,
         long minDocCount,
-        double minBound,
+        DoubleBounds extendedBounds,
-        double maxBound,
         DoubleBounds hardBounds,
         ValuesSourceConfig valuesSourceConfig,
         SearchContext context,
         Aggregator parent,
         CardinalityUpperBound cardinalityUpperBound,
         Map<String, Object> metadata) throws IOException {
-        super(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, hardBounds,
+        super(name, factories, interval, offset, order, keyed, minDocCount, extendedBounds, hardBounds,
             valuesSourceConfig.format(), context, parent, cardinalityUpperBound, metadata);
 
         // TODO: Stop using null here