ES|QL: Fix aggregation on null value

[dimitris-athanasiou]

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:

• 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 #137544
Closes #110257

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)

[elasticsearchmachine]

Hi @dimitris-athanasiou, I've created a changelog YAML for you.

[alex-spies]

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.

[alex-spies]

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 WHERE clauses

[dimitris-athanasiou]

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!

[alex-spies]

Leftover?

[alex-spies]

Also, since this closes #137544, let's add the repro queries from the issue to the spec tests?

[ivancea]

Added tests for FUSE, they work correctly in this PR

[dimitris-athanasiou]

I am getting test failures for the aggregation tests for test cases that contain null. I am not sure how to handle those. I need guidance for these. @bpintea Please let me know when you take a look at the PR.

[bpintea]

@dimitris-athanasiou "in real life", a query like STATS MAX(null) is replaced by a rule called SubstituteSurrogateAggregations with MvMax(null), which has an evaluator for the NULL type (the EvalOperator.CONSTANT_NULL_FACTORY). The AbstractAggregationTestCase#resolveExpression tries to emulate that (#resolveSurrogates). But that requires a literal (like null), not a field of type NULL (which actually does not exist, everything that comes from ES will have a non-NULL type).
So I think we'll probably need to modify the test to cater for this case.

[dimitris-athanasiou]

@bpintea Thanks for the explanation there!

If I get it right, would adding this at the end of resolveSurrogates() be the solution:

if (Expressions.anyMatch(expression.children(), Expressions::isGuaranteedNull)) {
    return new Literal(expression.source(), null, expression.dataType());
}

It seems to make the tests pass.

[bpintea]

If I get it right, would adding this at the end of resolveSurrogates() be the solution:

Maybe... but I feel the solution to that might need to update what we feed into the test, rather how we run the test.
I'd think that resolveSurrogates() shouldn't be changed if we can avoid passing DataType.NULL fields into it.

[elasticsearchmachine]

Hi @dimitris-athanasiou, I've updated the changelog YAML for you.

[ivancea]

LGTM!
As discussed, I'll continue the work on this PR, as it became a blocker for #139417
Mostly, fix the unit tests on nulls, add some extra tests, and review the other comments. And whatever comes from those changes

[elasticsearchmachine]

💔 Backport failed

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 139797