Adds hard_bounds to histogram aggregations by imotov · Pull Request #59175 · elastic/elasticsearch

imotov · 2020-07-07T16:37:03Z

Adds a hard_bounds parameter to explicitly limit the buckets that a histogram
can generate. This is especially useful in case of open ended ranges that can
produce a very large number of buckets.

Closes #50109

Adds a hard_bounds parameter to explicitly limit the buckets that a histogram can generate. This is especially useful in case of open ended ranges that can produce a very large number of buckets. Closes elastic#50109

elasticmachine · 2020-07-07T16:37:05Z

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

not-napoleon

This looks really good, thanks for fielding it. I know some users will be really happy to see this land :)

I left a couple of nits, but nothing major.

not-napoleon · 2020-07-08T14:17:34Z

.../org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregationBuilder.java

        offset = in.readLong();
-        extendedBounds = in.readOptionalWriteable(ExtendedBounds::new);
+        extendedBounds = in.readOptionalWriteable(LongBounds::new);
+        if (in.getVersion().onOrAfter(Version.V_8_0_0)) {


You'll change this after the backport, I presume?

Yes, that's just until backport.

not-napoleon · 2020-07-08T14:18:38Z

.../org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregationBuilder.java

+    /** Set hard bounds on this histogram, specifying boundaries outside which buckets cannot be created. */
+    public DateHistogramAggregationBuilder hardBounds(LongBounds hardBounds) {
+        if (hardBounds == null) {
+            throw new IllegalArgumentException("[hardBounds] must not be null: [" + name + "]");


Why do we not allow setting the bounds to null? It's not a required parameter.

This is consistent with all other optional parameters in this builder like order or externed_bounds.

...ava/org/elasticsearch/search/aggregations/bucket/histogram/DateRangeHistogramAggregator.java

not-napoleon · 2020-07-08T17:56:47Z

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

+
+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
+
+public class DoubleBounds implements ToXContentFragment, Writeable {


Am I correct that there's no common interface between DoubleBound and LongBound because we'd have to autobox in the collection loop? If so, maybe let's just leave a comment to that effect and maybe link the two in Javadoc, so there's at least something indicating there are two implementations here.

I am not sure I follow. Where do you expect them to intersect?

not-napoleon · 2020-07-08T18:07:24Z

...java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java

    private double offset = 0;
    private double minBound = Double.POSITIVE_INFINITY;
    private double maxBound = Double.NEGATIVE_INFINITY;
+    private DoubleBounds hardBounds;


Should we convert minBound and maxBound into a DoubleBounds instance? if not, should probably just leave a comment that it's done this way for legacy reasons, not out of any specific need.

I would like to refactor minBounds and maxBounds in a follow up PR. I just didn't feel like making this one more complicated than necessary. I will add TODO comment.

not-napoleon · 2020-07-08T18:11:19Z

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

        return max;
    }

+    public boolean contain(long time) {


time seems like a confusing name for this parameter. Maybe just value like the double version uses?

...in/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java

not-napoleon · 2020-07-08T18:30:42Z

...rg/elasticsearch/search/aggregations/bucket/histogram/DateRangeHistogramAggregatorTests.java

+        );
+    }
+
+    public void testOverlappingBounds() throws Exception {


I think it would be good to have a test for hard bounds exactly equal to extended bounds. That's a bit of an edge case, and we want to make sure it doesn't break down the line.

…ket/histogram/DateRangeHistogramAggregator.java Co-authored-by: Mark Tozzi <mark.tozzi@gmail.com>

…ket/histogram/RangeHistogramAggregator.java Co-authored-by: Mark Tozzi <mark.tozzi@gmail.com>

…strictive-bounds

imotov · 2020-07-14T14:45:02Z

This PR requires a bit tricky backport, so we will let it slip into 7.10 in order to not disrupt feature freeze today.

Adds a hard_bounds parameter to explicitly limit the buckets that a histogram can generate. This is especially useful in case of open ended ranges that can produce a very large number of buckets.

Refactors extendedBounds to use DoubleBounds instead of 2 variables. This is a follow up for #59175

Refactors extendedBounds to use DoubleBounds instead of 2 variables. This is a follow up for elastic#59175

Refactors extendedBounds to use DoubleBounds instead of 2 variables. This is a follow up for #59175

consulthys · 2020-09-22T11:11:38Z

Thanks for fixing this @imotov 👍

ClaudioConsolmagno · 2021-11-04T15:46:03Z

Should the renaming of ExtendedBounds to LongBounds have appeared in the 7.10 release notes or the migration guide?

Was upgrading elastic and dependencies today and came across this and was looking to see if there was any impact in simply renaming ExtendedBounds to LongBounds in my code so looked first in the notes and couldn't see anything. Had to search the repo until I tracked it down to this PR.

Adds hard_bounds to histogram aggregations

5f63ab8

Adds a hard_bounds parameter to explicitly limit the buckets that a histogram can generate. This is especially useful in case of open ended ranges that can produce a very large number of buckets. Closes elastic#50109

imotov added >enhancement :Analytics/Aggregations Aggregations v8.0.0 v7.9.0 labels Jul 7, 2020

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 7, 2020

imotov requested a review from not-napoleon July 7, 2020 17:32

not-napoleon approved these changes Jul 8, 2020

View reviewed changes

imotov and others added 7 commits July 13, 2020 13:32

Update server/src/main/java/org/elasticsearch/search/aggregations/buc…

5f0b7ad

…ket/histogram/DateRangeHistogramAggregator.java Co-authored-by: Mark Tozzi <mark.tozzi@gmail.com>

Update server/src/main/java/org/elasticsearch/search/aggregations/buc…

f656c75

…ket/histogram/DateRangeHistogramAggregator.java Co-authored-by: Mark Tozzi <mark.tozzi@gmail.com>

Update server/src/main/java/org/elasticsearch/search/aggregations/buc…

986d62f

…ket/histogram/RangeHistogramAggregator.java Co-authored-by: Mark Tozzi <mark.tozzi@gmail.com>

Update server/src/main/java/org/elasticsearch/search/aggregations/buc…

491fa7b

…ket/histogram/RangeHistogramAggregator.java Co-authored-by: Mark Tozzi <mark.tozzi@gmail.com>

Address review comments

7242618

Merge remote-tracking branch 'elastic/master' into issue-50109-add-re…

18c8af5

…strictive-bounds

Fix HistoBackedHistogramAggregator after merge

fbd8a60

imotov merged commit 0af410a into elastic:master Jul 13, 2020

imotov added backport pending and removed v7.9.0 labels Jul 13, 2020

imotov added the v7.10.0 label Jul 15, 2020

imotov removed the backport pending label Jul 16, 2020

imotov mentioned this pull request Jul 17, 2020

Docs: document hard_bounds parameter in histograms #59774

Closed

imotov deleted the issue-50109-add-restrictive-bounds branch July 17, 2020 17:52

imotov mentioned this pull request Jul 31, 2020

Refactor extendedBounds to use DoubleBounds #60556

Merged

imotov added a commit that referenced this pull request Aug 4, 2020

Refactor extendedBounds to use DoubleBounds (#60556)

bfc62a4

Refactors extendedBounds to use DoubleBounds instead of 2 variables. This is a follow up for #59175

imotov added a commit to imotov/elasticsearch that referenced this pull request Aug 4, 2020

Refactor extendedBounds to use DoubleBounds (elastic#60556)

8cc1c50

Refactors extendedBounds to use DoubleBounds instead of 2 variables. This is a follow up for elastic#59175

imotov mentioned this pull request Aug 4, 2020

[7.x] Refactor extendedBounds to use DoubleBounds (#60556) #60681

Merged

imotov added a commit that referenced this pull request Aug 4, 2020

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

959690a

Refactors extendedBounds to use DoubleBounds instead of 2 variables. This is a follow up for #59175

Mpdreamz mentioned this pull request Nov 16, 2020

7.10.1 Meta Ticket elastic/elasticsearch-net#5096

Closed

61 tasks

stevejgordon mentioned this pull request Nov 17, 2020

Add hard_bounds for histograms elastic/elasticsearch-net#5098

Merged

flash1293 mentioned this pull request Mar 4, 2021

Support for number, date and IP range data types elastic/kibana#76971

Merged

1 task

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021


		import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;

		public class DoubleBounds implements ToXContentFragment, Writeable {

Conversation

imotov commented Jul 7, 2020

Uh oh!

elasticmachine commented Jul 7, 2020

Uh oh!

not-napoleon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov commented Jul 14, 2020

Uh oh!

consulthys commented Sep 22, 2020

Uh oh!

ClaudioConsolmagno commented Nov 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants