Skip to content

Commit f3ccb70

Browse files
authored
ESQL: Fix INLINE STATS GROUP BY null being incorrectly pruned (elastic#140027)
This PR fixes the issue where `INLINE STATS GROUP BY null` was being incorrectly pruned by `PruneLeftJoinOnNullMatchingField`. Fixes elastic#139887 ## Problem For query: ``` FROM employees | INLINE STATS c = COUNT(*) BY n = null | KEEP c, n | LIMIT 3 ``` During `LogicalPlanOptimizer`: ``` Limit[3[INTEGER],false,false] \_EsqlProject[[c{r}#2, n{r}#4]] \_InlineJoin[LEFT,[n{r}#4],[n{r}#4]] |_Eval[[null[NULL] AS n#4]] | \_EsRelation[employees][<no-fields>{r$}#7] \_Aggregate[[n{r}#4],[COUNT(*[KEYWORD],true[BOOLEAN],PT0S[TIME_DURATION]) AS c#2, n{r}#4]] \_StubRelation[[<no-fields>{r$}#7, n{r}#4]] ``` The following join node: ``` InlineJoin[LEFT,[n{r}#4],[n{r}#4]] |_Eval[[null[NULL] AS n#4]] | \_EsRelation[employees][<no-fields>{r$}#7] \_Aggregate[[n{r}#4],[COUNT(*[KEYWORD],true[BOOLEAN],PT0S[TIME_DURATION]) AS c#2, n{r}#4]] \_StubRelation[[<no-fields>{r$}#7, n{r}#4]] ``` should NOT have `PruneLeftJoinOnNullMatchingField` applied, because the right side is an `Aggregate` (originating from `INLINE STATS`). Since `STATS` supports `GROUP BY null`, the join key being null is a valid use case. Pruning this join would incorrectly eliminate the aggregation results, changing the query semantics. During `LocalLogicalPlanOptimizer`: ``` ProjectExec[[c{r}#2, n{r}#4]] \_LimitExec[3[INTEGER],null] \_ExchangeExec[[c{r}#2, n{r}#4],false] \_FragmentExec[filter=null, estimatedRowSize=0, reducer=[], fragment=[<> Project[[c{r}#2, n{r}#4]] \_Limit[3[INTEGER],false,false] \_InlineJoin[LEFT,[n{r}#4],[n{r}#4]] |_Eval[[null[NULL] AS n#4]] | \_EsRelation[employees][<no-fields>{r$}#7] \_LocalRelation[[c{r}#2, n{r}#4],Page{blocks=[LongVectorBlock[vector=ConstantLongVector[positions=1, value=100]], ConstantNullBlock[positions=1]]}]<>]] ``` The following join node: ``` InlineJoin[LEFT,[n{r}#4],[n{r}#4]] |_Eval[[null[NULL] AS n#4]] | \_EsRelation[employees][<no-fields>{r$}#7] \_LocalRelation[[c{r}#2, n{r}#4],Page{blocks=[LongVectorBlock[vector=ConstantLongVector[positions=1, value=100]], ConstantNullBlock[positions=1]]}] ``` should NOT have `PruneLeftJoinOnNullMatchingField` applied, because the right side is a `LocalRelation` (the `Aggregate` was optimized into a `LocalRelation` containing the pre-computed aggregation results). Pruning this join when the join key is null would discard the valid aggregation results stored in the `LocalRelation`, incorrectly producing null values instead of the expected count. ## Solution The fix ensures that `PruneLeftJoinOnNullMatchingField` only applies to `LOOKUP JOIN` nodes, where `join.right()` is an `EsRelation`. For `INLINE STATS` joins, the right side can be: - `Aggregate` (before optimization), or - `LocalRelation` (after the aggregate is optimized) By checking `join.right() instanceof EsRelation`, we correctly skip the pruning optimization for `INLINE STATS` joins, preserving the expected query results when grouping by null.
1 parent ff7d81a commit f3ccb70

File tree

7 files changed

+487
-5
lines changed

7 files changed

+487
-5
lines changed

docs/changelog/140027.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 140027
2+
summary: "ESQL: Fix `INLINE STATS GROUP BY null` being incorrectly pruned"
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 139887

x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5190,3 +5190,122 @@ ROW x = 0
51905190
x:integer | a:long | b:long | c:boolean | d:boolean
51915191
0 | 0 | 0 | false | true
51925192
;
5193+
5194+
5195+
inlineStatsGroupByNull_Basic
5196+
required_capability: fix_inline_stats_group_by_null
5197+
5198+
ROW x = 1
5199+
| INLINE STATS c = COUNT(*) BY n = null
5200+
| KEEP c, n
5201+
| LIMIT 1
5202+
;
5203+
5204+
c:long | n:null
5205+
1 | null
5206+
;
5207+
5208+
5209+
inlineStatsGroupByNull_WithMultipleAggFunctions
5210+
required_capability: fix_inline_stats_group_by_null
5211+
5212+
ROW x = 10
5213+
| INLINE STATS c = COUNT(*), s = SUM(x), m = MAX(x) BY n = null
5214+
| KEEP c, s, m, n
5215+
| LIMIT 1
5216+
;
5217+
5218+
c:long | s:long | m:integer | n:null
5219+
1 | 10 | 10 | null
5220+
;
5221+
5222+
5223+
inlineStatsGroupByNull_WithEval
5224+
required_capability: fix_inline_stats_group_by_null
5225+
5226+
ROW x = 1, y = 2
5227+
| EVAL z = x + y
5228+
| INLINE STATS c = COUNT(*) BY n = null
5229+
| KEEP x, y, z, c, n
5230+
;
5231+
5232+
x:integer | y:integer | z:integer | c:long | n:null
5233+
1 | 2 | 3 | 1 | null
5234+
;
5235+
5236+
5237+
inlineStatsGroupByNull_WithNullLiteral
5238+
required_capability: fix_inline_stats_group_by_null
5239+
5240+
ROW x = 1
5241+
| INLINE STATS c = COUNT(*) BY null
5242+
| KEEP c, null
5243+
| LIMIT 1
5244+
;
5245+
5246+
c:long | null:null
5247+
1 | null
5248+
;
5249+
5250+
5251+
inlineStatsGroupByNull_FromEmployees
5252+
required_capability: fix_inline_stats_group_by_null
5253+
5254+
FROM employees
5255+
| INLINE STATS c = COUNT(*) BY n = null
5256+
| KEEP emp_no, c, n
5257+
| SORT emp_no
5258+
| LIMIT 3
5259+
;
5260+
5261+
emp_no:integer | c:long | n:null
5262+
10001 | 100 | null
5263+
10002 | 100 | null
5264+
10003 | 100 | null
5265+
;
5266+
5267+
5268+
inlineStatsGroupByNull_DoubleInlineStats
5269+
required_capability: fix_inline_stats_group_by_null
5270+
5271+
ROW x = 1
5272+
| INLINE STATS c1 = COUNT(*) BY n1 = null
5273+
| INLINE STATS c2 = COUNT(*) BY n2 = null
5274+
| KEEP c1, n1, c2, n2
5275+
| LIMIT 1
5276+
;
5277+
5278+
c1:long | n1:null | c2:long | n2:null
5279+
1 | null | 1 | null
5280+
;
5281+
5282+
5283+
inlineStatsGroupByNull_MultipleNullKeys
5284+
required_capability: fix_inline_stats_group_by_null
5285+
5286+
ROW x = 1
5287+
| INLINE STATS c = COUNT(*) BY n1 = null, n2 = null
5288+
| KEEP c, n1, n2
5289+
| LIMIT 1
5290+
;
5291+
5292+
c:long | n1:null | n2:null
5293+
1 | null | null
5294+
;
5295+
5296+
5297+
inlineStatsGroupByNull_MixedNullAndNonNull
5298+
required_capability: fix_inline_stats_group_by_null
5299+
5300+
FROM employees
5301+
| INLINE STATS c = COUNT(*) BY n = null, emp_no
5302+
| KEEP c, n, emp_no
5303+
| SORT emp_no
5304+
| LIMIT 3
5305+
;
5306+
5307+
c:long | n:null | emp_no:integer
5308+
1 | null | 10001
5309+
1 | null | 10002
5310+
1 | null | 10003
5311+
;

x-pack/plugin/esql/qa/testFixtures/src/main/resources/unmapped-nullify.csv-spec

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -430,16 +430,15 @@ null |1985 |null |abc |fork3
430430

431431
inlinestatsCount
432432
required_capability: optional_fields_nullify_tech_preview
433-
required_capability: inline_stats
433+
required_capability: fix_inline_stats_group_by_null
434434

435435
SET unmapped_fields="nullify"\;
436436
ROW x = 1
437437
| INLINE STATS c = COUNT(*), s = SUM(does_not_exist) BY d = does_not_exist
438438
;
439439

440-
# `c` should be just 0 : https://github.com/elastic/elasticsearch/issues/139887
441440
x:integer |does_not_exist:null|c:long |s:double |d:null
442-
1 |null |null |null |null
441+
1 |null |1 |null |null
443442
;
444443

445444
lookupJoin

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1886,7 +1886,7 @@ public enum Cap {
18861886
METADATA_TIER_FIELD(Build.current().isSnapshot()),
18871887
/**
18881888
* Fix folding of coalesce function
1889-
* https://github.com/elastic/elasticsearch/issues/139887
1889+
* https://github.com/elastic/elasticsearch/issues/139344
18901890
*/
18911891
FIX_FOLD_COALESCE,
18921892

@@ -1919,6 +1919,16 @@ public enum Cap {
19191919
*/
19201920
TS_STATS_BINARY_OPS,
19211921

1922+
/**
1923+
* Fix for INLINE STATS GROUP BY null being incorrectly pruned by PruneLeftJoinOnNullMatchingField.
1924+
* For INLINE STATS, the right side of the join can be Aggregate or LocalRelation (when optimized).
1925+
* The join key is always the grouping, and since STATS supports GROUP BY null, pruning the join when
1926+
* the join key (grouping) is null would incorrectly change the query results. This fix ensures
1927+
* PruneLeftJoinOnNullMatchingField only applies to LOOKUP JOIN (where right side is EsRelation).
1928+
* https://github.com/elastic/elasticsearch/issues/139887
1929+
*/
1930+
FIX_INLINE_STATS_GROUP_BY_NULL(INLINE_STATS.enabled),
1931+
19221932
/**
19231933
* Adds a conditional block loader for text fields that prefers using the sub-keyword field whenever possible.
19241934
*/

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/local/PruneLeftJoinOnNullMatchingField.java

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.elasticsearch.xpack.esql.plan.logical.Eval;
1717
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
1818
import org.elasticsearch.xpack.esql.plan.logical.Project;
19+
import org.elasticsearch.xpack.esql.plan.logical.join.InlineJoin;
1920
import org.elasticsearch.xpack.esql.plan.logical.join.Join;
2021

2122
import static org.elasticsearch.xpack.esql.core.expression.Expressions.isGuaranteedNull;
@@ -36,7 +37,16 @@ public PruneLeftJoinOnNullMatchingField() {
3637
@Override
3738
protected LogicalPlan rule(Join join, LogicalOptimizerContext ctx) {
3839
LogicalPlan plan = join;
39-
if (join.config().type() == LEFT) { // other types will have different replacement logic
40+
// Other join types will have different replacement logic.
41+
// This rule should only apply to LOOKUP JOIN, not INLINE STATS. For INLINE STATS, the join key is always
42+
// the grouping, and since STATS supports GROUP BY null, pruning the join when the join key (grouping) is
43+
// null would incorrectly change the query results.
44+
// Note: We use `instanceof InlineJoin == false` rather than `instanceof LookupJoin` because LookupJoin is
45+
// converted to a regular Join during analysis (via LookupJoin.surrogate()). By the time this optimizer rule
46+
// runs, the LookupJoin has already become a plain Join instance, so checking for LookupJoin would fail to
47+
// match the intended targets. The negative check correctly excludes InlineJoin while accepting all other
48+
// LEFT joins, including those originally created as LookupJoin.
49+
if (join.config().type() == LEFT && join instanceof InlineJoin == false) {
4050
AttributeMap<Expression> attributeMap = RuleUtils.foldableReferences(join, ctx);
4151

4252
for (var attr : AttributeSet.of(join.config().leftFields())) {

0 commit comments

Comments
 (0)