fix timewarp query results when using timezones and crossing DST transitions by clintropolis · Pull Request #5157 · apache/druid

clintropolis · 2017-12-13T09:21:01Z

changes:

TimewarpOperator will now compensate for daylight savings time shifts between date translation ranges for queries using a PeriodGranularity with a timezone defined
The Query interface has been expanded with 2 new methods, getGranularity and getTimezone.
BaseQuery now has a Granularity and a new constructor that takes an additional argument to set it. When using the original constructor, granularity will be set to Granularities.ALL.
The getTimezone method in BaseQuery will return the PeriodGranularity timezone if it is used, else it will default to DateTimeZone.UTC.
GroupByQuery, SearchQuery, SelectQuery, TimeseriesQuery, and TopNQuery all individually had a granularity, so this change to Query and BaseQuery cuts down on some duplicate code in addition providing a mechanism for TimewarpOperator (and anything else) that needs to be aware of query granularity or timezone.
The changes to BaseQuery result in serialization differences for DataSourceMetadataQuery, SegmentMetadataQuery, ScanQuery, and TimeBoundaryQuery, which previously did not have a granularity, but are now set to Granularities.ALL.

changes: * `TimewarpOperator` will now compensate for daylight savings time shifts between date translation ranges for queries using a `PeriodGranularity` with a timezone defined * introduces a new abstract query type `TimeBucketedQuery` for all queries which have a `Granularity` (100% not attached to this name). `GroupByQuery`, `SearchQuery`, `SelectQuery`, `TimeseriesQuery`, and `TopNQuery` all extend `TimeBucke tedQuery`, cutting down on some duplicate code and providing a mechanism for `TimewarpOperator` (and anything else) that needs to be aware of granularity

leventov · 2017-12-15T16:24:25Z

Labelling Design Review because this PR introduces a new @ExtensionPoint.

leventov · 2017-12-15T16:34:00Z

    for (DimensionSpec spec : this.dimensions) {
      Preconditions.checkArgument(spec != null, "dimensions has null DimensionSpec");
    }
+    Preconditions.checkNotNull(granularity, "Must specify a granularity");


This check should be done in TimeBucketedQuery's constructor

leventov · 2017-12-15T16:35:43Z

  )
  {
-    super(dataSource, querySegmentSpec, descending, context);
+    super(dataSource, querySegmentSpec, descending, context, granularity == null ? Granularities.ALL : granularity);


I suggest to extract method Granularities.nullToAll()

leventov · 2017-12-15T16:36:23Z

  )
  {
-    super(dataSource, querySegmentSpec, descending, context);
+    super(dataSource, querySegmentSpec, descending, context, granularity);


How do timeseries queries behave with null granularity?

I just tested and it throws a null pointer exception, moving the precondition check from group by into TimeBucketedQuery constructor should fix this.

It appears TimeseriesQueryBuilder defaults to Granularities.ALL which is why all of the tests which do not specify a granularity do not explode.

leventov · 2017-12-15T16:39:38Z

   * @return the offset between the mapped time and time t
   */
-  protected long computeOffset(final long t)
+  protected long computeOffset(final long t, final DateTimeZone tz)


Is it possible to refactor this method? Maybe it's code could be simplified?

…oAll, add getTimezone to TimeBucketQuery

leventov · 2017-12-18T17:40:05Z

@clintropolis could you please also give this PR a title that looks more sensible in git commit history.

drcrallen · 2018-01-03T16:32:00Z

All queries are time bucketed.

descending is already a property in BaseQuery

Would it make sense for BaseQuery to be modified to accept granularity as a constructor, instead of making another class layer that every query will want to use anyways?

clintropolis · 2018-01-04T21:29:57Z

@drcrallen The reason I added TimeBucketedQuery is because DataSourceMetadataQuery, SegmentMetadataQuery, ScanQuery, and TimeBoundaryQuery all extend BaseQuery but did not have a granularity. If you think it is preferable to make BaseQuery always have a granularity, I can merge TimeBucketedQuery into it and change these other query types to have an implicit 'All' or 'None', I just wasn't sure if conceptually that made sense (or would have any other unintended side effects).

Thanks for the review!

gianm · 2018-01-04T21:37:07Z

All queries are time bucketed.

They don't all take a granularity parameter, but if you count "hardcoded 'all' granularity" as "time bucketed" then, okay, sure :)

If you think it is preferable to make BaseQuery always have a granularity, I can merge TimeBucketedQuery into it and change these other query types to have an implicit 'All' or 'None', I just wasn't sure if conceptually that made sense (or would have any other unintended side effects).

If this is the way you go, it should be 'all', since 'none' really means 'millis'.

* add 'getGranularity' and 'getTimezone' to 'Query' interface * merge 'TimeBucketedQuery' into 'BaseQuery' * fixup tests from resulting serialization changes

leventov · 2018-01-08T22:52:02Z

@clintropolis there is a true error at least with intellij inspections, closing/reopening a pr won't help

clintropolis · 2018-01-08T23:02:44Z

Oops indeed. But the tests should pass, I think.

clintropolis · 2018-01-08T23:18:23Z

The inspection indicates that of the getGranularity and getTimezone methods I added to BaseQuery, only getTimezone should be on the Query interface, which I'm not sure makes a lot of sense without the granularity since it supplies the timezone. Should I rework to either remove both from Query interface, or does it make sense for them to be there and ignore the inspection somehow?

jon-wei · 2018-01-10T21:33:11Z

I personally think the inspection complaining about getGranularity is excessive, but I guess you could move the two methods to BaseQuery from Query, there aren't any Query implementations that aren't also BaseQuery, I think that would fix the inspection?

jon-wei · 2018-01-11T02:01:12Z

@clintropolis You can also add @SuppressWarnings("unused") to getGranularity() to suppress that inspection error, I would favor that over moving the methods to BaseQuery

…sitions (apache#5157) * timewarp and timezones changes: * `TimewarpOperator` will now compensate for daylight savings time shifts between date translation ranges for queries using a `PeriodGranularity` with a timezone defined * introduces a new abstract query type `TimeBucketedQuery` for all queries which have a `Granularity` (100% not attached to this name). `GroupByQuery`, `SearchQuery`, `SelectQuery`, `TimeseriesQuery`, and `TopNQuery` all extend `TimeBucke tedQuery`, cutting down on some duplicate code and providing a mechanism for `TimewarpOperator` (and anything else) that needs to be aware of granularity * move precondition check to TimeBucketedQuery, add Granularities.nullToAll, add getTimezone to TimeBucketQuery * formatting * more formatting * unused import * changes: * add 'getGranularity' and 'getTimezone' to 'Query' interface * merge 'TimeBucketedQuery' into 'BaseQuery' * fixup tests from resulting serialization changes * dedupe * fix after merge * suppress warning

…ng DST tran… (#5252) * fix timewarp query results when using timezones and crossing DST transitions (#5157) * timewarp and timezones changes: * `TimewarpOperator` will now compensate for daylight savings time shifts between date translation ranges for queries using a `PeriodGranularity` with a timezone defined * introduces a new abstract query type `TimeBucketedQuery` for all queries which have a `Granularity` (100% not attached to this name). `GroupByQuery`, `SearchQuery`, `SelectQuery`, `TimeseriesQuery`, and `TopNQuery` all extend `TimeBucke tedQuery`, cutting down on some duplicate code and providing a mechanism for `TimewarpOperator` (and anything else) that needs to be aware of granularity * move precondition check to TimeBucketedQuery, add Granularities.nullToAll, add getTimezone to TimeBucketQuery * formatting * more formatting * unused import * changes: * add 'getGranularity' and 'getTimezone' to 'Query' interface * merge 'TimeBucketedQuery' into 'BaseQuery' * fixup tests from resulting serialization changes * dedupe * fix after merge * suppress warning * Fix compile issue

…sitions (apache#5157) * timewarp and timezones changes: * `TimewarpOperator` will now compensate for daylight savings time shifts between date translation ranges for queries using a `PeriodGranularity` with a timezone defined * introduces a new abstract query type `TimeBucketedQuery` for all queries which have a `Granularity` (100% not attached to this name). `GroupByQuery`, `SearchQuery`, `SelectQuery`, `TimeseriesQuery`, and `TopNQuery` all extend `TimeBucke tedQuery`, cutting down on some duplicate code and providing a mechanism for `TimewarpOperator` (and anything else) that needs to be aware of granularity * move precondition check to TimeBucketedQuery, add Granularities.nullToAll, add getTimezone to TimeBucketQuery * formatting * more formatting * unused import * changes: * add 'getGranularity' and 'getTimezone' to 'Query' interface * merge 'TimeBucketedQuery' into 'BaseQuery' * fixup tests from resulting serialization changes * dedupe * fix after merge * suppress warning

leventov added Design Review Bug Area - Querying labels Dec 15, 2017

leventov reviewed Dec 15, 2017

View reviewed changes

leventov and others added 5 commits December 15, 2017 23:34

Merge remote-tracking branch 'upstream/master' into timewarp-timezone

9f5a3fe

move precondition check to TimeBucketedQuery, add Granularities.nullT…

b770a96

…oAll, add getTimezone to TimeBucketQuery

formatting

e41fe28

more formatting

140ce00

unused import

a8aae84

leventov approved these changes Dec 18, 2017

View reviewed changes

clintropolis changed the title ~~timewarp and timezones~~ fix timewarp query results when using timezones and crossing DST transitions Dec 18, 2017

leventov added this to the 0.12.0 milestone Dec 19, 2017

Clint Wylie added 2 commits January 5, 2018 13:52

changes:

4eaeb40

* add 'getGranularity' and 'getTimezone' to 'Query' interface * merge 'TimeBucketedQuery' into 'BaseQuery' * fixup tests from resulting serialization changes

dedupe

963173f

clintropolis closed this Jan 6, 2018

clintropolis reopened this Jan 6, 2018

Clint Wylie added 2 commits January 5, 2018 22:59

Merge remote-tracking branch 'upstream/master' into timewarp-timezone

9369035

fix after merge

caf857f

clintropolis closed this Jan 8, 2018

clintropolis reopened this Jan 8, 2018

Merge remote-tracking branch 'upstream/master' into timewarp-timezone

ca77dc3

Clint Wylie added 2 commits January 10, 2018 19:03

suppress warning

2752ee0

Merge remote-tracking branch 'upstream/master' into timewarp-timezone

af693af

jon-wei merged commit 491f8cc into apache:master Jan 11, 2018

jon-wei mentioned this pull request Jan 11, 2018

[Backport] fix timewarp query results when using timezones and crossing DST tran… #5252

Merged

FrankChen021 mentioned this pull request Nov 30, 2020

Fix doc about search query #10616

Merged

8 tasks

Conversation

clintropolis commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leventov commented Dec 15, 2017

Uh oh!

leventov Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

leventov Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

clintropolis Dec 17, 2017

Choose a reason for hiding this comment

Uh oh!

leventov Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

clintropolis Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis Dec 17, 2017

Choose a reason for hiding this comment

Uh oh!

leventov Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

leventov commented Dec 18, 2017

Uh oh!

drcrallen commented Jan 3, 2018

Uh oh!

clintropolis commented Jan 4, 2018

Uh oh!

gianm commented Jan 4, 2018

Uh oh!

leventov commented Jan 8, 2018

Uh oh!

clintropolis commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintropolis commented Jan 8, 2018

Uh oh!

jon-wei commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jon-wei commented Jan 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

clintropolis commented Dec 13, 2017 •

edited

Loading

clintropolis Dec 17, 2017 •

edited

Loading

clintropolis commented Jan 8, 2018 •

edited

Loading

jon-wei commented Jan 10, 2018 •

edited

Loading