Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using
DATE_TRUNC
and notHISTOGRAM
? Ideally it would be great if these were consistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
HISTOGRAM
groups the data byINTERVAL
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@john-bodley Here is the follow-up PR: #25717