Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor extendedBounds to use DoubleBounds #60556

Merged
merged 4 commits into from
Aug 4, 2020
Merged

Refactor extendedBounds to use DoubleBounds #60556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
703c2a7
Refactor extendedBounds to use DoubleBounds
imotov Jul 31, 2020
1ee94c9
Fix typos
imotov Aug 3, 2020
8fecc08
Fix NPE in bounds comparison
imotov Aug 3, 2020
06182f7
Address review comments
imotov Aug 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions ...a/org/elasticsearch/search/aggregations/bucket/histogram/AbstractHistogramAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@
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.
Expand All @@ -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 double maxBound;
protected final DoubleBounds extendedBounds;
protected final DoubleBounds hardBounds;
protected final LongKeyedBucketOrds bucketOrds;

Expand All @@ -61,8 +63,7 @@ public AbstractHistogramAggregator(
BucketOrder order,
boolean keyed,
long minDocCount,
double minBound,
double maxBound,
DoubleBounds extendedBounds,
DoubleBounds hardBounds,
DocValueFormat formatter,
SearchContext context,
Expand All @@ -80,8 +81,7 @@ public AbstractHistogramAggregator(
order.validate(this);
this.keyed = keyed;
this.minDocCount = minDocCount;
this.minBound = minBound;
this.maxBound = maxBound;
this.extendedBounds = extendedBounds;
this.hardBounds = hardBounds;
this.formatter = formatter;
bucketOrds = LongKeyedBucketOrds.build(context.bigArrays(), cardinalityUpperBound);
Expand All @@ -100,7 +100,8 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I

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());
});
Expand All @@ -110,7 +111,8 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
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());
}
Expand Down
23 changes: 23 additions & 0 deletions ...er/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DoubleBounds.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -125,6 +134,20 @@ public Double getMax() {
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;
Expand Down
104 changes: 64 additions & 40 deletions ...a/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,8 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<

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

PARSER.declareField((histogram, extendedBounds) -> {
histogram.extendedBounds(extendedBounds[0], extendedBounds[1]);
}, parser -> EXTENDED_BOUNDS_PARSER.apply(parser, null), Histogram.EXTENDED_BOUNDS_FIELD, ObjectParser.ValueType.OBJECT);
PARSER.declareField(HistogramAggregationBuilder::extendedBounds, parser -> DoubleBounds.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);
Expand All @@ -89,9 +88,7 @@ public static void registerAggregators(ValuesSourceRegistry.Builder builder) {

private double interval;
private double offset = 0;
//TODO: Replace with DoubleBounds
private double minBound = Double.POSITIVE_INFINITY;
private double maxBound = Double.NEGATIVE_INFINITY;
private DoubleBounds extendedBounds;
private DoubleBounds hardBounds;
private BucketOrder order = BucketOrder.key(true);
private boolean keyed = false;
Expand All @@ -113,8 +110,7 @@ protected HistogramAggregationBuilder(HistogramAggregationBuilder clone,
super(clone, factoriesBuilder, metadata);
this.interval = clone.interval;
this.offset = clone.offset;
this.minBound = clone.minBound;
this.maxBound = clone.maxBound;
this.extendedBounds = clone.extendedBounds;
this.hardBounds = clone.hardBounds;
this.order = clone.order;
this.keyed = clone.keyed;
Expand All @@ -134,8 +130,17 @@ public HistogramAggregationBuilder(StreamInput in) throws IOException {
minDocCount = in.readVLong();
interval = in.readDouble();
offset = in.readDouble();
minBound = in.readDouble();
maxBound = in.readDouble();
if (in.getVersion().onOrAfter(Version.V_8_0_0)) {
extendedBounds = 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);
}
}
if (in.getVersion().onOrAfter(Version.V_7_10_0)) {
hardBounds = in.readOptionalWriteable(DoubleBounds::new);
}
Expand All @@ -148,8 +153,17 @@ protected void innerWriteTo(StreamOutput out) throws IOException {
out.writeVLong(minDocCount);
out.writeDouble(interval);
out.writeDouble(offset);
out.writeDouble(minBound);
out.writeDouble(maxBound);
if (out.getVersion().onOrAfter(Version.V_8_0_0)) {
out.writeOptionalWriteable(extendedBounds);
} else {
if (extendedBounds != null) {
out.writeDouble(extendedBounds.getMin());
out.writeDouble(extendedBounds.getMax());
} else {
out.writeDouble(Double.POSITIVE_INFINITY);
out.writeDouble(Double.NEGATIVE_INFINITY);
}
}
if (out.getVersion().onOrAfter(Version.V_7_10_0)) {
out.writeOptionalWriteable(hardBounds);
}
Expand Down Expand Up @@ -182,12 +196,16 @@ public HistogramAggregationBuilder offset(double offset) {

/** 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;
}

/**
Expand All @@ -200,17 +218,23 @@ public double maxBound() {
* are not finite.
*/
public HistogramAggregationBuilder extendedBounds(double minBound, double maxBound) {
if (Double.isFinite(minBound) == false) {
throw new IllegalArgumentException("minBound must be finite, got: " + minBound);
}
if (Double.isFinite(maxBound) == false) {
throw new IllegalArgumentException("maxBound must be finite, got: " + maxBound);
}
if (maxBound < minBound) {
throw new IllegalArgumentException("maxBound [" + maxBound + "] must be greater than minBound [" + minBound + "]");
return extendedBounds(new DoubleBounds(minBound, 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 + "]");
}
this.minBound = minBound;
this.maxBound = maxBound;
this.extendedBounds = extendedBounds;
return this;
}

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

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)) {
builder.field("min", minBound);
}
if (Double.isFinite(maxBound)) {
builder.field("max", maxBound);
}
extendedBounds.toXContent(builder, params);
builder.endObject();
}

if (hardBounds != null) {
builder.startObject(Histogram.HARD_BOUNDS_FIELD.getPreferredName());
hardBounds.toXContent(builder, params);
builder.endObject();
}

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

if (hardBounds != null) {
if (hardBounds.getMax() != null && hardBounds.getMax() < maxBound) {
if (hardBounds != null && extendedBounds != null) {
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
Expand All @@ -362,8 +387,7 @@ public boolean equals(Object obj) {
&& Objects.equals(minDocCount, other.minDocCount)
&& Objects.equals(interval, other.interval)
&& Objects.equals(offset, other.offset)
&& Objects.equals(minBound, other.minBound)
&& Objects.equals(maxBound, other.maxBound)
&& Objects.equals(extendedBounds, other.extendedBounds)
&& Objects.equals(hardBounds, other.hardBounds);
}
}
12 changes: 5 additions & 7 deletions ...va/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -66,8 +66,7 @@ public HistogramAggregatorFactory(String name,
BucketOrder order,
boolean keyed,
long minDocCount,
double minBound,
double maxBound,
DoubleBounds extendedBounds,
DoubleBounds hardBounds,
QueryShardContext queryShardContext,
AggregatorFactory parent,
Expand All @@ -79,8 +78,7 @@ public HistogramAggregatorFactory(String name,
this.order = order;
this.keyed = keyed;
this.minDocCount = minDocCount;
this.minBound = minBound;
this.maxBound = maxBound;
this.extendedBounds = extendedBounds;
this.hardBounds = hardBounds;
}

Expand All @@ -100,15 +98,15 @@ protected Aggregator doCreateInternal(SearchContext searchContext,
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);
}

@Override
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);
}
}
3 changes: 1 addition & 2 deletions ...a/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorSupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ Aggregator build(
BucketOrder order,
boolean keyed,
long minDocCount,
double minBound,
double maxBound,
DoubleBounds extendedBounds,
DoubleBounds hardBounds,
ValuesSourceConfig valuesSourceConfig,
SearchContext context,
Expand Down
6 changes: 2 additions & 4 deletions ...va/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ public NumericHistogramAggregator(
BucketOrder order,
boolean keyed,
long minDocCount,
double minBound,
double maxBound,
DoubleBounds extendedBounds,
DoubleBounds hardBounds,
ValuesSourceConfig valuesSourceConfig,
SearchContext context,
Expand All @@ -69,8 +68,7 @@ public NumericHistogramAggregator(
order,
keyed,
minDocCount,
minBound,
maxBound,
extendedBounds,
hardBounds,
valuesSourceConfig.format(),
context,
Expand Down
6 changes: 2 additions & 4 deletions ...java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ public RangeHistogramAggregator(
BucketOrder order,
boolean keyed,
long minDocCount,
double minBound,
double maxBound,
DoubleBounds extendedBounds,
DoubleBounds hardBounds,
ValuesSourceConfig valuesSourceConfig,
SearchContext context,
Expand All @@ -66,8 +65,7 @@ public RangeHistogramAggregator(
order,
keyed,
minDocCount,
minBound,
maxBound,
extendedBounds,
hardBounds,
valuesSourceConfig.format(),
context,
Expand Down
Loading