ES|QL: Fix aggregation on null value #139797
Conversation
This correctly handles aggregations on null values. Aggregations on constants do not work as per elastic#100634. However, we can do better in terms of handling null. We achieve this by 2 changes: - we do not fold aggregate functions as `AggregateMapper` does not handle `Literal` values. - we reuse the existing `ReplaceStatsFilteredAggWithEval` rule to replace aggs on null with an eval. I have renamed `ReplaceStatsFilteredAggWithEval` to `ReplaceStatsFilteredOrNullAggWithEval` to capture the new scenario where we apply the rule. Closes elastic#137544
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
Team:Analytics
|
Hi @dimitris-athanasiou, I've created a changelog YAML for you. |
dimitris-athanasiou
added
auto-backport
alex-spies
left a comment
alex-spies
left a comment
There was a problem hiding this comment.
Thank you @dimitris-athanasiou !
Sorry for the drive-by, but this got my attention as we've had issues with aggs on consts in the past. Not a full review, but I wanted to understand the main mechanism.
We're performing the same optimization as for STATS some_agg(...) WHERE false - which is semantically the same, as the WHERE false filter is the same as having no rows, or a single null row, being fed into the agg function. This looks correct to me.
There was a problem hiding this comment.
It'd be lovely if we could add a couple more tests, as we're adding something pretty new here:
- tests with different agg functions, esp. the special ones from
ReplaceStatsFilteredAggWithEval#mapNullToValue - tests with more than 1 agg function where 1 or more get null literals, and some where another agg function does not get a null value.
- tests with
BY - tests with
INLINE STATS - tests with per-agg
WHEREclauses
There was a problem hiding this comment.
tests with different agg functions, esp. the special ones from ReplaceStatsFilteredAggWithEval#mapNullToValue
There are already tests for count/count_distinct:
- stats.countNull
- stats.countDistinctNull
Those were both working because those two functions are not nullable and they were escaping null-folding because of that.
I'll definitely add tests for all other points you bring up!
| import static org.hamcrest.Matchers.startsWith; | ||
|
|
||
| //@TestLogging(value = "org.elasticsearch.xpack.esql:TRACE", reason = "debug") | ||
| @TestLogging(value = "org.elasticsearch.xpack.esql:TRACE", reason = "debug") |
There was a problem hiding this comment.
Also, since this closes #137544, let's add the repro queries from the issue to the spec tests?
3 participants
This correctly handles aggregations on null values.
Aggregations on constants do not work as per #100634.
However, we can do better in terms of handling null.
We achieve this by 2 changes:
AggregateMapperdoes not handleLiteralvalues.ReplaceStatsFilteredAggWithEvalrule to replace aggs on null with an eval.I have renamed
ReplaceStatsFilteredAggWithEvaltoReplaceStatsFilteredOrNullAggWithEvalto capture the new scenario where we apply the rule.Closes #137544