diff --git a/docs/changelog/139797.yaml b/docs/changelog/139797.yaml new file mode 100644 index 0000000000000..ddad70a494991 --- /dev/null +++ b/docs/changelog/139797.yaml @@ -0,0 +1,7 @@ +pr: 139797 +summary: Fix aggregation on null value +area: ES|QL +type: bug +issues: + - 110257 + - 137544 diff --git a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeForkRestTest.java b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeForkRestTest.java index de7d96666b36e..3ca406846103b 100644 --- a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeForkRestTest.java +++ b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeForkRestTest.java @@ -14,7 +14,11 @@ import java.io.IOException; import java.util.List; -import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.*; +import static org.elasticsearch.xpack.esql.CsvTestUtils.loadCsvSpecValues; +import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.FORK_V9; +import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.METRICS_GROUP_BY_ALL; +import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND; +import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.UNMAPPED_FIELDS; import static org.elasticsearch.xpack.esql.qa.rest.RestEsqlTestCase.hasCapabilities; /** @@ -79,5 +83,10 @@ protected void shouldSkipTest(String testName) throws IOException { ); assumeTrue("Cluster needs to support FORK", hasCapabilities(adminClient(), List.of(FORK_V9.capabilityName()))); + + assumeFalse( + "Tests expecting a _fork column can't be tested as _fork will be dropped", + loadCsvSpecValues(testCase.expectedResults).columnNames().contains("_fork") + ); } } diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvTestUtils.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvTestUtils.java index 6951c93a1da2c..01cd41716ce9b 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvTestUtils.java +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvTestUtils.java @@ -492,7 +492,7 @@ public enum Type { IP_RANGE(InetAddresses::parseCidr, BytesRef.class), DATE_RANGE(EsqlDataTypeConverter::parseDateRange, LongRangeBlockBuilder.LongRange.class), VERSION(v -> new org.elasticsearch.xpack.versionfield.Version(v).toBytesRef(), BytesRef.class), - NULL(s -> null, Void.class), + NULL(s -> s, Void.class), DATETIME( x -> x == null ? null : DateFormatters.from(UTC_DATE_TIME_FORMATTER.parse(x)).toInstant().toEpochMilli(), (l, r) -> l instanceof Long maybeIP ? maybeIP.compareTo((Long) r) : l.toString().compareTo(r.toString()), diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/absent.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/absent.csv-spec index 07338590a4e1f..37baddf80383f 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/absent.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/absent.csv-spec @@ -139,3 +139,25 @@ ROW a = 3 x:boolean true ; + +fixAbsentOnLiteralNull +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null +| STATS ABSENT(x) +; + +ABSENT(x):boolean +true +; + +fixAbsentOnChildLiteralNull +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = 0 +| STATS ABSENT(null) +; + +ABSENT(null):boolean +true +; diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/fuse.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/fuse.csv-spec index eee7bce1d1042..327f80bb9fe61 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/fuse.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/fuse.csv-spec @@ -819,3 +819,47 @@ a | 0.0 b | 0.0 c | 0.0 ; + +fuseWithRowWithNullValues +required_capability: fuse_v6 +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null, _id = "", _index = "", _score = 1.0, my_fork = "" +| LIMIT 1 +| FUSE LINEAR GROUP BY my_fork +; + +_score:double | x:null | _id:keyword | _index:keyword | my_fork:keyword +1 | null | "" | "" | "" +; + +fuseWithForkAndRowWithNullValues +required_capability: fork_v9 +required_capability: fuse_v6 +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null, _id = "", _index = "", _score = 1.0 +| FORK (EVAl y = 2) (EVAl y = 3) +| FUSE LINEAR +; + +_score:double | x:null | _id:keyword | _index:keyword | y:integer | _fork:keyword +2 | null | "" | "" | [2, 3] | [fork1,fork2] +; + +fuseWithForkAndFromWithNullValues +required_capability: fork_v9 +required_capability: fuse_v6 +required_capability: fix_agg_on_null_by_replacing_with_eval + +FROM employees METADATA _id, _index, _score +| EVAL x = null +| FORK ( WHERE emp_no:10001 ) + ( WHERE emp_no:10002 ) +| FUSE +| KEEP x +; + +x:null +null +; diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec index fc037510a11c1..be50713930250 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec @@ -5113,3 +5113,80 @@ FROM employees 29175 |2.0 |281.0 |48248.55 30404 |3.0 |281.0 |48248.55 ; + +fixInlineStatsValuesOnReferenceToNullUncasted +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null +| INLINE STATS VALUES(x) +; + +x:null | VALUES(x):null +null | null +; + +fixInlineStatsValuesOnReferenceToNullCasted +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null::long +| INLINE STATS VALUES(x) +; + +x:long | VALUES(x):long +null | null +; + +fixInlineStatsValuesOnLiteralNull +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = 1 +| INLINE STATS y = VALUES(null) +; + +x:integer | y:null +1 | null +; + +fixInlineStatsAggOnNullGivenMultipleAggs +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW w = 1, x = null, y = null, z = 42 +| INLINE STATS a = MIN(w), b = MIN(x), c = MAX(y), d = MEDIAN(z) +; + +w:integer | x:null | y:null | z:integer | a:integer | b:null | c:null | d:double +1 | null | null | 42 | 1 | null | null | 42 +; + +fixInlineStatsAggOnNullGivenMultipleAggsWithExpressionsAndBy +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW w = 1 * 10, x = null, y = null + 3, z = 42 +| INLINE STATS a = MIN(w) + 1, b = MIN(x), c = MAX(y) by z +; + +w:integer | x:null | y:integer | a:integer | b:null | c:integer | z:integer +10 | null | null | 11 | null | null | 42 +; + +fixInlineStatsAggOnNullWithSpecialReturnValueAggs +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null +| INLINE STATS a = COUNT(x), b = COUNT_DISTINCT(x), c = PRESENT(x), d = ABSENT(x) +; + +x:null | a:long | b:long | c:boolean | d:boolean +null | 0 | 0 | false | true +; + +fixInlineStatsAggOnNullAsChildWithSpecialReturnValueAggs +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = 0 +| INLINE STATS a = COUNT(null), b = COUNT_DISTINCT(null), c = PRESENT(null), d = ABSENT(null) +; + +x:integer | a:long | b:long | c:boolean | d:boolean +0 | 0 | 0 | false | true +; diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/k8s-timeseries-rate.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/k8s-timeseries-rate.csv-spec index f08871f7a170e..4aefefd22ecf6 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/k8s-timeseries-rate.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/k8s-timeseries-rate.csv-spec @@ -507,3 +507,24 @@ max_rate:double 13.173725 ; + +rateOnNulls +required_capability: ts_command_v0 +required_capability: metrics_group_by_all +required_capability: metrics_group_by_all_with_ts_dimensions +required_capability: fix_agg_on_null_by_replacing_with_eval + +TS k8s +| EVAL null_col = null +| STATS + rate = rate(network.total_bytes_in), + null_literal = rate(null), + null_propagated = rate(null_col) +| EVAL rate=ROUND(rate, 4) +| SORT rate DESC +| LIMIT 2; + +null_literal:double | null_propagated:double | rate:double +null | null | 13.1737 +null | null | 10.6491 +; diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/present.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/present.csv-spec index 84e5eceb32358..ceb01beb1acfd 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/present.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/present.csv-spec @@ -162,3 +162,26 @@ ROW a = 3 x:boolean false ; + +fixPresentOnLiteralNull +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null +| STATS PRESENT(x) +; + +PRESENT(x):boolean +false +; + +fixPresentOnChildLiteralNull +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = 0 +| STATS PRESENT(null) +; + +PRESENT(null):boolean +false +; + diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec index c1387e3c5a909..853e01c2c0281 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec @@ -3600,6 +3600,96 @@ a:double | b:double | c:long 6.0 | 6.0 | 8 ; +fixStatsValuesOnReferenceToNullUncasted +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null +| STATS VALUES(x) +; + +VALUES(x):null +null +; + +fixStatsValuesOnReferenceToNullCasted +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null::long +| STATS VALUES(x) +; + +VALUES(x):long +null +; + +fixStatsValuesOnLiteralNull +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = 1 +| STATS y = VALUES(null) +; + +y:null +null +; + +fixStatsAggOnNullGivenMultipleAggs +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW w = 1, x = null, y = null, z = 42 +| STATS a = MIN(w), b = MIN(x), c = MAX(y), d = MEDIAN(z) +; + +a:integer | b:null | c:null | d:double +1 | null | null | 42 +; + +fixStatsAggOnNullGivenMultipleAggsWithExpressionsAndBy +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW w = 1 * 10, x = null, y = null + 3, z = 42 +| STATS a = MIN(w), b = MIN(x), c = MAX(y), d = MEDIAN(z) + 1 by x +; + +a:integer | b:null | c:integer | d:double | x:null +10 | null | null | 43 | null +; + +fixStatsAggOnNullGivenMultipleAggsWithWhereClauses +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null, y = null, z = 1 +| STATS a = MIN(x) WHERE z + 1 == 2, + b = MAX(y) WHERE z + 1 == 3, + c = VALUES(z) WHERE z + 1 == 3 +; + +a:null | b:null | c:integer +null | null | null +; + + +fixStatsAggOnNullWithSpecialReturnValueAggs +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = null +| STATS a = COUNT(x), b = COUNT_DISTINCT(x), c = PRESENT(x), d = ABSENT(x) +; + +a:long | b:long | c:boolean | d:boolean +0 | 0 | false | true +; + +fixStatsAggOnNullAsChildWithSpecialReturnValueAggs +required_capability: fix_agg_on_null_by_replacing_with_eval + +ROW x = 0 +| STATS a = COUNT(null), b = COUNT_DISTINCT(null), c = PRESENT(null), d = ABSENT(null) +; + +a:long | b:long | c:boolean | d:boolean +0 | 0 | false | true +; statsMvConstantGroupByWhere required_capability: fix_stats_mv_constant_fold diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java index dadd731b98049..e6645d942c733 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java @@ -12,6 +12,7 @@ import org.elasticsearch.compute.lucene.read.ValuesSourceReaderOperator; import org.elasticsearch.features.NodeFeature; import org.elasticsearch.rest.action.admin.cluster.RestNodesCapabilitiesAction; +import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceStatsFilteredOrNullAggWithEval; import org.elasticsearch.xpack.esql.plugin.EsqlFeatures; import java.util.ArrayList; @@ -1828,7 +1829,7 @@ public enum Cap { FIX_INLINE_STATS_INCORRECT_PRUNNING(INLINE_STATS.enabled), /** - * {@link org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceStatsFilteredAggWithEval} replaced a stats + * {@link ReplaceStatsFilteredOrNullAggWithEval} replaced a stats * with false filter with null with {@link org.elasticsearch.xpack.esql.expression.function.aggregate.Present} or * {@link org.elasticsearch.xpack.esql.expression.function.aggregate.Absent} */ @@ -1849,6 +1850,14 @@ public enum Cap { */ ENABLE_REDUCE_NODE_LATE_MATERIALIZATION(Build.current().isSnapshot()), + /** + * {@link ReplaceStatsFilteredOrNullAggWithEval} now replaces an + * {@link org.elasticsearch.xpack.esql.expression.function.aggregate.AggregateFunction} with null value with an + * {@link org.elasticsearch.xpack.esql.plan.logical.Eval}. + * https://github.com/elastic/elasticsearch/issues/137544 + */ + FIX_AGG_ON_NULL_BY_REPLACING_WITH_EVAL, + /** * Support for requesting the "_tier" metadata field. */ diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/Foldables.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/Foldables.java index 2fe237c8d9294..78fdc98467f86 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/Foldables.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/Foldables.java @@ -195,7 +195,7 @@ public static double doubleValueOf(Expression field, String sourceText, String f return n.doubleValue(); } throw new EsqlIllegalArgumentException( - Strings.format(null, "[{}] value must be a constant number in [{}], found [{}]", fieldName, sourceText, field) + Strings.format("[%s] value must be a constant number in [%s], found [%s]", fieldName, sourceText, field) ); } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Percentile.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Percentile.java index 4bc738b9e5e2f..f31299e07e6f3 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Percentile.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Percentile.java @@ -35,6 +35,7 @@ import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.FIRST; import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND; import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isFoldable; +import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isNotNull; import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType; import static org.elasticsearch.xpack.esql.expression.Foldables.doubleValueOf; @@ -143,7 +144,7 @@ protected TypeResolution resolveType() { sourceText(), SECOND, "numeric except unsigned_long" - ).and(isFoldable(percentile, sourceText(), SECOND)); + ).and(isFoldable(percentile, sourceText(), SECOND)).and(isNotNull(percentile, sourceText(), SECOND)); } @Override diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java index 40470f257d4bc..383518407e314 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java @@ -60,7 +60,7 @@ import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceOrderByExpressionWithEval; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceRegexMatch; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceRowAsLocalRelation; -import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceStatsFilteredAggWithEval; +import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceStatsFilteredOrNullAggWithEval; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceStringCasingWithInsensitiveEquals; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceTrivialTypeConversions; import org.elasticsearch.xpack.esql.optimizer.rules.logical.SetAsOptimized; @@ -206,7 +206,7 @@ protected static Batch operators() { // TODO: bifunction can now (since we now have just one data types set) be pushed into the rule new SimplifyComparisonsArithmetics(DataType::areCompatible), new ReplaceStringCasingWithInsensitiveEquals(), - new ReplaceStatsFilteredAggWithEval(), + new ReplaceStatsFilteredOrNullAggWithEval(), new ExtractAggregateCommonFilter(), // prune/elimination new PruneFilters(), diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/FoldNull.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/FoldNull.java index d0bf09aacebf1..08124549106f9 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/FoldNull.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/FoldNull.java @@ -43,6 +43,9 @@ public Expression rule(Expression e, LogicalOptimizerContext ctx) { // Non-evaluatable functions stay as a STATS grouping (It isn't moved to an early EVAL like other groupings), // so folding it to null would currently break the plan, as we don't create an attribute/channel for that null value. && e instanceof GroupingFunction.NonEvaluatableGroupingFunction == false + // We cannot fold aggregate functions until we resolve https://github.com/elastic/elasticsearch/issues/100634. + // AggregateMapper cannot handle aggregate functions with literal values. + && e instanceof AggregateFunction == false && e.children().stream().anyMatch(FoldNull::isNull)) { return Literal.of(e, null); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredOrNullAggWithEval.java similarity index 86% rename from x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java rename to x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredOrNullAggWithEval.java index 82d9e00f2b479..6ccb31dd8ce8a 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredOrNullAggWithEval.java @@ -15,6 +15,7 @@ import org.elasticsearch.xpack.esql.core.expression.Literal; import org.elasticsearch.xpack.esql.core.expression.NamedExpression; import org.elasticsearch.xpack.esql.core.tree.Source; +import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.core.util.Holder; import org.elasticsearch.xpack.esql.expression.function.aggregate.Absent; import org.elasticsearch.xpack.esql.expression.function.aggregate.AggregateFunction; @@ -34,11 +35,20 @@ import java.util.List; /** - * Replaces an aggregation function having a false/null filter with an EVAL node. + * Replaces an aggregation function with an EVAL node under 2 conditions. + * + * First, having a false/null filter * 
  *     ... | STATS/INLINE STATS x = someAgg(y) WHERE FALSE {BY z} | ...
  *     =>
- *     ... | STATS/INLINE STATS x = someAgg(y) {BY z} > | EVAL x = NULL | KEEP x{, z} | ...
+ *     ... | EVAL x = NULL | KEEP x{, z} | ...
+ *
+ * + * Second, having an agg on a null value + * 
+ *     ... | STATS/INLINE STATS x = someAgg(null) {BY z} | ...
+ *     =>
+ *     ... | EVAL x = NULL | KEEP x{, z} | ...
  *
* * This rule is applied to both STATS' {@link Aggregate} and {@link InlineJoin} right-hand side {@link Aggregate} plans. @@ -46,7 +56,9 @@ * its right-hand side {@link Aggregate}. * Skipped in local optimizer: once a fragment contains an Agg, this can no longer be pruned, which the rule can do */ -public class ReplaceStatsFilteredAggWithEval extends OptimizerRules.OptimizerRule implements OptimizerRules.CoordinatorOnly { +public class ReplaceStatsFilteredOrNullAggWithEval extends OptimizerRules.OptimizerRule + implements + OptimizerRules.CoordinatorOnly { @Override protected LogicalPlan rule(LogicalPlan plan) { Aggregate aggregate; @@ -69,11 +81,7 @@ protected LogicalPlan rule(LogicalPlan plan) { List newProjections = new ArrayList<>(oldAggSize); for (var ne : aggregate.aggregates()) { - if (ne instanceof Alias alias - && alias.child() instanceof AggregateFunction aggFunction - && aggFunction.hasFilter() - && aggFunction.filter() instanceof Literal literal - && Boolean.FALSE.equals(literal.value())) { + if (ne instanceof Alias alias && alias.child() instanceof AggregateFunction aggFunction && shouldReplace(aggFunction)) { Object value = mapNullToValue(aggFunction); Alias newAlias = alias.replaceChild(Literal.of(aggFunction, value)); @@ -119,6 +127,14 @@ protected LogicalPlan rule(LogicalPlan plan) { return plan; } + private static boolean shouldReplace(AggregateFunction aggFunction) { + return hasFalseFilter(aggFunction) || DataType.isNull(aggFunction.field().dataType()); + } + + private static boolean hasFalseFilter(AggregateFunction aggFunction) { + return aggFunction.hasFilter() && aggFunction.filter() instanceof Literal literal && Boolean.FALSE.equals(literal.value()); + } + private static Object mapNullToValue(AggregateFunction aggFunction) { return switch (aggFunction) { case Count ignored -> 0L; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java index 2eac64b7d4162..dd233f531a6d2 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java @@ -65,33 +65,13 @@ public abstract class AbstractAggregationTestCase extends AbstractFunctionTestCa */ protected static Iterable parameterSuppliersFromTypedDataWithDefaultChecks( List suppliers, - boolean entirelyNullPreservesType, PositionalErrorMessageSupplier positionalErrorMessageSupplier ) { return parameterSuppliersFromTypedData( - errorsForCasesWithoutExamples( - withNoRowsExpectingNull(anyNullIsNull(entirelyNullPreservesType, randomizeBytesRefsOffset(suppliers))), - positionalErrorMessageSupplier - ) + errorsForCasesWithoutExamples(withNoRowsExpectingNull(randomizeBytesRefsOffset(suppliers)), positionalErrorMessageSupplier) ); } - /** - * Converts a list of test cases into a list of parameter suppliers. - * Also, adds a default set of extra test cases. - *

- * Use if possible, as this method may get updated with new checks in the future. - *

- * - * @param entirelyNullPreservesType See {@link #anyNullIsNull(boolean, List)} - */ - protected static Iterable parameterSuppliersFromTypedDataWithDefaultChecks( - List suppliers, - boolean entirelyNullPreservesType - ) { - return parameterSuppliersFromTypedData(anyNullIsNull(entirelyNullPreservesType, randomizeBytesRefsOffset(suppliers))); - } - protected static Iterable parameterSuppliersFromTypedDataWithDefaultChecks(List suppliers) { return parameterSuppliersFromTypedData(withNoRowsExpectingNull(randomizeBytesRefsOffset(suppliers))); } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/ErrorsForCasesWithoutExamplesTestCase.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/ErrorsForCasesWithoutExamplesTestCase.java index ec30831004652..0fae624ae5786 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/ErrorsForCasesWithoutExamplesTestCase.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/ErrorsForCasesWithoutExamplesTestCase.java @@ -12,6 +12,7 @@ import org.elasticsearch.xpack.esql.core.expression.TypeResolutions; import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.core.type.DataType; +import org.elasticsearch.xpack.esql.expression.function.aggregate.AggregateFunction; import org.elasticsearch.xpack.esql.expression.function.scalar.multivalue.MvCountErrorTests; import org.hamcrest.Matcher; @@ -85,6 +86,13 @@ public final void test() { args.add(randomLiteral(type)); } Expression expression = build(Source.synthetic(sourceForSignature(signature)), args); + // Aggs cannot receive NULL typed parameters in its aggregating field since + // https://github.com/elastic/elasticsearch/pull/139797, + // and until https://github.com/elastic/elasticsearch/issues/100634 is solved. + // TODO: This doesn't take into account aggs with multiple aggregating parameters + if (expression instanceof AggregateFunction af && af.field().dataType() == DataType.NULL) { + continue; + } assertTrue("expected unresolved " + expression, expression.typeResolved().unresolved()); assertThat(expression.typeResolved().message(), expectedTypeErrorMatcher(validPerPosition, signature)); checked++; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AvgTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AvgTests.java index da3b137f0a959..5510d23acaf76 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AvgTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AvgTests.java @@ -59,7 +59,7 @@ public static Iterable parameters() { ) ); - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, true); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/HistogramMergeTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/HistogramMergeTests.java index 86049343b32aa..bcb48fffc0a29 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/HistogramMergeTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/HistogramMergeTests.java @@ -51,7 +51,7 @@ public static Iterable parameters() { .map(HistogramMergeTests::makeSupplier) .collect(Collectors.toCollection(() -> suppliers)); - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, false); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MaxTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MaxTests.java index 12deb8334037f..753637783d447 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MaxTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MaxTests.java @@ -204,7 +204,7 @@ public static Iterable parameters() { ) ); - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, false); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianAbsoluteDeviationTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianAbsoluteDeviationTests.java index d925a12d649b3..345f7d6cc7497 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianAbsoluteDeviationTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianAbsoluteDeviationTests.java @@ -39,7 +39,7 @@ public static Iterable parameters() { MultiRowTestCaseSupplier.doubleCases(1, 1000, -Double.MAX_VALUE, Double.MAX_VALUE, true) ).flatMap(List::stream).map(MedianAbsoluteDeviationTests::makeSupplier).toList(); - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, true); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianTests.java index 81996c169dc5d..4a81aab431341 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MedianTests.java @@ -76,7 +76,7 @@ public static Iterable parameters() { ) ); - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, true); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MinTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MinTests.java index 40460bf3e6944..c699565f97dfc 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MinTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/MinTests.java @@ -204,7 +204,7 @@ public static Iterable parameters() { ) ); - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, false); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/PercentileErrorTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/PercentileErrorTests.java index 1a83c72620860..6e8b9c95b4e43 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/PercentileErrorTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/PercentileErrorTests.java @@ -8,6 +8,7 @@ package org.elasticsearch.xpack.esql.expression.function.aggregate; import org.elasticsearch.xpack.esql.core.expression.Expression; +import org.elasticsearch.xpack.esql.core.expression.TypeResolutions; import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.expression.function.ErrorsForCasesWithoutExamplesTestCase; @@ -15,9 +16,11 @@ import org.hamcrest.Matcher; import java.util.List; +import java.util.Locale; import java.util.Set; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; public class PercentileErrorTests extends ErrorsForCasesWithoutExamplesTestCase { @Override @@ -32,6 +35,15 @@ protected Expression build(Source source, List args) { @Override protected Matcher expectedTypeErrorMatcher(List> validPerPosition, List signature) { + if (signature.get(1) == DataType.NULL && validPerPosition.getFirst().contains(signature.get(0))) { + return is( + TypeResolutions.ParamOrdinal.fromIndex(1).name().toLowerCase(Locale.ROOT) + + " argument of [" + + sourceForSignature(signature) + + "] cannot be null, received []" + ); + } + return equalTo(typeErrorMessage(true, validPerPosition, signature, (v, p) -> switch (p) { case 0 -> "exponential_histogram, tdigest or numeric except unsigned_long"; case 1 -> "numeric except unsigned_long"; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/PercentileTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/PercentileTests.java index 0a2c8ba11c52d..380e303a71d5c 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/PercentileTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/PercentileTests.java @@ -63,7 +63,7 @@ public static Iterable parameters() { } } - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, false); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SampleTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SampleTests.java index 36e8be88c6284..b0b8bdd87d412 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SampleTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SampleTests.java @@ -44,7 +44,6 @@ public static Iterable parameters() { for (var limitCaseSupplier : TestCaseSupplier.intCases(1, 100, false)) { Stream.of( - MultiRowTestCaseSupplier.nullCases(1, 1000), MultiRowTestCaseSupplier.intCases(1, 1000, Integer.MIN_VALUE, Integer.MAX_VALUE, true), MultiRowTestCaseSupplier.longCases(1, 1000, Long.MIN_VALUE, Long.MAX_VALUE, true), MultiRowTestCaseSupplier.ulongCases(1, 1000, BigInteger.ZERO, UNSIGNED_LONG_MAX, true), diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/StdDevTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/StdDevTests.java index f070852dafddb..32ef7f4787473 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/StdDevTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/StdDevTests.java @@ -41,7 +41,7 @@ public static Iterable parameters() { MultiRowTestCaseSupplier.doubleCases(1, 1000, -Double.MAX_VALUE, Double.MAX_VALUE, true) ).flatMap(List::stream).map(StdDevTests::makeSupplier).collect(Collectors.toCollection(() -> suppliers)); - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, true); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesErrorTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesErrorTests.java index 36a207b7683c6..e9c39f2db6737 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesErrorTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesErrorTests.java @@ -53,7 +53,8 @@ protected void assertCheckedSignatures(Set> invalidSignatureSampl List.of(DataType.EXPONENTIAL_HISTOGRAM), List.of(DataType.AGGREGATE_METRIC_DOUBLE), List.of(DataType.TDIGEST), - List.of(DataType.HISTOGRAM) + List.of(DataType.HISTOGRAM), + List.of(DataType.NULL) ) ) ); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesTests.java index bca2a1d683a95..ab545e3011e3d 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/ValuesTests.java @@ -65,7 +65,7 @@ public static Iterable parameters() { MultiRowTestCaseSupplier.geohexCases(1, 100) ).flatMap(List::stream).map(ValuesTests::makeSupplier).collect(Collectors.toCollection(() -> suppliers)); - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, false); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/VarianceTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/VarianceTests.java index 99b853b24c3d2..c10308f61cba6 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/VarianceTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/VarianceTests.java @@ -41,7 +41,7 @@ public static Iterable parameters() { MultiRowTestCaseSupplier.doubleCases(1, 1000, -Double.MAX_VALUE, Double.MAX_VALUE, true) ).flatMap(List::stream).map(VarianceTests::makeSupplier).collect(Collectors.toCollection(() -> suppliers)); - return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers, true); + return parameterSuppliersFromTypedDataWithDefaultChecks(suppliers); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/FoldNullTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/FoldNullTests.java index 0f247a1308c73..b6c205fea97a5 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/FoldNullTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/FoldNullTests.java @@ -23,8 +23,8 @@ import org.elasticsearch.xpack.esql.expression.function.aggregate.MedianAbsoluteDeviation; import org.elasticsearch.xpack.esql.expression.function.aggregate.Min; import org.elasticsearch.xpack.esql.expression.function.aggregate.Percentile; -import org.elasticsearch.xpack.esql.expression.function.aggregate.SpatialCentroid; import org.elasticsearch.xpack.esql.expression.function.aggregate.Sum; +import org.elasticsearch.xpack.esql.expression.function.aggregate.Values; import org.elasticsearch.xpack.esql.expression.function.grouping.Bucket; import org.elasticsearch.xpack.esql.expression.function.grouping.Categorize; import org.elasticsearch.xpack.esql.expression.function.scalar.convert.ToString; @@ -160,8 +160,6 @@ public void testGenericNullableExpression() { assertNullLiteral(foldNull(new Cos(EMPTY, NULL))); // string functions assertNullLiteral(foldNull(new LTrim(EMPTY, NULL))); - // spatial - assertNullLiteral(foldNull(new SpatialCentroid(EMPTY, NULL))); // ip assertNullLiteral(foldNull(new CIDRMatch(EMPTY, NULL, List.of(NULL)))); // conversion @@ -182,50 +180,34 @@ public void testNullFoldingDoesNotApplyOnLogicalExpressions() { @SuppressWarnings("unchecked") public void testNullFoldingDoesNotApplyOnAggregate() throws Exception { - List> items = List.of(Max.class, Min.class); + List> items = List.of( + Avg.class, + Count.class, + Max.class, + Median.class, + MedianAbsoluteDeviation.class, + Min.class, + Sum.class, + Values.class + ); for (Class clazz : items) { Constructor ctor = clazz.getConstructor(Source.class, Expression.class); AggregateFunction conditionalFunction = ctor.newInstance(EMPTY, getFieldAttribute("a")); assertEquals(conditionalFunction, foldNull(conditionalFunction)); conditionalFunction = ctor.newInstance(EMPTY, NULL); - assertEquals(NULL, foldNull(conditionalFunction)); + assertEquals(conditionalFunction, foldNull(conditionalFunction)); } - Avg avg = new Avg(EMPTY, getFieldAttribute("a")); - assertEquals(avg, foldNull(avg)); - avg = new Avg(EMPTY, NULL); - assertEquals(new Literal(EMPTY, null, DOUBLE), foldNull(avg)); - - Count count = new Count(EMPTY, getFieldAttribute("a")); - assertEquals(count, foldNull(count)); - count = new Count(EMPTY, NULL); - assertEquals(count, foldNull(count)); - CountDistinct countd = new CountDistinct(EMPTY, getFieldAttribute("a"), getFieldAttribute("a")); assertEquals(countd, foldNull(countd)); countd = new CountDistinct(EMPTY, NULL, NULL); - assertEquals(new Literal(EMPTY, null, LONG), foldNull(countd)); - - Median median = new Median(EMPTY, getFieldAttribute("a")); - assertEquals(median, foldNull(median)); - median = new Median(EMPTY, NULL); - assertEquals(new Literal(EMPTY, null, DOUBLE), foldNull(median)); - - MedianAbsoluteDeviation medianad = new MedianAbsoluteDeviation(EMPTY, getFieldAttribute("a")); - assertEquals(medianad, foldNull(medianad)); - medianad = new MedianAbsoluteDeviation(EMPTY, NULL); - assertEquals(new Literal(EMPTY, null, DOUBLE), foldNull(medianad)); + assertEquals(countd, foldNull(countd)); Percentile percentile = new Percentile(EMPTY, getFieldAttribute("a"), getFieldAttribute("a")); assertEquals(percentile, foldNull(percentile)); percentile = new Percentile(EMPTY, NULL, NULL); - assertEquals(new Literal(EMPTY, null, DOUBLE), foldNull(percentile)); - - Sum sum = new Sum(EMPTY, getFieldAttribute("a")); - assertEquals(sum, foldNull(sum)); - sum = new Sum(EMPTY, NULL); - assertEquals(new Literal(EMPTY, null, DOUBLE), foldNull(sum)); + assertEquals(percentile, foldNull(percentile)); } public void testNullFoldableDoesNotApplyToIsNullAndNotNull() { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEvalTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredOrNullAggWithEvalTests.java similarity index 84% rename from x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEvalTests.java rename to x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredOrNullAggWithEvalTests.java index dac060dca0617..6ee4c74d9ffd2 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEvalTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredOrNullAggWithEvalTests.java @@ -7,6 +7,8 @@ package org.elasticsearch.xpack.esql.optimizer.rules.logical; +import org.elasticsearch.compute.data.BooleanBlock; +import org.elasticsearch.compute.data.LongBlock; import org.elasticsearch.compute.data.LongVectorBlock; import org.elasticsearch.compute.data.Page; import org.elasticsearch.xpack.esql.core.expression.Alias; @@ -31,14 +33,16 @@ import static org.elasticsearch.xpack.esql.EsqlTestUtils.as; import static org.elasticsearch.xpack.esql.core.type.DataType.INTEGER; import static org.elasticsearch.xpack.esql.core.type.DataType.LONG; +import static org.elasticsearch.xpack.esql.core.type.DataType.NULL; import static org.elasticsearch.xpack.esql.optimizer.LogicalPlanOptimizerTests.releaseBuildForInlineStats; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.startsWith; -public class ReplaceStatsFilteredAggWithEvalTests extends AbstractLogicalPlanOptimizerTests { +public class ReplaceStatsFilteredOrNullAggWithEvalTests extends AbstractLogicalPlanOptimizerTests { /** * 
{@code
@@ -799,4 +803,154 @@ public void testReplaceTwoConsecutiveInlineStats_WithFalseFilters() {
         assertThat(aliasCc.child().fold(FoldContext.small()), is(0L));
         as(eval.child(), EsRelation.class);
     }
+
+    /**
+     * 
{@code
+     * Limit[1000[INTEGER],false,false]
+     * \_LocalRelation[[max(x){r}#6],Page{blocks=[ConstantNullBlock[positions=1]]}]
+     * }

+     */
+    public void testReplaceStatsMaxOnNullReferenceWithEvalSingleAgg() {
+        var plan = plan("""
+            row x = null
+            | stats max(x)
+            """);
+
+        var project = as(plan, Limit.class);
+        var source = as(project.child(), LocalRelation.class);
+        assertThat(Expressions.names(source.output()), contains("max(x)"));
+        Page page = source.supplier().get();
+        assertThat(page.getBlockCount(), is(1));
+        assertThat(page.getBlock(0).getPositionCount(), is(1));
+        assertTrue(page.getBlock(0).areAllValuesNull());
+    }
+
+    /**
+     * 
{@code
+     * Project[[y{r}#6]]
+     * \_Eval[[null[NULL] AS y#6]]
+     *   \_Limit[1000[INTEGER],false,false]
+     *     \_LocalRelation[[{e}#7],Page{blocks=[ConstantNullBlock[positions=1]]}]
+     * }

+     */
+    public void testReplaceStatsMaxOnNullLiteralWithEvalSingleAgg() {
+        var plan = plan("""
+            row x = 3
+            | stats y = max(null)
+            """);
+
+        var project = as(plan, Project.class);
+        assertThat(Expressions.names(project.projections()), contains("y"));
+        var eval = as(project.child(), Eval.class);
+        assertThat(eval.fields().size(), is(1));
+
+        var alias = as(eval.fields().getFirst(), Alias.class);
+        assertTrue(alias.child().foldable());
+        assertThat(alias.child().fold(FoldContext.small()), nullValue());
+        assertThat(alias.child().dataType(), is(NULL));
+
+        var limit = as(eval.child(), Limit.class);
+        var source = as(limit.child(), LocalRelation.class);
+    }
+
+    /**
+     * 
{@code
+     * Project[[max(x){r}#9, sum(y){r}#11, x{r}#4]]
+     * \_Eval[[null[NULL] AS max(x)#9]]
+     *   \_Limit[1000[INTEGER],false,false]
+     *     \_Aggregate[[x{r}#4],[SUM(y{r}#6,true[BOOLEAN],PT0S[TIME_DURATION],compensated[KEYWORD]) AS sum(y)#11, x{r}#4]]
+     *       \_LocalRelation[[x{r}#4, y{r}#6],Page{blocks=[ConstantNullBlock[positions=1], IntVectorBlock[vector=..]]}]
+     * }

+     */
+    public void testReplaceStatsMaxOnNullWithEvalAndAgg() {
+        var plan = plan("""
+            row x = null, y = 1
+            | stats max(x),
+                    sum(y)
+              by x
+            """);
+
+        var project = as(plan, Project.class);
+        assertThat(Expressions.names(project.projections()), contains("max(x)", "sum(y)", "x"));
+        var eval = as(project.child(), Eval.class);
+        assertThat(eval.fields().size(), is(1));
+
+        var alias = as(eval.fields().getFirst(), Alias.class);
+        assertTrue(alias.child().foldable());
+        assertThat(alias.child().fold(FoldContext.small()), nullValue());
+        assertThat(alias.child().dataType(), is(NULL));
+
+        var limit = as(eval.child(), Limit.class);
+        var aggregate = as(limit.child(), Aggregate.class);
+        var source = as(aggregate.child(), LocalRelation.class);
+    }
+
+    /**
+     * 
{@code
+     * Project[[a{r}#6, b{r}#8]]
+     * \_Eval[[0[LONG] AS a#6]]
+     *   \_Limit[1000[INTEGER],false,false]
+     *     \_LocalRelation[[b{r}#8],Page{blocks=[BooleanVectorBlock[vector=ConstantBooleanVector[positions=1, value=false]]]}]
+     * }

+     */
+    public void testReplaceStatsOnNullLiteralWithEvalSpecialFunctions() {
+        // COUNT(null) surrogates to COUNT(*) * MV_COUNT(null), and ABSENT(x) surrogates to NOT(PRESENT(x)), so they're not included
+        var plan = plan("""
+            row x = 3
+            | stats a = COUNT_DISTINCT(null), b = PRESENT(null)
+            """);
+
+        var project = as(plan, Project.class);
+        assertThat(Expressions.names(project.projections()), contains("a", "b"));
+        var eval = as(project.child(), Eval.class);
+        assertThat(eval.fields().size(), is(1));
+
+        var alias = as(eval.fields().getFirst(), Alias.class);
+        assertThat(alias.name(), is("a"));
+        assertTrue(alias.child().foldable());
+        assertThat(alias.child().fold(FoldContext.small()), is(0L));
+        assertThat(alias.child().dataType(), is(LONG));
+
+        var limit = as(eval.child(), Limit.class);
+        var source = as(limit.child(), LocalRelation.class);
+        assertThat(Expressions.names(source.output()), contains("b"));
+
+        var page = source.supplier().get();
+        assertThat(page.getBlockCount(), is(1));
+        assertThat(page.getPositionCount(), is(1));
+        assertThat(page.getBlock(0), instanceOf(BooleanBlock.class));
+        assertThat(((BooleanBlock) page.getBlock(0)).getBoolean(0), is(false));
+    }
+
+    /**
+     * 
{@code
+     * Limit[1000[INTEGER],false,false]
+     * \_LocalRelation[[a{r}#7, b{r}#10, c{r}#13],Page{blocks=[
+     *     LongVectorBlock[vector=ConstantLongVector[positions=1, value=0]],
+     *     LongVectorBlock[vector=ConstantLongVector[positions=1, value=0]],
+     *     BooleanVectorBlock[vector=ConstantBooleanVector[positions=1, value=false]]
+     *   ]}]
+     * }

+     */
+    public void testReplaceStatsOnPropagatedNullLiteralWithEvalSpecialFunctions() {
+        // ABSENT(x) surrogates to NOT(PRESENT(x)), so it's not included
+        var plan = plan("""
+            row x = null
+            | stats a = COUNT(x), b = COUNT_DISTINCT(x), c = PRESENT(x)
+            """);
+
+        var limit = as(plan, Limit.class);
+        var source = as(limit.child(), LocalRelation.class);
+        assertThat(Expressions.names(source.output()), contains("a", "b", "c"));
+
+        var page = source.supplier().get();
+        assertThat(page.getBlockCount(), is(3));
+        assertThat(page.getPositionCount(), is(1));
+        assertThat(page.getBlock(0), instanceOf(LongBlock.class));
+        assertThat(((LongBlock) page.getBlock(0)).getLong(0), is(0L));
+        assertThat(page.getBlock(1), instanceOf(LongBlock.class));
+        assertThat(((LongBlock) page.getBlock(1)).getLong(0), is(0L));
+        assertThat(page.getBlock(2), instanceOf(BooleanBlock.class));
+        assertThat(((BooleanBlock) page.getBlock(2)).getBoolean(0), is(false));
+    }
 }