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

ES|QL: Improve aggregation over constants handling #112392

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

ES|QL: Improve aggregation over constants handling #112392

Changes from 1 commit
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 Jul 8, 2024
74129a0
One more test from one of the reported bugs
astefan Aug 30, 2024
ef3960e
Addressing reviews
astefan Sep 9, 2024
c2db11c
Make count_distinct deal with Validateable interface
astefan Sep 18, 2024
d66004b
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Sep 18, 2024
aedad55
More count_distinct fixes, more tests for percentile's foldable second
astefan Sep 19, 2024
47db145
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Oct 1, 2024
4c03ce2
Add more tests
astefan Oct 3, 2024
0e8d484
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Oct 3, 2024
33a8587
Update docs/changelog/112392.yaml
astefan Oct 3, 2024
636a6e0
Address feedback
astefan Oct 3, 2024
0f83b2b
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Oct 3, 2024
786daa6
Merge branch 'aggregations_over_constants' of https://github.com/aste…
astefan Oct 3, 2024
5000836
More capabilities to csv-spec files
astefan Oct 4, 2024
7034814
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Oct 4, 2024
f27ccd0
More bwc checks
astefan Oct 7, 2024
c444a61
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Oct 7, 2024
d6ee3be
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Oct 15, 2024
020bc9f
Cleanup after merging "main"
astefan Oct 15, 2024
178c383
Skip one more test
astefan Oct 16, 2024
29dd29f
Change one test according to reviews
astefan Oct 16, 2024
8ab89dc
More reviews
astefan Oct 16, 2024
db2259b
Bug fix related to the source() used when creating the Validations
astefan Oct 18, 2024
db006d6
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Nov 27, 2024
1ef3b87
Fix some things after pull from main
astefan Nov 28, 2024
968dcfb
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Cleanup after merging "main"
  • Loading branch information
@astefan
astefan committed Oct 15, 2024
commit 020bc9f37fef102ebeb3390cd6c55cbb27a988c1
4 changes: 0 additions & 4 deletions .../esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

weighted_avg and top are special in that they can take 2(+) arguments which, in case of weighted_avg, can even be non-foldable. I think we should add tests that show foldable propagation and propagation of nulls into weighted_avg and top as well.

Original file line number Diff line number Diff line change
Expand Up @@ -5615,10 +5615,6 @@ public void testMatchFunctionIsNotNullable() {
);
}

private Literal nullOf(DataType dataType) {
return new Literal(Source.EMPTY, null, dataType);
}

public void testCheckPercentileFoldableSecondArgument() {
Copy link
Contributor

Choose a reason for hiding this comment

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

thought (not for this PR): should we throw tests like this into an own class, or into the VerifierTests? This is not strictly an optimizer tests, we only cannot run it like the remaining verifier tests due to depending on the optimizer to run before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. It must be a ValidatableTests kind of class, because these tests are specific to this approach of performing the validation. I've moved them to a new class. We need a followup to clean the rest of the "Verification" exception checks.

VerificationException e = expectThrows(VerificationException.class, () -> plan("""
from test
Expand Down