Adds a minimum interval to `auto_date_histogram`. #42814

pcsanwald · 2019-06-03T18:24:43Z

Closes #41757 by adding a new, optional parameter, minimum_interval, to auto_date_histogram.

note: I need to add docs, but the work should be functionally complete. Here's what I did to test:

restricting the roundings passed into to the aggregator.

elasticmachine · 2019-06-03T18:24:45Z

Pinging @elastic/es-analytics-geo

pcsanwald · 2019-06-03T18:25:22Z

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

    }

+    private static List<String> allowedIntervals = Arrays.asList("second", "minute", "hour", "day", "month", "year");


thought about using an enum here, didn't seem to buy too much.

Could possibly re-use Rounding.DateTimeUnit enum, although that also includes quarter. And would have to add string representations to the enum since it doesn't right now. Might not be worth the hassle. Another candidate is DateHistogramAggregationBuilder.DATE_FIELD_UNITS, although that also includes 1w, etc.

Although it does make me wonder, was quarter skipped on purpose and/or some reason to not have it?

EDIT: Might be moot given Colin's comment below

colings86

@pcsanwald I left some comments, thanks for opening the PR

colings86 · 2019-06-04T08:44:14Z

...g/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregatorFactory.java

+                                              RoundingInfo[] roundingInfos,
+                                              SearchContext context,
+                                              AggregatorFactory<?> parent,
+                                              AggregatorFactories.Builder subFactoriesBuilder,
            Map<String, Object> metaData) throws IOException {


nit: Indent isn't inline with the rest of the constructor args here

colings86 · 2019-06-04T08:45:47Z

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

+                                List<PipelineAggregator> pipelineAggregators,
+                                Map<String, Object> metaData) throws IOException {
+
+        super(minimumIntervalExpression, factories, aggregationContext, parent, pipelineAggregators, metaData);


The aggregator name seems to have been replaced by the minimumIntervalExpression, the name arg seems to have been removed entirely.

Also, it's not clear to me why the minimumIntervalExpression needs to be passed to the AutoDateHistogramAggregator?

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

polyfractal

Just a few minor notes on my end :)

We are planning to backport this to 7.x is, right?

polyfractal · 2019-06-05T19:00:45Z

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

    }

+    private static List<String> allowedIntervals = Arrays.asList("second", "minute", "hour", "day", "month", "year");


Could possibly re-use Rounding.DateTimeUnit enum, although that also includes quarter. And would have to add string representations to the enum since it doesn't right now. Might not be worth the hassle. Another candidate is DateHistogramAggregationBuilder.DATE_FIELD_UNITS, although that also includes 1w, etc.

Although it does make me wonder, was quarter skipped on purpose and/or some reason to not have it?

EDIT: Might be moot given Colin's comment below

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

polyfractal · 2019-06-05T19:10:00Z

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

@@ -101,12 +128,16 @@ public AutoDateHistogramAggregationBuilder(String name) {
    public AutoDateHistogramAggregationBuilder(StreamInput in) throws IOException {
        super(in, ValuesSourceType.NUMERIC, ValueType.DATE);
        numBuckets = in.readVInt();
+        if (in.getVersion().onOrAfter(Version.V_8_0_0)) {


Just a procedural note: if we are backporting this to 7.x, the backport will need to change the version, and then we'll need to push a followup commit to master which change the version appropriately. I like to leave a #TODO change version after backport so I don't forget, but that's just personal preference.

Just an FYI since version checks still confuse me from time to time :)

believe I addressed this comment

…ket/histogram/AutoDateHistogramAggregationBuilder.java Co-Authored-By: Zachary Tong <[email protected]>

colings86

@pcsanwald Thanks for making the changes. I left a couple of comments and a suggestion for adding even more security around the RoundingInfo objects

colings86 · 2019-06-07T09:25:42Z

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

            this.rounding = rounding;
            this.roughEstimateDurationMillis = roughEstimateDurationMillis;
            this.unitAbbreviation = unitAbbreviation;
            this.innerIntervals = innerIntervals;
+            this.dateTimeUnit = dateTimeUnit;


can we assert dateTimeUnit != null; here so we definitely catch the case where the allowedIntervals map doesn't contain an entry for the date time unit used in the rounding?

addressed in 95ee1cf

colings86 · 2019-06-07T09:27:09Z

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

    }

+    public static final Map<Rounding.DateTimeUnit, String> allowedIntervals = Map.ofEntries(


nit: since this is a static constant we should give it an uppercase name

addressed in 95ee1cf

colings86 · 2019-06-07T09:31:14Z

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

-            365 * 24 * 60 * 60 * 1000L, "y", 1, 5, 10, 20, 50, 100);
-        return roundings;
+            365 * 24 * 60 * 60 * 1000L, "y",
+            allowedIntervals.get(Rounding.DateTimeUnit.YEAR_OF_CENTURY), 1, 5, 10, 20, 50, 100);


suggestion: instead of having to pass in the same DateTimeUnit twice and relying on them being the same we could change RoundingInfo so it's constructor has the signature:

public RoundingInfo(DateTimeUnit dateTimeUnit, ZoneId timeZone, long roughEstimateDurationMillis, String unitAbbreviation, int... innerIntervals)

The RoundingInfo constructor could then call createRounding() and allowedIntervals.get() and this would ensure we can never have a RoundingInfo where the dateTimeUnit String and the Rounding don't match the same DateTimeUnit?

great suggestion! this simplified the calling logic a lot. addressed in 95ee1cf

pcsanwald · 2019-06-10T00:29:13Z

@elasticmachine update branch

colings86 · 2019-06-10T09:36:59Z

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

+                            long roughEstimateDurationMillis,
+                            String unitAbbreviation,
+                            int... innerIntervals) {
+            this.rounding = createRounding(Rounding.DateTimeUnit.SECOND_OF_MINUTE, timeZone);


This seems wrong, the rounding should be created with the dateTimeUnit no?

colings86 · 2019-06-10T09:38:23Z

.../elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java

            this.roughEstimateDurationMillis = roughEstimateDurationMillis;
            this.unitAbbreviation = unitAbbreviation;
            this.innerIntervals = innerIntervals;
+            assert dateTimeUnit != null;


I think we need to check that the dateTimeUnit is not null, but with Objects.RequireNonNull(). The assert I suggested was meant for this.dateTimeUnit to ensure we never ask for an entry from the map that doesn't exist

colings86 · 2019-06-10T09:39:58Z

...ticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilderTests.java

+        assertThat(roundings.length, equalTo(6));
+    }
+
+}


Can we make these tests check the roundings that are created to make sure they contain the correct rounding levels, not just the number of roundings returned?

…anwald/elasticsearch into auto-interval-datehisto-min-interval

colings86 · 2019-06-10T12:22:19Z

I just realised that we don't have any documentation for this parameter yet. Coudl we add that?

pcsanwald · 2019-06-10T15:56:48Z

pcsanwald · 2019-06-10T17:28:23Z

@elasticmachine test this please

pcsanwald · 2019-06-10T19:20:57Z

@elasticmachine test this please

colings86 · 2019-06-11T09:16:53Z

docs/reference/aggregations/bucket/autodatehistogram-aggregation.asciidoc

@@ -258,6 +258,28 @@ Like with the normal <<search-aggregations-bucket-datehistogram-aggregation, `da
 scripts and value level scripts are supported. This aggregation does not however, support the `min_doc_count`, 
 `extended_bounds` and `order` parameters.  

+==== Minimum Interval parameter
+
+The `minimum_interval` allows the caller to specify the minimum rounding interval that should be used.


Could you add a bit more detail, for example, to explain that the aggregation will start from the specified interval and that it will make the collection more efficient (faster) since it won't waste time collected at lower intervals.

Also we should list the available values for this parameter.

addressed in 388127c

Adds a minimum interval to `auto_date_histogram`. We do this by restricting the roundings passed into to the aggregator.

…#43285) Backports minimum interval to date histogram

Adds a minimum interval to auto_date_histogram. We do this by

984ca5c

restricting the roundings passed into to the aggregator.

pcsanwald added WIP :Analytics/Aggregations Aggregations v8.0.0 labels Jun 3, 2019

pcsanwald requested review from colings86 and polyfractal June 3, 2019 18:24

pcsanwald commented Jun 3, 2019

View reviewed changes

fix serialization bug which caused test failure

0ef2b6f

colings86 reviewed Jun 4, 2019

View reviewed changes

Paul Sanwald added 2 commits June 4, 2019 07:09

Merge branch 'master' into auto-interval-datehisto-min-interval

e8ad7b7

cleaning up some unused changes

c95a65b

$@polyfractal$ polyfractal mentioned this pull request Jun 4, 2019

[TSVB] Introducing Timerange Data Mode for Metric Style Visualizations elastic/kibana#37185

Merged

$polyfractal$

polyfractal reviewed Jun 5, 2019

View reviewed changes

Paul Sanwald and others added 5 commits June 5, 2019 19:40

address code review comments: don't depend on arrays being in sync

f40767e

$@polyfractal$

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

5dcf24e

…ket/histogram/AutoDateHistogramAggregationBuilder.java Co-Authored-By: Zachary Tong <[email protected]>

refactoring slightly to make better use of allowedIntervals

a74104b

merge

9812736

fix failing test

a820ef6

pcsanwald requested review from colings86 and polyfractal June 6, 2019 22:55

colings86 reviewed Jun 7, 2019

View reviewed changes

refactor constructor to simplify calling logic

95ee1cf

pcsanwald requested a review from colings86 June 9, 2019 22:55

Merge branch 'master' into auto-interval-datehisto-min-interval

92c1e78

colings86 reviewed Jun 10, 2019

View reviewed changes

Paul Sanwald added 2 commits June 10, 2019 08:08

improve test coverage and fix silly bug in create roundings

3c5163c

Merge branch 'auto-interval-datehisto-min-interval' of github.com:pcs…

5eac1f0

…anwald/elasticsearch into auto-interval-datehisto-min-interval

colings86 approved these changes Jun 10, 2019

View reviewed changes

add docs for a min interval param

28a5bb4

Merge branch 'master' into auto-interval-datehisto-min-interval

9151df8

colings86 reviewed Jun 11, 2019

View reviewed changes

add docs clarifications

388127c

pcsanwald added backport pending and removed WIP labels Jun 11, 2019

pcsanwald merged commit 6357857 into elastic:master Jun 11, 2019

pcsanwald pushed a commit to pcsanwald/elasticsearch that referenced this pull request Jun 17, 2019

Adds a minimum interval to auto_date_histogram. (elastic#42814)

babd950

Adds a minimum interval to `auto_date_histogram`. We do this by restricting the roundings passed into to the aggregator.

pcsanwald mentioned this pull request Jun 17, 2019

[backport] Adds a minimum interval to auto_date_histogram. (#42814) #43285

Merged

pcsanwald pushed a commit that referenced this pull request Jun 19, 2019

[backport] Adds a minimum interval to auto_date_histogram. (#42814) (…

8578aba

…#43285) Backports minimum interval to date histogram

$@polyfractal$ polyfractal added v7.3.0 and removed backport pending labels Jun 30, 2019

jpountz added the >feature label Jul 5, 2019

Mpdreamz mentioned this pull request Aug 7, 2019

[meta] 7.3 Release elastic/elasticsearch-net#4001

Closed

16 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a minimum interval to `auto_date_histogram`. #42814

Adds a minimum interval to `auto_date_histogram`. #42814

pcsanwald commented Jun 3, 2019

elasticmachine commented Jun 3, 2019

pcsanwald Jun 3, 2019

$@polyfractal$ polyfractal Jun 5, 2019

colings86 left a comment

colings86 Jun 4, 2019

colings86 Jun 4, 2019

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal Jun 5, 2019

$@polyfractal$ polyfractal Jun 5, 2019

pcsanwald Jun 6, 2019

colings86 left a comment

colings86 Jun 7, 2019

pcsanwald Jun 9, 2019

colings86 Jun 7, 2019

pcsanwald Jun 9, 2019 •

edited

Loading

colings86 Jun 7, 2019

pcsanwald Jun 9, 2019

pcsanwald commented Jun 10, 2019

colings86 Jun 10, 2019

colings86 Jun 10, 2019

colings86 Jun 10, 2019

colings86 commented Jun 10, 2019

pcsanwald commented Jun 10, 2019

pcsanwald commented Jun 10, 2019

pcsanwald commented Jun 10, 2019

colings86 Jun 11, 2019

pcsanwald Jun 11, 2019

		}

		private static List<String> allowedIntervals = Arrays.asList("second", "minute", "hour", "day", "month", "year");

		}

		public static final Map<Rounding.DateTimeUnit, String> allowedIntervals = Map.ofEntries(

Adds a minimum interval to auto_date_histogram. #42814

Adds a minimum interval to auto_date_histogram. #42814

Conversation

pcsanwald commented Jun 3, 2019

elasticmachine commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polyfractal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcsanwald Jun 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcsanwald commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colings86 commented Jun 10, 2019

pcsanwald commented Jun 10, 2019

pcsanwald commented Jun 10, 2019

pcsanwald commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adds a minimum interval to `auto_date_histogram`. #42814

Adds a minimum interval to `auto_date_histogram`. #42814

$@polyfractal$ polyfractal left a comment

pcsanwald Jun 9, 2019 •

edited

Loading