Skip to content

Commit bfc62a4

Browse files
imotovimotov
authored
Refactor extendedBounds to use DoubleBounds (#60556)
Refactors extendedBounds to use DoubleBounds instead of 2 variables. This is a follow up for #59175
1 parent e1f2479 commit bfc62a4

File tree

9 files changed

+116
-75
lines changed

9 files changed

+116
-75
lines changed

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

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@
3737
import java.util.Map;
3838
import java.util.function.BiConsumer;
3939

40+
import static org.elasticsearch.search.aggregations.bucket.histogram.DoubleBounds.getEffectiveMax;
41+
import static org.elasticsearch.search.aggregations.bucket.histogram.DoubleBounds.getEffectiveMin;
42+
4043
/**
4144
* Base class for functionality shared between aggregators for this
4245
* {@code histogram} aggregation.
@@ -48,8 +51,7 @@ public abstract class AbstractHistogramAggregator extends BucketsAggregator {
4851
protected final BucketOrder order;
4952
protected final boolean keyed;
5053
protected final long minDocCount;
51-
protected final double minBound;
52-
protected final double maxBound;
54+
protected final DoubleBounds extendedBounds;
5355
protected final DoubleBounds hardBounds;
5456
protected final LongKeyedBucketOrds bucketOrds;
5557

@@ -61,8 +63,7 @@ public AbstractHistogramAggregator(
6163
BucketOrder order,
6264
boolean keyed,
6365
long minDocCount,
64-
double minBound,
65-
double maxBound,
66+
DoubleBounds extendedBounds,
6667
DoubleBounds hardBounds,
6768
DocValueFormat formatter,
6869
SearchContext context,
@@ -80,8 +81,7 @@ public AbstractHistogramAggregator(
8081
order.validate(this);
8182
this.keyed = keyed;
8283
this.minDocCount = minDocCount;
83-
this.minBound = minBound;
84-
this.maxBound = maxBound;
84+
this.extendedBounds = extendedBounds;
8585
this.hardBounds = hardBounds;
8686
this.formatter = formatter;
8787
bucketOrds = LongKeyedBucketOrds.build(context.bigArrays(), cardinalityUpperBound);
@@ -100,7 +100,8 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
100100

101101
EmptyBucketInfo emptyBucketInfo = null;
102102
if (minDocCount == 0) {
103-
emptyBucketInfo = new EmptyBucketInfo(interval, offset, minBound, maxBound, buildEmptySubAggregations());
103+
emptyBucketInfo = new EmptyBucketInfo(interval, offset, getEffectiveMin(extendedBounds),
104+
getEffectiveMax(extendedBounds), buildEmptySubAggregations());
104105
}
105106
return new InternalHistogram(name, buckets, order, minDocCount, emptyBucketInfo, formatter, keyed, metadata());
106107
});
@@ -110,7 +111,8 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
110111
public InternalAggregation buildEmptyAggregation() {
111112
InternalHistogram.EmptyBucketInfo emptyBucketInfo = null;
112113
if (minDocCount == 0) {
113-
emptyBucketInfo = new InternalHistogram.EmptyBucketInfo(interval, offset, minBound, maxBound, buildEmptySubAggregations());
114+
emptyBucketInfo = new InternalHistogram.EmptyBucketInfo(interval, offset, getEffectiveMin(extendedBounds),
115+
getEffectiveMax(extendedBounds), buildEmptySubAggregations());
114116
}
115117
return new InternalHistogram(name, Collections.emptyList(), order, minDocCount, emptyBucketInfo, formatter, keyed, metadata());
116118
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,15 @@ public class DoubleBounds implements ToXContentFragment, Writeable {
7070
* Construct with bounds.
7171
*/
7272
public DoubleBounds(Double min, Double max) {
73+
if (min != null && Double.isFinite(min) == false) {
74+
throw new IllegalArgumentException("min bound must be finite, got: " + min);
75+
}
76+
if (max != null && Double.isFinite(max) == false) {
77+
throw new IllegalArgumentException("max bound must be finite, got: " + max);
78+
}
79+
if (max != null && min != null && max < min) {
80+
throw new IllegalArgumentException("max bound [" + max + "] must be greater than min bound [" + min + "]");
81+
}
7382
this.min = min;
7483
this.max = max;
7584
}
@@ -125,6 +134,20 @@ public Double getMax() {
125134
return max;
126135
}
127136

137+
/**
138+
* returns bounds min if it is defined or POSITIVE_INFINITY otherwise
139+
*/
140+
public static double getEffectiveMin(DoubleBounds bounds) {
141+
return bounds == null || bounds.min == null ? Double.POSITIVE_INFINITY : bounds.min;
142+
}
143+
144+
/**
145+
* returns bounds max if it is defined or NEGATIVE_INFINITY otherwise
146+
*/
147+
public static Double getEffectiveMax(DoubleBounds bounds) {
148+
return bounds == null || bounds.max == null ? Double.NEGATIVE_INFINITY : bounds.max;
149+
}
150+
128151
public boolean contain(double value) {
129152
if (max != null && value > max) {
130153
return false;

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

Lines changed: 64 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,8 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
7272

7373
PARSER.declareLong(HistogramAggregationBuilder::minDocCount, Histogram.MIN_DOC_COUNT_FIELD);
7474

75-
PARSER.declareField((histogram, extendedBounds) -> {
76-
histogram.extendedBounds(extendedBounds[0], extendedBounds[1]);
77-
}, parser -> EXTENDED_BOUNDS_PARSER.apply(parser, null), Histogram.EXTENDED_BOUNDS_FIELD, ObjectParser.ValueType.OBJECT);
75+
PARSER.declareField(HistogramAggregationBuilder::extendedBounds, parser -> DoubleBounds.PARSER.apply(parser, null),
76+
Histogram.EXTENDED_BOUNDS_FIELD, ObjectParser.ValueType.OBJECT);
7877

7978
PARSER.declareField(HistogramAggregationBuilder::hardBounds, parser -> DoubleBounds.PARSER.apply(parser, null),
8079
Histogram.HARD_BOUNDS_FIELD, ObjectParser.ValueType.OBJECT);
@@ -89,9 +88,7 @@ public static void registerAggregators(ValuesSourceRegistry.Builder builder) {
8988

9089
private double interval;
9190
private double offset = 0;
92-
//TODO: Replace with DoubleBounds
93-
private double minBound = Double.POSITIVE_INFINITY;
94-
private double maxBound = Double.NEGATIVE_INFINITY;
91+
private DoubleBounds extendedBounds;
9592
private DoubleBounds hardBounds;
9693
private BucketOrder order = BucketOrder.key(true);
9794
private boolean keyed = false;
@@ -113,8 +110,7 @@ protected HistogramAggregationBuilder(HistogramAggregationBuilder clone,
113110
super(clone, factoriesBuilder, metadata);
114111
this.interval = clone.interval;
115112
this.offset = clone.offset;
116-
this.minBound = clone.minBound;
117-
this.maxBound = clone.maxBound;
113+
this.extendedBounds = clone.extendedBounds;
118114
this.hardBounds = clone.hardBounds;
119115
this.order = clone.order;
120116
this.keyed = clone.keyed;
@@ -134,8 +130,17 @@ public HistogramAggregationBuilder(StreamInput in) throws IOException {
134130
minDocCount = in.readVLong();
135131
interval = in.readDouble();
136132
offset = in.readDouble();
137-
minBound = in.readDouble();
138-
maxBound = in.readDouble();
133+
if (in.getVersion().onOrAfter(Version.V_8_0_0)) {
134+
extendedBounds = in.readOptionalWriteable(DoubleBounds::new);
135+
} else {
136+
double minBound = in.readDouble();
137+
double maxBound = in.readDouble();
138+
if (minBound == Double.POSITIVE_INFINITY && maxBound == Double.NEGATIVE_INFINITY) {
139+
extendedBounds = null;
140+
} else {
141+
extendedBounds = new DoubleBounds(minBound, maxBound);
142+
}
143+
}
139144
if (in.getVersion().onOrAfter(Version.V_7_10_0)) {
140145
hardBounds = in.readOptionalWriteable(DoubleBounds::new);
141146
}
@@ -148,8 +153,17 @@ protected void innerWriteTo(StreamOutput out) throws IOException {
148153
out.writeVLong(minDocCount);
149154
out.writeDouble(interval);
150155
out.writeDouble(offset);
151-
out.writeDouble(minBound);
152-
out.writeDouble(maxBound);
156+
if (out.getVersion().onOrAfter(Version.V_8_0_0)) {
157+
out.writeOptionalWriteable(extendedBounds);
158+
} else {
159+
if (extendedBounds != null) {
160+
out.writeDouble(extendedBounds.getMin());
161+
out.writeDouble(extendedBounds.getMax());
162+
} else {
163+
out.writeDouble(Double.POSITIVE_INFINITY);
164+
out.writeDouble(Double.NEGATIVE_INFINITY);
165+
}
166+
}
153167
if (out.getVersion().onOrAfter(Version.V_7_10_0)) {
154168
out.writeOptionalWriteable(hardBounds);
155169
}
@@ -182,12 +196,16 @@ public HistogramAggregationBuilder offset(double offset) {
182196

183197
/** Get the current minimum bound that is set on this builder. */
184198
public double minBound() {
185-
return minBound;
199+
return extendedBounds.getMin();
186200
}
187201

188202
/** Get the current maximum bound that is set on this builder. */
189203
public double maxBound() {
190-
return maxBound;
204+
return extendedBounds.getMax();
205+
}
206+
207+
protected DoubleBounds extendedBounds() {
208+
return extendedBounds;
191209
}
192210

193211
/**
@@ -200,17 +218,23 @@ public double maxBound() {
200218
* are not finite.
201219
*/
202220
public HistogramAggregationBuilder extendedBounds(double minBound, double maxBound) {
203-
if (Double.isFinite(minBound) == false) {
204-
throw new IllegalArgumentException("minBound must be finite, got: " + minBound);
205-
}
206-
if (Double.isFinite(maxBound) == false) {
207-
throw new IllegalArgumentException("maxBound must be finite, got: " + maxBound);
208-
}
209-
if (maxBound < minBound) {
210-
throw new IllegalArgumentException("maxBound [" + maxBound + "] must be greater than minBound [" + minBound + "]");
221+
return extendedBounds(new DoubleBounds(minBound, maxBound));
222+
}
223+
224+
/**
225+
* Set extended bounds on this builder: buckets between {@code minBound} and
226+
* {@code maxBound} will be created even if no documents fell into these
227+
* buckets.
228+
*
229+
* @throws IllegalArgumentException
230+
* if maxBound is less that minBound, or if either of the bounds
231+
* are not finite.
232+
*/
233+
public HistogramAggregationBuilder extendedBounds(DoubleBounds extendedBounds) {
234+
if (extendedBounds == null) {
235+
throw new IllegalArgumentException("[extended_bounds] must not be null: [" + name + "]");
211236
}
212-
this.minBound = minBound;
213-
this.maxBound = maxBound;
237+
this.extendedBounds = extendedBounds;
214238
return this;
215239
}
216240

@@ -307,14 +331,15 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
307331

308332
builder.field(Histogram.MIN_DOC_COUNT_FIELD.getPreferredName(), minDocCount);
309333

310-
if (Double.isFinite(minBound) || Double.isFinite(maxBound)) {
334+
if (extendedBounds != null) {
311335
builder.startObject(Histogram.EXTENDED_BOUNDS_FIELD.getPreferredName());
312-
if (Double.isFinite(minBound)) {
313-
builder.field("min", minBound);
314-
}
315-
if (Double.isFinite(maxBound)) {
316-
builder.field("max", maxBound);
317-
}
336+
extendedBounds.toXContent(builder, params);
337+
builder.endObject();
338+
}
339+
340+
if (hardBounds != null) {
341+
builder.startObject(Histogram.HARD_BOUNDS_FIELD.getPreferredName());
342+
hardBounds.toXContent(builder, params);
318343
builder.endObject();
319344
}
320345

@@ -332,23 +357,23 @@ protected ValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardC
332357
AggregatorFactory parent,
333358
AggregatorFactories.Builder subFactoriesBuilder) throws IOException {
334359

335-
if (hardBounds != null) {
336-
if (hardBounds.getMax() != null && hardBounds.getMax() < maxBound) {
360+
if (hardBounds != null && extendedBounds != null) {
361+
if (hardBounds.getMax() != null && extendedBounds.getMax() != null && hardBounds.getMax() < extendedBounds.getMax()) {
337362
throw new IllegalArgumentException("Extended bounds have to be inside hard bounds, hard bounds: [" +
338-
hardBounds + "], extended bounds: [" + minBound + "--" + maxBound + "]");
363+
hardBounds + "], extended bounds: [" + extendedBounds.getMin() + "--" + extendedBounds.getMax() + "]");
339364
}
340-
if (hardBounds.getMin() != null && hardBounds.getMin() > minBound) {
365+
if (hardBounds.getMin() != null && extendedBounds.getMin() != null && hardBounds.getMin() > extendedBounds.getMin()) {
341366
throw new IllegalArgumentException("Extended bounds have to be inside hard bounds, hard bounds: [" +
342-
hardBounds + "], extended bounds: [" + minBound + "--" + maxBound + "]");
367+
hardBounds + "], extended bounds: [" + extendedBounds.getMin() + "--" + extendedBounds.getMax() + "]");
343368
}
344369
}
345-
return new HistogramAggregatorFactory(name, config, interval, offset, order, keyed, minDocCount, minBound, maxBound,
370+
return new HistogramAggregatorFactory(name, config, interval, offset, order, keyed, minDocCount, extendedBounds,
346371
hardBounds, queryShardContext, parent, subFactoriesBuilder, metadata);
347372
}
348373

349374
@Override
350375
public int hashCode() {
351-
return Objects.hash(super.hashCode(), order, keyed, minDocCount, interval, offset, minBound, maxBound, hardBounds);
376+
return Objects.hash(super.hashCode(), order, keyed, minDocCount, interval, offset, extendedBounds, hardBounds);
352377
}
353378

354379
@Override
@@ -362,8 +387,7 @@ public boolean equals(Object obj) {
362387
&& Objects.equals(minDocCount, other.minDocCount)
363388
&& Objects.equals(interval, other.interval)
364389
&& Objects.equals(offset, other.offset)
365-
&& Objects.equals(minBound, other.minBound)
366-
&& Objects.equals(maxBound, other.maxBound)
390+
&& Objects.equals(extendedBounds, other.extendedBounds)
367391
&& Objects.equals(hardBounds, other.hardBounds);
368392
}
369393
}

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact
4747
private final BucketOrder order;
4848
private final boolean keyed;
4949
private final long minDocCount;
50-
private final double minBound, maxBound;
50+
private final DoubleBounds extendedBounds;
5151
private final DoubleBounds hardBounds;
5252

5353
static void registerAggregators(ValuesSourceRegistry.Builder builder) {
@@ -66,8 +66,7 @@ public HistogramAggregatorFactory(String name,
6666
BucketOrder order,
6767
boolean keyed,
6868
long minDocCount,
69-
double minBound,
70-
double maxBound,
69+
DoubleBounds extendedBounds,
7170
DoubleBounds hardBounds,
7271
QueryShardContext queryShardContext,
7372
AggregatorFactory parent,
@@ -79,8 +78,7 @@ public HistogramAggregatorFactory(String name,
7978
this.order = order;
8079
this.keyed = keyed;
8180
this.minDocCount = minDocCount;
82-
this.minBound = minBound;
83-
this.maxBound = maxBound;
81+
this.extendedBounds = extendedBounds;
8482
this.hardBounds = hardBounds;
8583
}
8684

@@ -100,15 +98,15 @@ protected Aggregator doCreateInternal(SearchContext searchContext,
10098
aggregatorSupplier.getClass().toString() + "]");
10199
}
102100
HistogramAggregatorSupplier histogramAggregatorSupplier = (HistogramAggregatorSupplier) aggregatorSupplier;
103-
return histogramAggregatorSupplier.build(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound,
101+
return histogramAggregatorSupplier.build(name, factories, interval, offset, order, keyed, minDocCount, extendedBounds,
104102
hardBounds, config, searchContext, parent, cardinality, metadata);
105103
}
106104

107105
@Override
108106
protected Aggregator createUnmapped(SearchContext searchContext,
109107
Aggregator parent,
110108
Map<String, Object> metadata) throws IOException {
111-
return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound,
109+
return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, extendedBounds,
112110
hardBounds, config, searchContext, parent, CardinalityUpperBound.NONE, metadata);
113111
}
114112
}

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,7 @@ Aggregator build(
3838
BucketOrder order,
3939
boolean keyed,
4040
long minDocCount,
41-
double minBound,
42-
double maxBound,
41+
DoubleBounds extendedBounds,
4342
DoubleBounds hardBounds,
4443
ValuesSourceConfig valuesSourceConfig,
4544
SearchContext context,

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,7 @@ public NumericHistogramAggregator(
5252
BucketOrder order,
5353
boolean keyed,
5454
long minDocCount,
55-
double minBound,
56-
double maxBound,
55+
DoubleBounds extendedBounds,
5756
DoubleBounds hardBounds,
5857
ValuesSourceConfig valuesSourceConfig,
5958
SearchContext context,
@@ -69,8 +68,7 @@ public NumericHistogramAggregator(
6968
order,
7069
keyed,
7170
minDocCount,
72-
minBound,
73-
maxBound,
71+
extendedBounds,
7472
hardBounds,
7573
valuesSourceConfig.format(),
7674
context,

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,7 @@ public RangeHistogramAggregator(
4949
BucketOrder order,
5050
boolean keyed,
5151
long minDocCount,
52-
double minBound,
53-
double maxBound,
52+
DoubleBounds extendedBounds,
5453
DoubleBounds hardBounds,
5554
ValuesSourceConfig valuesSourceConfig,
5655
SearchContext context,
@@ -66,8 +65,7 @@ public RangeHistogramAggregator(
6665
order,
6766
keyed,
6867
minDocCount,
69-
minBound,
70-
maxBound,
68+
extendedBounds,
7169
hardBounds,
7270
valuesSourceConfig.format(),
7371
context,

0 commit comments

Comments
 (0)