Support offset in composite aggs by nik9000 · Pull Request #50609 · elastic/elasticsearch

nik9000 · 2020-01-03T15:41:17Z

Adds support for the offset parameter to the date_histogram source
of composite aggs. The offset parameter is supported by the normal
date_histogram aggregation and is useful for folks that need to
measure things from, say, 6am one day to 6am the next day.

This is implemented by creating a new Rounding that knows how to
handle offsets and delegates to other rounding implementations. That
implementation doesn't fully implement the Rounding contract, namely
nextRoundingValue. That method isn't used by composite aggs so I can't
be sure that any implementation that I add will be correct. I propose to
leave it throwing UnsupportedOperationException until I need it.

Closes #48757

Adds support for the `offset` parameter to the `date_histogram` source of composite aggs. The `offset` parameter is supported by the normal `date_histogram` aggregation and is useful for folks that need to measure things from, say, 6am one day to 6am the next day. This is implemented by creating a new `Rounding` that knows how to handle offsets and delegates to other rounding implementations. That implementation doesn't fully implement the `Rounding` contract, namely `nextRoundingValue`. That method isn't used by composite aggs so I can't be sure that any implementation that I add will be correct. I propose to leave it throwing `UnsupportedOperationException` until I need it. Closes elastic#48757

elasticmachine · 2020-01-03T15:41:20Z

Pinging @elastic/es-docs (>docs)

elasticmachine · 2020-01-03T15:41:21Z

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

nik9000 · 2020-01-03T15:41:51Z

Please have a close look at this one! It has wire changes and it has been more than a year since I made any of those....

nik9000 · 2020-01-03T15:43:09Z

server/src/main/java/org/elasticsearch/common/Rounding.java

+
+        @Override
+        public void innerWriteTo(StreamOutput out) throws IOException {
+            if (out.getVersion().before(Version.V_8_0_0)) {


We don't actually serialize this Rounding but I believe I've got this bit right so I kept it. I'd like to move the other offset code over to this Rounding which means that we will eventually serialize it. At that point we'll get exhaustive tests for this code.

Modulo the version check which I discuss below.

nik9000 · 2020-01-03T15:45:06Z

...org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java

        super(in);
        dateHistogramInterval = new DateIntervalWrapper(in);
        timeZone = in.readOptionalZoneId();
+        if (in.getVersion().after(Version.V_8_0_0)) {


I think this is the way to do this, but it has been a long time. I want to land this change in master and 7.x, but these tests have no change of passing the bwc tests until I backport it.

Now that I think about it, maybe this version check should be V_7_6_0 actually. Help!

The tests caught an error - this at least be onOrAfter but I'll switch it to V_7_6_0 while I'm there.

nik9000 · 2020-01-03T15:46:49Z

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


+*Offset*
+
+include::datehistogram-aggregation.asciidoc[tag=offset-explanation]


I figured because the option is the same I should just include the docs from the normal date histogram.

nik9000 · 2020-01-03T16:24:47Z

Hurray! Integration test failure! Will debug. Good robots, catching things.

nik9000 · 2020-01-03T16:56:27Z

@elasticmachine, run elasticsearch-ci/2

nik9000 · 2020-01-03T18:00:43Z

The default distro failure is due to the thing not being backported. I'll find a way to get this working.

This reverts commit 95da5cb.

nik9000 · 2020-01-03T18:38:48Z

The default distro failure is due to the thing not being backported. I'll find a way to get this working.

I believe I've got this sorted thanks to @DaveCTurner and @cbuescher.

polyfractal

Left a few comments/questions, but I think this looks good. Main question was around the new Rounding class, most of the rest is cosmetic :)

polyfractal · 2020-01-03T18:25:46Z

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


+*Offset*
+
+include::datehistogram-aggregation.asciidoc[tag=offset-explanation]


polyfractal · 2020-01-03T18:52:00Z

server/src/main/java/org/elasticsearch/common/Rounding.java

        }
    }

+    static class OffsetRounding extends Rounding {


Is the intention to migrate the existing users of offsets (regular date histo agg, etc) over to this wrapper? E.g. it seems a bit heavyweight to create a whole new rounding, but if the idea is to followup with changes to the existing users of offset (rather than baking the logic into the aggs) it makes sense to me.

Also, we should probably add a javadoc explaining why/when to use this class

Yeah, I did intend to migrate the rest over of the uses of offset over to it.

I added javadoc on the public interface to the class which is what the rest of the classes in this file do. Do you think I should duplicate it onto this one? Or add something maybe?

Ah, missed that it was documented on the public method. Since the rest of the class does it that way, fine with me :)

Thanks for the explanation!

polyfractal · 2020-01-03T18:52:44Z

server/src/main/java/org/elasticsearch/common/Rounding.java

+
+        OffsetRounding(StreamInput in) throws IOException {
+            delegate = Rounding.read(in);
+            offset = in.readLong();


maybe read/write ZLong instead? I suspect offsets will be small-to-medium'ish sized and either positive or negative, so zlong might be a win?

In other places offsets are written as long and I just copied it. But I'd be fine with zlong.

Pushed the switch to zlong.

polyfractal · 2020-01-03T18:54:14Z

server/src/main/java/org/elasticsearch/common/Rounding.java

            case TimeUnitRounding.ID:
-                rounding = new TimeUnitRounding(in);
-                break;
+                return new TimeUnitRounding(in);


🙏 for the cleanup here :)

I'm not sure everyone would consider that cleaner, but I sure do!

polyfractal · 2020-01-03T19:03:49Z

server/src/test/java/org/elasticsearch/common/RoundingTests.java

@@ -195,6 +195,16 @@ public void testTimeUnitRoundingDST() {
        assertThat(tzRounding_chg.round(time("2014-11-02T06:01:01", chg)), isDate(time("2014-11-02T06:00:00", chg), chg));
    }



Hmm, do you know if we have any serialization tests for Rounding? I was looking to see if we test that the id() bytes are correct and don't accidentally change (similar to what we do with AbstractWriteableEnumTestCase enum tests)... but couldn't find any serialization tests at all.

If we have them somewhere, let's add a test for the id byte. If not, probably too much to add to this PR but we should file a ticket so we don't forget to add some tests... makes me uneasy that such a widespread class doesn't have serialization tests :)

I believe we end up testing serialization as part of testing things like extended bounds bucket response. Adding a unit test for just this class's serialization makes sense to me though. I figured I'd wait until I used it in a context where we serialized it but since I'm writing the serialization code now I probably ought to write the test now.

I pushed a wire test case.

polyfractal · 2020-01-03T19:04:29Z

server/src/test/java/org/elasticsearch/common/RoundingTests.java

        assertThat(tzRounding_chg.round(time("2014-11-02T06:01:01", chg)), isDate(time("2014-11-02T06:00:00", chg), chg));
    }

+    public void testOffsetRounding() {


Let's add another test that does negative offsets too?

I pushed a test with negative offsets.

The serialization was subtly wrong, particularly in the Eire time zone.

nik9000 · 2020-01-03T20:44:29Z

@polyfractal, I've pushed everything you asked for.

nik9000 · 2020-01-03T20:46:17Z

I reworked the serialization a bit, moving the write methods so they are near the read methods and replacing writeString(zone.getId()) with writeZoneId(zone). More importantly, I replaced DateUtil.of(readString()) with readZoneId. The former changes the zone on load, making the hashcode and equals inconsistent.

nik9000 · 2020-01-03T20:47:30Z

server/src/main/java/org/elasticsearch/common/Rounding.java

            }
            TimeUnitRounding other = (TimeUnitRounding) obj;
-            return Objects.equals(unit, other.unit) && Objects.equals(timeZone, other.timeZone);
+            return unit == other.unit && timeZone.equals(other.timeZone);


I'll revert this change. It snuck in because I thought the comparison was wrong.

nik9000 · 2020-01-03T20:49:18Z

server/src/main/java/org/elasticsearch/common/Rounding.java

+
+        @Override
+        public String toString() {
+            return "Rounding[" + interval + " in " + timeZone + "]";


The toString implementations weren't consistent which made reading the error messages hard. So I changed them.

Adds support for the `offset` parameter to the `date_histogram` source of composite aggs. The `offset` parameter is supported by the normal `date_histogram` aggregation and is useful for folks that need to measure things from, say, 6am one day to 6am the next day. This is implemented by creating a new `Rounding` that knows how to handle offsets and delegates to other rounding implementations. That implementation doesn't fully implement the `Rounding` contract, namely `nextRoundingValue`. That method isn't used by composite aggs so I can't be sure that any implementation that I add will be correct. I propose to leave it throwing `UnsupportedOperationException` until I need it. Closes elastic#48757

nik9000 · 2020-01-09T18:19:37Z

Thanks for the reviews @polyfractal and @jimczi !

Adds support for the `offset` parameter to the `date_histogram` source of composite aggs. The `offset` parameter is supported by the normal `date_histogram` aggregation and is useful for folks that need to measure things from, say, 6am one day to 6am the next day. This is implemented by creating a new `Rounding` that knows how to handle offsets and delegates to other rounding implementations. That implementation doesn't fully implement the `Rounding` contract, namely `nextRoundingValue`. That method isn't used by composite aggs so I can't be sure that any implementation that I add will be correct. I propose to leave it throwing `UnsupportedOperationException` until I need it. Closes #48757

When deserializing time zones in the Rounding classes we used to include a tiny normalization step via `DateUtils.of(in.readString())` that was lost in elastic#50609. Its at least necessary for some tests, e.g. the cause of elastic#50827 is that when sending the default time zone ZoneOffset.UTC on a stream pre 7.0 we convert it to a "UTC" string id via `DateUtils.zoneIdToDateTimeZone`. This gets then read back as a UTC ZoneRegion, which should behave the same but fails the equality tests in our serialization tests. Reverting to the previous behaviour with an additional normalization step on 7.x.

When I implemented elastic#50609 I suppressed a few backwards compatibility checks until I finished the backport. I've now finished the backport so this enables the tests.

When I implemented #50609 I suppressed a few backwards compatibility checks until I finished the backport. I've now finished the backport so this enables the tests.

We added a new rounding in elastic#50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset.

When deserializing time zones in the Rounding classes we used to include a tiny normalization step via `DateUtils.of(in.readString())` that was lost in #50609. Its at least necessary for some tests, e.g. the cause of #50827 is that when sending the default time zone ZoneOffset.UTC on a stream pre 7.0 we convert it to a "UTC" string id via `DateUtils.zoneIdToDateTimeZone`. This gets then read back as a UTC ZoneRegion, which should behave the same but fails the equality tests in our serialization tests. Reverting to the previous behaviour with an additional normalization step on 7.x. Co-authored-by: Nik Everett <nik9000@gmail.com> Closes #50827

We added a new rounding in #50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset.

…0873) We added a new rounding in elastic#50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset.

…50978) We added a new rounding in #50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset.

Adds support for the `offset` parameter to the `date_histogram` source of composite aggs. The `offset` parameter is supported by the normal `date_histogram` aggregation and is useful for folks that need to measure things from, say, 6am one day to 6am the next day. This is implemented by creating a new `Rounding` that knows how to handle offsets and delegates to other rounding implementations. That implementation doesn't fully implement the `Rounding` contract, namely `nextRoundingValue`. That method isn't used by composite aggs so I can't be sure that any implementation that I add will be correct. I propose to leave it throwing `UnsupportedOperationException` until I need it. Closes elastic#48757

When I implemented elastic#50609 I suppressed a few backwards compatibility checks until I finished the backport. I've now finished the backport so this enables the tests.

We added a new rounding in elastic#50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset.

We added a new rounding in #50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset. This is a redo of #50873 with more integration tests. This reverts commit d114c9d.

) We added a new rounding in elastic#50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset. This is a redo of elastic#50873 with more integration tests. This reverts commit d114c9d.

…51495) We added a new rounding in #50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset. This is a redo of #50873 with more integration tests. This reverts commit d114c9d.

nik9000 added >enhancement >docs General docs changes :Analytics/Aggregations Aggregations v8.0.0 v7.6.0 labels Jan 3, 2020

nik9000 requested review from jimczi and polyfractal January 3, 2020 15:41

nik9000 commented Jan 3, 2020

View reviewed changes

version checks

b8f692e

nik9000 added 3 commits January 3, 2020 13:08

use 8.0.0

95da5cb

Revert "use 8.0.0"

e7eea38

This reverts commit 95da5cb.

Disable bwc tests

393388c

$polyfractal$

polyfractal reviewed Jan 3, 2020

View reviewed changes

nik9000 added 3 commits January 3, 2020 14:43

Merge branch 'master' into composite_date_offset

14de4c8

Rework public interface and add wire tests

b271143

The serialization was subtly wrong, particularly in the Eire time zone.

zlong

08fc267

nik9000 commented Jan 3, 2020

View reviewed changes

Revert equals

2113c0e

nik9000 commented Jan 3, 2020

View reviewed changes

Fix from review

fc77e34

nik9000 merged commit 326d696 into elastic:master Jan 7, 2020

nik9000 added the backport pending label Jan 7, 2020

nik9000 removed the backport pending label Jan 9, 2020

cbuescher mentioned this pull request Jan 10, 2020

Fix time zone issue in Rounding serialization #50845

Merged

nik9000 mentioned this pull request Jan 10, 2020

Drop BWC suppressions for composite aggs #50857

Merged

nik9000 added a commit that referenced this pull request Jan 10, 2020

Drop BWC suppressions for composite aggs (#50857)

dfccbf0

When I implemented #50609 I suppressed a few backwards compatibility checks until I finished the backport. I've now finished the backport so this enables the tests.

nik9000 mentioned this pull request Jan 10, 2020

Begin moving date_histogram to offset rounding #50873

Merged

nik9000 mentioned this pull request Jan 14, 2020

Begin moving date_histogram to offset rounding (backport of #50873) #50978

Merged

nik9000 mentioned this pull request Jan 21, 2020

Begin moving date_histogram to offset rounding (take two) #51271

Merged

nik9000 mentioned this pull request Jan 27, 2020

Begin moving date_histogram to offset rounding (take two) (#51271) #51495

Merged

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket elastic/elasticsearch-net#4525

Closed

38 tasks

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


		Offset

		include::datehistogram-aggregation.asciidoc[tag=offset-explanation]

		@@ -195,6 +195,16 @@ public void testTimeUnitRoundingDST() {
		assertThat(tzRounding_chg.round(time("2014-11-02T06:01:01", chg)), isDate(time("2014-11-02T06:00:00", chg), chg));
		}

Conversation

nik9000 commented Jan 3, 2020

Uh oh!

elasticmachine commented Jan 3, 2020

Uh oh!

elasticmachine commented Jan 3, 2020

Uh oh!

nik9000 commented Jan 3, 2020

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!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jan 3, 2020

Uh oh!

nik9000 commented Jan 3, 2020

Uh oh!

nik9000 commented Jan 3, 2020

Uh oh!

nik9000 commented Jan 3, 2020

Uh oh!

polyfractal 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jan 3, 2020

Uh oh!

nik9000 commented Jan 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jan 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

$@polyfractal$ polyfractal left a comment