-
Notifications
You must be signed in to change notification settings - Fork 25.8k
ES|QL: Improve aggregation over constants handling #112392
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
Open
astefan
wants to merge
26
commits into
elastic:main
Choose a base branch
from
astefan:aggregations_over_constants
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
5159109
Add separate rule for dealing with nulls in aggregations
astefan 74129a0
One more test from one of the reported bugs
astefan ef3960e
Addressing reviews
astefan c2db11c
Make count_distinct deal with Validateable interface
astefan d66004b
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan aedad55
More count_distinct fixes, more tests for percentile's foldable second
astefan 47db145
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan 4c03ce2
Add more tests
astefan 0e8d484
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan 33a8587
Update docs/changelog/112392.yaml
astefan 636a6e0
Address feedback
astefan 0f83b2b
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan 786daa6
Merge branch 'aggregations_over_constants' of https://github.com/aste…
astefan 5000836
More capabilities to csv-spec files
astefan 7034814
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan f27ccd0
More bwc checks
astefan c444a61
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan d6ee3be
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan 020bc9f
Cleanup after merging "main"
astefan 178c383
Skip one more test
astefan 29dd29f
Change one test according to reviews
astefan 8ab89dc
More reviews
astefan db2259b
Bug fix related to the source() used when creating the Validations
astefan db006d6
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan 1ef3b87
Fix some things after pull from main
astefan 968dcfb
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| pr: 112392 | ||
| summary: "ES|QL: Improve aggregation over constants handling" | ||
| area: ES|QL | ||
| type: bug | ||
| issues: | ||
| - 110257 | ||
| - 100170 | ||
| - 104430 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alex-spies marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (More for a follow-up, but): Should we maybe move the aggs on const tests to their own file, like |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
This one is converted to null, which I suppose is not expected from a user perspective. Is this something to be fixed later? Is it related with #108215?
Maybe it should have a comment or something here
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 whole result here looks inconsistent.
I expected
nullsince this is consistent with e.g.SUMandAVGover 0 rows/rows with onlynullvalues.Currently, running the same aggregation on an empty index returns
POINT (NaN NaN), which itself is inconsistent with the fact that we shouldn't haveNaNvalues in our results - but maybe this consistent with geospatial standards?In any case, the 3 results should be the same. But it's fine to fix this in a follow-up issue. Let's figure out what the result should be and throw this inconsistency into an issue (new or existing).
@craigtaverner , is
POINT(NaN NaN)really the result we need to return?Uh oh!
There was an error while loading. Please reload this page.
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.
Per #106025, returning
nullis correct! Thanks @craigtaverner for pointing me to this issue!