ESQL: AVG aggregation tests and ignore complex surrogates #110579
Merged
Conversation
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
…se-tests-random-cases
# Conflicts: # x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/TypeResolutions.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/TopList.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/FunctionName.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/TopListTests.java
# Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/TopTests.java
…-tests # Conflicts: # docs/reference/esql/functions/aggregation-functions.asciidoc
Contributor
|
Documentation preview: |
Contributor
Author
|
@elasticmachine update branch |
ivancea
added
Team:Analytics
ivancea
commented
Jul 8, 2024
| examples = { | ||
| @Example(file = "stats", tag = "avg"), | ||
| @Example( | ||
| description = "The expression can use inline functions. For example, to calculate the average " |
Contributor
Author
There was a problem hiding this comment.
Added the existing examples here
Collaborator
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Collaborator
|
Pinging @elastic/kibana-esql (ES|QL-ui) |
nik9000
reviewed
Jul 8, 2024
| examples = { | ||
| @Example(file = "stats", tag = "avg"), | ||
| @Example( | ||
| description = "The expression can use inline functions. For example, to calculate the average " |
| expression.children() | ||
| .stream() | ||
| .allMatch(child -> child instanceof FieldAttribute || child instanceof DeepCopy || child instanceof Literal) | ||
| ); |
Member
There was a problem hiding this comment.
I think this is the right thing to do for now because it doesn't block the rest of the aggs. It's really a shame most of the tests for AVG don't pick up, but that's life sometimes. We'll get there.
| MultiRowTestCaseSupplier.doubleCases(1, 1000, -Double.MAX_VALUE, Double.MAX_VALUE, true) | ||
| ).flatMap(List::stream).toList()) { | ||
| suppliers.add(AvgTests.makeSupplier(fieldCaseSupplier)); | ||
| } |
Member
There was a problem hiding this comment.
This is probably clearer as either a fully functional thing with, like .collect(supplier -> suppliers.add(AvgTests.makeSupplier(supplier)). Or fully for loopy without flatMap.
| case INTEGER -> fieldTypedData.multiRowData() | ||
| .stream() | ||
| .map(v -> (Integer) v) | ||
| .collect(Collectors.summarizingInt(Integer::intValue)) |
Member
There was a problem hiding this comment.
I've found that sometimes we don't line up with these fancy methods in Collectors. So long as we do that's all good.
Contributor
Author
There was a problem hiding this comment.
As this works with doubles, I guess the worst that could happen is we comparing doubles with an error margin (Which I think was commented already somewhere?). If some test fails because of that, we can use it as an example and do it
8 tasks
nik9000
approved these changes
Jul 8, 2024
ivancea
added
the
auto-merge-without-approval
tvernum
pushed a commit
that referenced
this pull request
Feb 25, 2025
Some work around aggregation tests, with AVG as an example: - Added tests and autogenerated docs for AVG - As AVG uses "complex" surrogates (A combination of functions), we can't trivially execute them without a complete plan. As I'm not sure it's worth it for most aggregations, I'm skipping those cases for now, as to avoid blocking other aggs tests. The bad side effect of skipping those tests is that most tests in AvgTests are actually ignored (74 of 100)
tvernum
pushed a commit
that referenced
this pull request
Feb 25, 2025
Some work around aggregation tests, with AVG as an example: - Added tests and autogenerated docs for AVG - As AVG uses "complex" surrogates (A combination of functions), we can't trivially execute them without a complete plan. As I'm not sure it's worth it for most aggregations, I'm skipping those cases for now, as to avoid blocking other aggs tests. The bad side effect of skipping those tests is that most tests in AvgTests are actually ignored (74 of 100)
This was referenced Dec 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Some work around aggregation tests, with AVG as an example:
The bad side effect of skipping those tests is that most tests in AvgTests are actually ignored (74 of 100)