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.

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

Support time_zone on composite's date_histogram #51172

Merged
merged 9 commits into from
Jan 27, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jan 17, 2020

We've been parsing the time_zone parameter on date_histogram for a
while but it hasn't done anything. This wires it up.

Closes #45199
Inspired by #45200

We've been parsing the `time_zone` parameter on `date_hitogram` for a
while but it hasn't *done* anything. This wires it up.

Closes elastic#45199
Inspired by elastic#45200
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, but I'm a bit confused why the timezone tests in CompositeAggregatorTests pass without modification?

E.g. since DateHistogramValuesSourceBuilder would set it's own timezone before, so the shard rounding math was correct (e.g. in DateHistogramValuesSourceBuilder#innderBuild()) but the VS resolution would use null and thus the formatter would be in UTC. But with this fix the correct formatter should be used, so I'm a little surprised the tests are still passing.

@nik9000
Copy link
Member Author

nik9000 commented Jan 17, 2020

Changes look good, but I'm a bit confused why the timezone tests in CompositeAggregatorTests pass without modification?

I'll see what I can find out!

@nik9000
Copy link
Member Author

nik9000 commented Jan 20, 2020

@polyfractal it looks like we just didn't have any unit tests using time zones. I added some.

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @nik9000

Copy link
Contributor

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM2, thanks for adding the test :)

(The test I was looking at before was testWithDateHistogramAndTimeZone(), but I'm probably misreading what it's supposed to do, only skimmed over it.)

@nik9000
Copy link
Member Author

nik9000 commented Jan 23, 2020

(The test I was looking at before was testWithDateHistogramAndTimeZone(), but I'm probably misreading what it's supposed to do, only skimmed over it.)

Wow, I can't believe I missed that test! I spent a while staring at it and I think it didn't catch the problem or the change because there weren't any times that changed which interval they are part of with the time zone.

@nik9000 nik9000 merged commit d060d73 into elastic:master Jan 27, 2020
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Jan 27, 2020
We've been parsing the `time_zone` parameter on `date_hitogram` for a
while but it hasn't *done* anything. This wires it up.

Closes elastic#45199
Inspired by elastic#45200
@nik9000
Copy link
Member Author

nik9000 commented Jan 27, 2020

Thanks @polyfractal and @jimczi !

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Jan 27, 2020
We've been parsing the `time_zone` parameter on `date_hitogram` for a
while but it hasn't *done* anything. This wires it up.

Closes elastic#45199
Inspired by elastic#45200
nik9000 added a commit that referenced this pull request Jan 27, 2020
We've been parsing the `time_zone` parameter on `date_hitogram` for a
while but it hasn't *done* anything. This wires it up.

Closes #45199
Inspired by #45200
@polyfractal polyfractal added v7.6.0 and removed v7.6.1 labels Feb 7, 2020
nik9000 added a commit that referenced this pull request Feb 11, 2020
We've been parsing the `time_zone` parameter on `date_hitogram` for a
while but it hasn't *done* anything. This wires it up.

Closes #45199
Inspired by #45200
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Feb 11, 2020
Now that elastic#51172 is fully backported we can fix the `skip` clause in the
bwc tests for it.
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Feb 11, 2020
Now that elastic#51172 is fully backported we can fix the `skip` clause in the
bwc tests for it.
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Feb 11, 2020
Now that elastic#51172 is fully backported we can fix the `skip` clause in the
bwc tests for it.
nik9000 added a commit that referenced this pull request Feb 12, 2020
Now that #51172 is fully backported we can fix the `skip` clause in the
bwc tests for it.
nik9000 added a commit that referenced this pull request Feb 12, 2020
Now that #51172 is fully backported we can fix the `skip` clause in the
bwc tests for it.
nik9000 added a commit that referenced this pull request Feb 12, 2020
Now that #51172 is fully backported we can fix the `skip` clause in the
bwc tests for it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

time_zone parameter in composite aggregation with date_histogram source doesn't work
5 participants