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.

Sign up for GitHub

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

Jump to bottom

Fixes time_zone parameter in composite aggregation #45200

Closed
wants to merge 1 commit into from
Closed

Fixes time_zone parameter in composite aggregation #45200

wants to merge 1 commit into from

Conversation

clement-tourriere
Copy link

This PR is a proposal for fixing the issue with time_zone parameter in composite aggregation.

Closes #45199

@dliappis dliappis added the :Analytics/Aggregations Aggregations label Aug 5, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@dliappis dliappis added the >bug label Aug 5, 2019
@polyfractal
Copy link
Contributor

Sorry for the delay @clement-tourriere, slipped through the cracks :(

@nik9000 mind reviewing this, since you've been working on composite stuff lately?

@polyfractal polyfractal requested a review from nik9000 January 13, 2020 20:21
nik9000
nik9000 requested changes Jan 13, 2020
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@clement-tourriere I think there is a simpler way to get this.

Moving the ZoneId member up a level in the class hierarchy makes the fix a little more complex than you have right now because we need to change the serialization logic. And I'm not sure any other sorts of value sources need the time zone.

Are you still interested in this one? If not I'd be happy to take it from here. But if you are then please give it a shot! I'm always excited to have more folks looking at Elasticsearch.

...ava/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java
@@ -276,7 +302,7 @@ public String format() {

public final CompositeValuesSourceConfig build(SearchContext context) throws IOException {
ValuesSourceConfig<?> config = ValuesSourceConfig.resolve(context.getQueryShardContext(),
valueType, field, script, null,null, format);
valueType, field, script, null, timeZone, format);
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it'd be cleaner to make a

protected ZoneId timeZone()

method on this class and override it in the DateHistogramValuesSourceBuilder?

Copy link
Author

Choose a reason for hiding this comment

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

I think it would be OK to do that. I will look in detail tomorrow.
Thank you very much for your review.

@nik9000
Copy link
Member

nik9000 commented Jan 13, 2020

@clement-tourriere, I replied above but I made an error when trying to ping you. So here is a real ping. Sorry if you end up with two pings because of this!

@clement-tourriere
Copy link
Author

I didn't have time to work a lot on this PR these last few days. Maybe it is better if you take it from here @nik9000 .
Concerning the ZoneId member, I think you're right and it's easier to let it in the DateHistogramBuilder for the moment (since date range aggregation is not available in composite for the moment #38650)

@ghost
Copy link

ghost commented Jan 15, 2020

Hi @clement-tourriere, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@nik9000
Copy link
Member

nik9000 commented Jan 15, 2020

I didn't have time to work a lot on this PR these last few days. Maybe it is better if you take it from here @nik9000 .
Concerning the ZoneId member, I think you're right and it's easier to let it in the DateHistogramBuilder for the moment (since date range aggregation is not available in composite for the moment #38650)

Sure!

Hi @clement-tourriere, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

Grumble grumble. @clement-tourriere, is there any chance you could make this robot happy? I'd love to base my fix on your PR and I can only do that if the CLA checker is happy.

@clement-tourriere
Copy link
Author

I'm sorry, I signed the CLA long time ago (5 years I think), I don't know why it's not working anymore. It should be good now.

@nik9000
Copy link
Member

nik9000 commented Jan 15, 2020 •
edited
Loading

I'm sorry, I signed the CLA long time ago (5 years I think), I don't know why it's not working anymore. It should be good now.

Good enough for me!

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Jan 17, 2020
@nik9000
Support time_zone on composite's date_histogram
71a8e18 
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 nik9000 mentioned this pull request Jan 17, 2020
Merged
@nik9000
Copy link
Member

nik9000 commented Jan 17, 2020

Superseded by #51172.

@nik9000 nik9000 closed this Jan 17, 2020
nik9000 added a commit that referenced this pull request Jan 27, 2020
@nik9000
Support time_zone on composite's date_histogram (#51172)
d060d73 
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 Jan 27, 2020
@nik9000
Support time_zone on composite's date_histogram (elastic#51172)
6b0e3a9 
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 nik9000 mentioned this pull request Jan 27, 2020
Merged
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Jan 27, 2020
@nik9000
Support time_zone on composite's date_histogram (elastic#51172)
19b1941 
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 nik9000 mentioned this pull request Jan 27, 2020
Merged
nik9000 added a commit that referenced this pull request Jan 27, 2020
@nik9000
Support time_zone on composite's date_histogram (#51172) (#51491)
faf6bb2 
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 that referenced this pull request Feb 11, 2020
@nik9000
Support time_zone on composite's date_histogram (#51172) (#51494)
5e0ced5 
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
@codebrain codebrain mentioned this pull request Apr 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 requested changes

Assignees
No one assigned
Labels
:Analytics/Aggregations Aggregations >bug
Projects
None yet
Milestone
No milestone
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
@clement-tourriere @elasticmachine @polyfractal @nik9000 @dliappis