Skip to content

Commit 6dd6c4f

Browse files
committed
Don't duplicate Limit on left InlineJoin
...as this reduces the data the (inline) aggs run on, which is wrong.
1 parent a4e3b0a commit 6dd6c4f

File tree

3 files changed

+106
-2
lines changed

3 files changed

+106
-2
lines changed

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,33 @@ emp_no:integer | languages:integer | max_lang:integer | gender:keyword
5151
10014 | 5 | 5 | null
5252
;
5353

54+
maxOfIntByKeywordLimit1Aggd
55+
required_capability: inlinestats_v10
56+
57+
FROM employees
58+
| KEEP emp_no, languages, gender
59+
| INLINESTATS max_lang = MAX(languages) BY gender
60+
| LIMIT 1
61+
| STATS v = VALUES(max_lang)
62+
;
63+
64+
v:integer
65+
5
66+
;
67+
68+
simpleAvg
69+
required_capability: inlinestats_v10
70+
71+
FROM employees
72+
| INLINESTATS a = AVG(salary)
73+
| LIMIT 2
74+
| KEEP a;
75+
76+
a:double
77+
48248.55
78+
48248.55
79+
;
80+
5481
maxOfLongByKeyword
5582
required_capability: inlinestats_v10
5683

@@ -247,7 +274,8 @@ emp_no:integer |avg_worked_seconds:long|avg_avg_worked_seconds:double|languages:
247274
;
248275

249276
// TODO: INLINESTATS unit test needed for this one
250-
pushDownSort_To_LeftSideOnly
277+
// https://github.com/elastic/elasticsearch/issues/113727
278+
pushDownSort_To_LeftSideOnly-Ignore
251279
required_capability: inlinestats_v10
252280

253281
from employees

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.elasticsearch.xpack.esql.plan.logical.RegexExtract;
1919
import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan;
2020
import org.elasticsearch.xpack.esql.plan.logical.inference.InferencePlan;
21+
import org.elasticsearch.xpack.esql.plan.logical.join.InlineJoin;
2122
import org.elasticsearch.xpack.esql.plan.logical.join.Join;
2223
import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes;
2324

@@ -64,7 +65,7 @@ public LogicalPlan rule(Limit limit, LogicalOptimizerContext ctx) {
6465
}
6566
}
6667
}
67-
} else if (limit.child() instanceof Join join && join.config().type() == JoinTypes.LEFT) {
68+
} else if (limit.child() instanceof Join join && join.config().type() == JoinTypes.LEFT && join instanceof InlineJoin == false) {
6869
// Left joins increase the number of rows if any join key has multiple matches from the right hand side.
6970
// Therefore, we cannot simply push down the limit - but we can add another limit before the join.
7071
// To avoid repeating this infinitely, we have to set duplicated = true.

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6005,6 +6005,81 @@ public void testInlinestatsWithRow() {
60056005
);
60066006
}
60076007

6008+
/*
6009+
* EsqlProject[[a{r}#4]]
6010+
* \_Limit[2[INTEGER],false]
6011+
* \_InlineJoin[LEFT,[],[],[]]
6012+
* |_EsRelation[test][_meta_field{f}#12, emp_no{f}#6, first_name{f}#7, ge..]
6013+
* \_Project[[a{r}#4]]
6014+
* \_Eval[[$$SUM$a$0{r$}#17 / $$COUNT$a$1{r$}#18 AS a#4]]
6015+
* \_Aggregate[[],[SUM(salary{f}#11,true[BOOLEAN],compensated[KEYWORD]) AS $$SUM$a$0#17,
6016+
* COUNT(salary{f}#11,true[BOOLEAN]) AS $$COUNT$a$1#18]]
6017+
* \_StubRelation[[_meta_field{f}#12, emp_no{f}#6, first_name{f}#7, gender{f}#8, hire_date{f}#13, job{f}#14, job.raw{f}#15,
6018+
* languages{f}#9, last_name{f}#10, long_noidx{f}#16, salary{f}#11]]
6019+
*/
6020+
public void testInlinestatsWithLimit() {
6021+
var query = """
6022+
FROM employees
6023+
| INLINESTATS a = AVG(salary)
6024+
| LIMIT 2
6025+
| KEEP a
6026+
""";
6027+
if (releaseBuildForInlinestats(query)) {
6028+
return;
6029+
}
6030+
var plan = optimizedPlan(query);
6031+
6032+
var project = as(plan, EsqlProject.class);
6033+
assertThat(Expressions.names(project.projections()), is(List.of("a")));
6034+
var limit = asLimit(project.child(), 2, false);
6035+
var inlineJoin = as(limit.child(), InlineJoin.class);
6036+
// Left
6037+
var relation = as(inlineJoin.left(), EsRelation.class);
6038+
// Right
6039+
var rightProject = as(inlineJoin.right(), Project.class);
6040+
assertThat(Expressions.names(rightProject.projections()), contains("a"));
6041+
var eval = as(rightProject.child(), Eval.class);
6042+
assertThat(Expressions.names(eval.fields()), is(List.of("a")));
6043+
var agg = as(eval.child(), Aggregate.class);
6044+
var stub = as(agg.child(), StubRelation.class);
6045+
}
6046+
6047+
/*
6048+
* Limit[10000[INTEGER],false]
6049+
* \_Aggregate[[],[VALUES(max_lang{r}#7,true[BOOLEAN]) AS v#11]]
6050+
* \_Limit[1[INTEGER],false]
6051+
* \_InlineJoin[LEFT,[gender{f}#14],[gender{f}#14],[gender{r}#14]]
6052+
* |_EsqlProject[[emp_no{f}#12, languages{f}#15, gender{f}#14]]
6053+
* | \_EsRelation[test][_meta_field{f}#18, emp_no{f}#12, first_name{f}#13, ..]
6054+
* \_Aggregate[[gender{f}#14],[MAX(languages{f}#15,true[BOOLEAN]) AS max_lang#7, gender{f}#14]]
6055+
* \_StubRelation[[emp_no{f}#12, languages{f}#15, gender{f}#14]]
6056+
*/
6057+
public void testInlinestatsWithLimitAndAgg() {
6058+
var query = """
6059+
FROM employees
6060+
| KEEP emp_no, languages, gender
6061+
| INLINESTATS max_lang = MAX(languages) BY gender
6062+
| LIMIT 1
6063+
| STATS v = VALUES(max_lang)
6064+
""";
6065+
if (releaseBuildForInlinestats(query)) {
6066+
return;
6067+
}
6068+
var plan = optimizedPlan(query);
6069+
6070+
var limit = asLimit(plan, 10000, false);
6071+
var aggregate = as(limit.child(), Aggregate.class);
6072+
assertThat(Expressions.names(aggregate.aggregates()), is(List.of("v")));
6073+
var innerLimit = asLimit(aggregate.child(), 1, false);
6074+
var inlineJoin = as(innerLimit.child(), InlineJoin.class);
6075+
// Left
6076+
var project = as(inlineJoin.left(), EsqlProject.class);
6077+
var relation = as(project.child(), EsRelation.class);
6078+
// Right
6079+
var agg = as(inlineJoin.right(), Aggregate.class);
6080+
var stub = as(agg.child(), StubRelation.class);
6081+
}
6082+
60086083
/**
60096084
* Expects
60106085
*

0 commit comments

Comments
 (0)