Skip to content

Commit 959690a

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

File tree

9 files changed

+112
-75
lines changed

9 files changed

+112
-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: 60 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,10 +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();
139133
if (in.getVersion().onOrAfter(Version.V_7_10_0)) {
134+
extendedBounds = in.readOptionalWriteable(DoubleBounds::new);
140135
hardBounds = in.readOptionalWriteable(DoubleBounds::new);
136+
} else {
137+
double minBound = in.readDouble();
138+
double maxBound = in.readDouble();
139+
if (minBound == Double.POSITIVE_INFINITY && maxBound == Double.NEGATIVE_INFINITY) {
140+
extendedBounds = null;
141+
} else {
142+
extendedBounds = new DoubleBounds(minBound, maxBound);
143+
}
141144
}
142145
}
143146

@@ -148,10 +151,17 @@ protected void innerWriteTo(StreamOutput out) throws IOException {
148151
out.writeVLong(minDocCount);
149152
out.writeDouble(interval);
150153
out.writeDouble(offset);
151-
out.writeDouble(minBound);
152-
out.writeDouble(maxBound);
153154
if (out.getVersion().onOrAfter(Version.V_7_10_0)) {
155+
out.writeOptionalWriteable(extendedBounds);
154156
out.writeOptionalWriteable(hardBounds);
157+
} else {
158+
if (extendedBounds != null) {
159+
out.writeDouble(extendedBounds.getMin());
160+
out.writeDouble(extendedBounds.getMax());
161+
} else {
162+
out.writeDouble(Double.POSITIVE_INFINITY);
163+
out.writeDouble(Double.NEGATIVE_INFINITY);
164+
}
155165
}
156166
}
157167

@@ -182,12 +192,16 @@ public HistogramAggregationBuilder offset(double offset) {
182192

183193
/** Get the current minimum bound that is set on this builder. */
184194
public double minBound() {
185-
return minBound;
195+
return extendedBounds.getMin();
186196
}
187197

188198
/** Get the current maximum bound that is set on this builder. */
189199
public double maxBound() {
190-
return maxBound;
200+
return extendedBounds.getMax();
201+
}
202+
203+
protected DoubleBounds extendedBounds() {
204+
return extendedBounds;
191205
}
192206

193207
/**
@@ -200,17 +214,23 @@ public double maxBound() {
200214
* are not finite.
201215
*/
202216
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 + "]");
217+
return extendedBounds(new DoubleBounds(minBound, maxBound));
218+
}
219+
220+
/**
221+
* Set extended bounds on this builder: buckets between {@code minBound} and
222+
* {@code maxBound} will be created even if no documents fell into these
223+
* buckets.
224+
*
225+
* @throws IllegalArgumentException
226+
* if maxBound is less that minBound, or if either of the bounds
227+
* are not finite.
228+
*/
229+
public HistogramAggregationBuilder extendedBounds(DoubleBounds extendedBounds) {
230+
if (extendedBounds == null) {
231+
throw new IllegalArgumentException("[extended_bounds] must not be null: [" + name + "]");
211232
}
212-
this.minBound = minBound;
213-
this.maxBound = maxBound;
233+
this.extendedBounds = extendedBounds;
214234
return this;
215235
}
216236

@@ -307,14 +327,15 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params)
307327

308328
builder.field(Histogram.MIN_DOC_COUNT_FIELD.getPreferredName(), minDocCount);
309329

310-
if (Double.isFinite(minBound) || Double.isFinite(maxBound)) {
330+
if (extendedBounds != null) {
311331
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-
}
332+
extendedBounds.toXContent(builder, params);
333+
builder.endObject();
334+
}
335+
336+
if (hardBounds != null) {
337+
builder.startObject(Histogram.HARD_BOUNDS_FIELD.getPreferredName());
338+
hardBounds.toXContent(builder, params);
318339
builder.endObject();
319340
}
320341

@@ -332,23 +353,23 @@ protected ValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardC
332353
AggregatorFactory parent,
333354
AggregatorFactories.Builder subFactoriesBuilder) throws IOException {
334355

335-
if (hardBounds != null) {
336-
if (hardBounds.getMax() != null && hardBounds.getMax() < maxBound) {
356+
if (hardBounds != null && extendedBounds != null) {
357+
if (hardBounds.getMax() != null && extendedBounds.getMax() != null && hardBounds.getMax() < extendedBounds.getMax()) {
337358
throw new IllegalArgumentException("Extended bounds have to be inside hard bounds, hard bounds: [" +
338-
hardBounds + "], extended bounds: [" + minBound + "--" + maxBound + "]");
359+
hardBounds + "], extended bounds: [" + extendedBounds.getMin() + "--" + extendedBounds.getMax() + "]");
339360
}
340-
if (hardBounds.getMin() != null && hardBounds.getMin() > minBound) {
361+
if (hardBounds.getMin() != null && extendedBounds.getMin() != null && hardBounds.getMin() > extendedBounds.getMin()) {
341362
throw new IllegalArgumentException("Extended bounds have to be inside hard bounds, hard bounds: [" +
342-
hardBounds + "], extended bounds: [" + minBound + "--" + maxBound + "]");
363+
hardBounds + "], extended bounds: [" + extendedBounds.getMin() + "--" + extendedBounds.getMax() + "]");
343364
}
344365
}
345-
return new HistogramAggregatorFactory(name, config, interval, offset, order, keyed, minDocCount, minBound, maxBound,
366+
return new HistogramAggregatorFactory(name, config, interval, offset, order, keyed, minDocCount, extendedBounds,
346367
hardBounds, queryShardContext, parent, subFactoriesBuilder, metadata);
347368
}
348369

349370
@Override
350371
public int hashCode() {
351-
return Objects.hash(super.hashCode(), order, keyed, minDocCount, interval, offset, minBound, maxBound, hardBounds);
372+
return Objects.hash(super.hashCode(), order, keyed, minDocCount, interval, offset, extendedBounds, hardBounds);
352373
}
353374

354375
@Override
@@ -362,8 +383,7 @@ public boolean equals(Object obj) {
362383
&& Objects.equals(minDocCount, other.minDocCount)
363384
&& Objects.equals(interval, other.interval)
364385
&& Objects.equals(offset, other.offset)
365-
&& Objects.equals(minBound, other.minBound)
366-
&& Objects.equals(maxBound, other.maxBound)
386+
&& Objects.equals(extendedBounds, other.extendedBounds)
367387
&& Objects.equals(hardBounds, other.hardBounds);
368388
}
369389
}

‎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)