Fixes time_zone parameter in composite aggregation #45200
Conversation
|
Pinging @elastic/es-analytics-geo |
|
Sorry for the delay @clement-tourriere, slipped through the cracks :( @nik9000 mind reviewing this, since you've been working on composite stuff lately? |
There was a problem hiding this comment.
@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.
| @@ -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); | |||
There was a problem hiding this comment.
Do you think it'd be cleaner to make a
protected ZoneId timeZone()
method on this class and override it in the DateHistogramValuesSourceBuilder?
There was a problem hiding this comment.
I think it would be OK to do that. I will look in detail tomorrow.
Thank you very much for your review.
|
@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! |
|
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 . |
|
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? |
Sure!
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. |
|
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! |
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
|
Superseded by #51172. |
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
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
This PR is a proposal for fixing the issue with time_zone parameter in composite aggregation.
Closes #45199