Skip to content

Commit 17d5933

Browse files
astefanastefan
committed
Add more tests and fix incorrect pruning of columns of the inlinestats'
Aggregation. Move two methods from EsqlSession to InlineJoin. Address reviews Add more comments
1 parent 4ed8c9b commit 17d5933

File tree

6 files changed

+243
-62
lines changed

6 files changed

+243
-62
lines changed

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

Lines changed: 123 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,6 @@ emp_no:integer | avg_worked_seconds:long | max_avg_worked_seconds:long | languag
201201
;
202202

203203
two
204-
required_capability: join_planning_v1
205204
required_capability: inlinestats_v8
206205

207206
FROM employees
@@ -210,17 +209,24 @@ FROM employees
210209
| WHERE avg_worked_seconds > avg_avg_worked_seconds
211210
| INLINESTATS max_languages = MAX(languages) BY gender
212211
| SORT emp_no ASC
213-
| LIMIT 3;
212+
| LIMIT 10;
214213

215214
emp_no:integer |avg_worked_seconds:long|avg_avg_worked_seconds:double|languages:integer|max_languages:integer|gender:keyword
216215
10002 |328922887 |3.133013149047619E8 |5 |5 |F
217216
10006 |372957040 |2.978159518235294E8 |3 |5 |F
218217
10007 |393084805 |2.863684210555556E8 |4 |5 |F
218+
10010 |315236372 |2.863684210555556E8 |4 |5 |null
219+
10012 |365510850 |3.133013149047619E8 |5 |5 |null
220+
10015 |390266432 |3.133013149047619E8 |5 |5 |null
221+
10018 |309604079 |3.0318626831578946E8 |2 |5 |null
222+
10019 |342855721 |2.94833632E8 |1 |5 |null
223+
10020 |373309605 |3.181719481E8 |null |5 |M
224+
10023 |330870342 |3.181719481E8 |null |5 |F
219225
;
220226

221227
three
222228
required_capability: inlinestats_v8
223-
// fails with AssertionError at org.elasticsearch.xpack.esql.plan.logical.Limit.writeTo(Limit.java:70)
229+
// used to fail with AssertionError at org.elasticsearch.xpack.esql.plan.logical.Limit.writeTo(Limit.java:70)
224230

225231
FROM employees
226232
| KEEP emp_no, languages, avg_worked_seconds, gender
@@ -245,6 +251,32 @@ emp_no:integer |avg_worked_seconds:long|avg_avg_worked_seconds:double|languages:
245251
10023 |330870342 |3.181719481E8 |null |5 |3 |5 |F
246252
;
247253

254+
// TODO: INLINESTATS unit test needed for this one
255+
pushDownSort_To_LeftSideOnly
256+
required_capability: inlinestats_v8
257+
258+
from employees
259+
| sort emp_no desc
260+
| inlinestats avg = avg(salary) by languages
261+
| limit 5
262+
| keep emp_no, avg, languages, gender
263+
;
264+
265+
emp_no:integer| avg:double |languages:integer|gender:keyword
266+
10001 |57305.0 |2 |M
267+
10002 |46272.5 |5 |F
268+
10003 |61805.0 |4 |M
269+
10004 |46272.5 |5 |M
270+
10005 |63528.0 |1 |M
271+
;
272+
273+
from employees
274+
| sort emp_no desc
275+
| inlinestats avg = avg(salary) by languages
276+
| limit 5
277+
| keep emp_no, avg, languages, gender
278+
;
279+
248280
byMultivaluedSimple
249281
required_capability: inlinestats_v8
250282

@@ -313,7 +345,6 @@ FROM airports
313345
;
314346

315347
mvMinMvExpand
316-
required_capability: join_planning_v1
317348
required_capability: inlinestats_v8
318349

319350
FROM airports
@@ -378,7 +409,6 @@ abbrev:keyword | country:keyword | count:long
378409
;
379410

380411
afterLookup
381-
required_capability: join_planning_v1
382412
required_capability: inlinestats_v8
383413
required_capability: join_lookup_v12
384414

@@ -403,7 +433,6 @@ ZNZ |4 |German
403433
;
404434

405435
afterEnrich
406-
required_capability: join_planning_v1
407436
required_capability: inlinestats_v8
408437
required_capability: enrich_load
409438

@@ -466,7 +495,6 @@ emp_no:integer | languages:integer | max_salary:integer
466495
;
467496

468497
beforeEnrich
469-
required_capability: join_planning_v1
470498
required_capability: inlinestats_v8
471499
required_capability: enrich_load
472500

@@ -486,7 +514,6 @@ ACA |Acapulco de Juárez|385 |major |Acapulco de
486514
;
487515

488516
beforeAndAfterEnrich
489-
required_capability: join_planning_v1
490517
required_capability: inlinestats_v8
491518
required_capability: enrich_load
492519

@@ -510,7 +537,6 @@ ALL |Albenga |499 |mid |1
510537
;
511538

512539
shadowing
513-
required_capability: join_planning_v1
514540
required_capability: inlinestats_v8
515541

516542
ROW left = "left", client_ip = "172.21.0.5", env = "env", right = "right"
@@ -522,7 +548,6 @@ left | right | right | 172.21.0.5
522548
;
523549

524550
shadowingMulti
525-
required_capability: join_planning_v1
526551
required_capability: inlinestats_v8
527552

528553
ROW left = "left", airport = "Zurich Airport ZRH", city = "Zürich", middle = "middle", region = "North-East Switzerland", right = "right"
@@ -534,7 +559,6 @@ left | middle | right | left | left
534559
;
535560

536561
shadowingSelf
537-
required_capability: join_planning_v1
538562
required_capability: inlinestats_v8
539563

540564
ROW city = "Raleigh"
@@ -546,7 +570,6 @@ city:long
546570
;
547571

548572
shadowingSelfBySelf
549-
required_capability: join_planning_v1
550573
required_capability: inlinestats_v8
551574

552575
ROW city = "Raleigh"
@@ -559,7 +582,6 @@ Raleigh
559582
;
560583

561584
shadowingInternal
562-
required_capability: join_planning_v1
563585
required_capability: inlinestats_v8
564586

565587
ROW city = "Zürich"
@@ -585,9 +607,8 @@ row x = 1
585607
2 |3 |3 |1 |1
586608
;
587609

588-
ignoreUnusedEvaledValue-Ignore
610+
ignoreUnusedEvaledValue_AndInlineStats
589611
required_capability: inlinestats_v8
590-
// fails with expected [keys] to be non-empty
591612

592613
ROW x = 1
593614
| INLINESTATS max(x)
@@ -599,9 +620,21 @@ x:integer
599620
1
600621
;
601622

602-
ignoreUnusedEvaledValue2-Ignore
623+
ignoreUnusedEvaledValue_AndInlineStats2
624+
required_capability: inlinestats_v8
625+
626+
ROW x = 1, z = 2
627+
| INLINESTATS max(x)
628+
| EVAL a = x + 1, b = z + 2
629+
| KEEP x, z
630+
;
631+
632+
x:integer | z:integer
633+
1 | 2
634+
;
635+
636+
ignoreUnusedEvaledValue_AndInlineStats3
603637
required_capability: inlinestats_v8
604-
// fails with expected [keys] to be non-empty
605638

606639
from employees
607640
| inlinestats max(salary)
@@ -615,6 +648,36 @@ from employees
615648
74999
616649
;
617650

651+
ignoreUnusedEvaledValue_AndInlineStats4
652+
required_capability: inlinestats_v8
653+
654+
from employees
655+
| inlinestats max(salary), m = min(salary) by gender
656+
| eval y = concat(gender, "")
657+
| keep emp_no
658+
| sort emp_no desc
659+
| limit 1
660+
;
661+
662+
emp_no:integer
663+
10100
664+
;
665+
666+
ignoreUnusedEvaledValue_AndInlineStats5
667+
required_capability: inlinestats_v8
668+
669+
from employees
670+
| inlinestats max(salary), m = min(salary) by gender
671+
| eval y = m / 2
672+
| keep emp_no
673+
| sort emp_no desc
674+
| limit 1
675+
;
676+
677+
emp_no:integer
678+
10100
679+
;
680+
618681
byConstant
619682
required_capability: inlinestats_v8
620683

@@ -691,7 +754,6 @@ abbrev:keyword | scalerank:integer | location:geo_point
691754
;
692755

693756
byTwoCalculatedSecondOverwrites
694-
required_capability: join_planning_v1
695757
required_capability: stats_alias_collision_warnings
696758
required_capability: inlinestats_v8
697759

@@ -713,7 +775,6 @@ abbrev:keyword | scalerank:integer | location:geo_point
713775
;
714776

715777
byTwoCalculatedSecondOverwritesReferencingFirst
716-
required_capability: join_planning_v1
717778
required_capability: stats_alias_collision_warnings
718779
required_capability: inlinestats_v8
719780

@@ -737,7 +798,6 @@ abbrev:keyword | scalerank:integer | location:geo_point
737798

738799

739800
groupShadowsAgg
740-
required_capability: join_planning_v1
741801
required_capability: stats_alias_collision_warnings
742802
required_capability: inlinestats_v8
743803

@@ -1128,6 +1188,49 @@ salary:integer |gender:keyword
11281188
25976 |F
11291189
;
11301190

1191+
doubleShadowing_WithIntertwinedFilters
1192+
required_capability: inlinestats_v8
1193+
1194+
FROM employees
1195+
| WHERE salary > 30000
1196+
| INLINESTATS salary = min(salary) BY gender
1197+
| WHERE salary > 31000
1198+
| INLINESTATS salary = max(salary) BY gender
1199+
| WHERE salary > 31500
1200+
| KEEP salary, gender
1201+
| SORT salary DESC, gender
1202+
;
1203+
warning:No limit defined, adding default limit of [1000]
1204+
1205+
salary:integer |gender:keyword
1206+
31120 |null
1207+
31120 |null
1208+
31120 |null
1209+
31120 |null
1210+
31120 |null
1211+
31120 |null
1212+
31120 |null
1213+
31120 |null
1214+
31120 |null
1215+
;
1216+
1217+
shadowingAggregateByNextGrouping
1218+
required_capability: inlinestats_v8
1219+
1220+
FROM employees
1221+
| KEEP gender, languages, emp_no, salary
1222+
| INLINESTATS gender = count_distinct(gender) BY languages
1223+
| INLINESTATS avg(salary) BY gender
1224+
| SORT emp_no
1225+
| LIMIT 3
1226+
;
1227+
1228+
emp_no:integer |salary:integer |languages:integer|avg(salary):double|gender:keyword
1229+
10001 |57305 |2 |48248.55 |2
1230+
10002 |56371 |5 |48248.55 |2
1231+
10003 |61805 |4 |48248.55 |2
1232+
;
1233+
11311234
doubleShadowingWithEval
11321235
required_capability: inlinestats_v8
11331236

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.elasticsearch.xpack.esql.rule.Rule;
3030

3131
import java.util.ArrayList;
32+
import java.util.Collections;
3233
import java.util.List;
3334

3435
/**
@@ -41,7 +42,7 @@ public LogicalPlan apply(LogicalPlan plan) {
4142
// track used references
4243
var used = plan.outputSet().asBuilder();
4344
// track inlinestats' own aggregation output (right-hand side of the join) so that any other plan on the left-hand side of the
44-
// inline join won't have it's columns pruned due to the lack of "visibility" into the right hand side output/Attributes
45+
// inline join won't have its columns pruned due to the lack of "visibility" into the right hand side output/Attributes
4546
var inlineJoinRightOutput = new ArrayList<Attribute>();
4647
Holder<Boolean> forkPresent = new Holder<>(false);
4748

@@ -104,6 +105,16 @@ public LogicalPlan apply(LogicalPlan plan) {
104105
p = aggregate.with(aggregate.groupings(), remaining);
105106
}
106107
}
108+
} else if (p instanceof InlineJoin ij) {// TODO: InlineStats - add tests for this IJ removal
109+
var remaining = removeUnused(ij.right().output(), used, Collections.emptyList());
110+
if (remaining != null) {
111+
if (remaining.isEmpty()) {
112+
// remove the InlineJoin altogether
113+
p = ij.left();
114+
recheck = true;
115+
}
116+
// TODO: InlineStats - prune only the unused columns from it?
117+
}
107118
} else if (p instanceof Eval eval) {
108119
var remaining = removeUnused(eval.fields(), used, inlineJoinRightOutput);
109120
// no fields, no eval

0 commit comments

Comments
 (0)