feat: Add week time grain for Elasticsearch datasets #25683
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -46,6 +46,7 @@ class ElasticSearchEngineSpec(BaseEngineSpec): # pylint: disable=abstract-metho | |||
| TimeGrain.MINUTE: "HISTOGRAM({col}, INTERVAL 1 MINUTE)", | |||
| TimeGrain.HOUR: "HISTOGRAM({col}, INTERVAL 1 HOUR)", | |||
| TimeGrain.DAY: "HISTOGRAM({col}, INTERVAL 1 DAY)", | |||
| TimeGrain.WEEK: "DATE_TRUNC('week', {col})", | |||
There was a problem hiding this comment.
Why are we using DATE_TRUNC and not HISTOGRAM? Ideally it would be great if these were consistent.
There was a problem hiding this comment.
The HISTOGRAM groups the data by INTERVAL but there is no "week" interval in Elasticsearch.
Perhaps there is a refactor needed to use DATE_TRUNC for the other time grains as well? I could do it, but as I am new to this community, could you let me know if I should do it in this PR or open a separate one for refactoring? 😄
There was a problem hiding this comment.
@mikelv92 thanks for the explanation. I think it's ok to merge this PR as is, but if you're interested in doing a fast follow to change all the existing time grains to use DATE_TRUNC—which seems to be the blessed option—that would be great.
There was a problem hiding this comment.
@john-bodley Here is the follow-up PR: #25717
| @@ -0,0 +1,53 @@ | |||
| # Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
Woot! Thanks for adding a test.
john-bodley
changed the title
Co-authored-by: Mikel Vuka <mikel.vuka@zalando.de>
mistercrunch
added
🏷️ bot
Co-authored-by: Mikel Vuka <mikel.vuka@zalando.de>
SUMMARY
This PR adds the week time grain for the Elasticsearch database engine. This is needed if the user wants to see a week granularity in a time series chart.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
before:
after:
TESTING INSTRUCTIONS
To verify the changes:
ADDITIONAL INFORMATION