Skip to content

Commit 1e9c355

Browse files
committed
Adapt PruneColumns to have "visibility" into the right-hand side of
the inlinestats JOIN
1 parent d3f2838 commit 1e9c355

File tree

2 files changed

+66
-11
lines changed

2 files changed

+66
-11
lines changed

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

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1052,11 +1052,57 @@ alpha db server |alpha |127.0.0.1 |hosts |127.0.0.1|1
10521052
;
10531053

10541054
doubleShadowing
1055+
required_capability: inlinestats_v8
10551056

10561057
FROM employees
10571058
| INLINESTATS salary = min(salary) BY gender
10581059
| INLINESTATS salary = max(salary) BY gender
10591060
| KEEP salary, gender
1060-
| SORT gender DESC
1061+
| SORT salary DESC, gender
10611062
| LIMIT 5
10621063
;
1064+
1065+
salary:integer |gender:keyword
1066+
25976 |F
1067+
25976 |F
1068+
25976 |F
1069+
25976 |F
1070+
25976 |F
1071+
;
1072+
1073+
doubleShadowingWithEval
1074+
required_capability: inlinestats_v8
1075+
1076+
from employees
1077+
| eval salary = salary/100
1078+
| inlinestats salary=min(salary) by gender
1079+
| inlinestats salary=max(salary) by gender
1080+
| keep salary, gender
1081+
| sort salary desc, gender
1082+
| limit 5
1083+
;
1084+
1085+
salary:integer|gender:keyword
1086+
259 |F
1087+
259 |F
1088+
259 |F
1089+
259 |F
1090+
259 |F
1091+
;
1092+
1093+
doubleShadowingWithDoubleStats
1094+
required_capability: inlinestats_v8
1095+
1096+
from employees
1097+
| stats salary=min(salary) by gender
1098+
| stats salary=max(salary) by gender
1099+
| inlinestats salary=min(salary)
1100+
| inlinestats salary=max(salary)
1101+
;
1102+
ignoreOrder:true
1103+
1104+
gender:keyword |salary:integer
1105+
null |25324
1106+
F |25324
1107+
M |25324
1108+
;

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

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import org.elasticsearch.xpack.esql.plan.logical.Fork;
2323
import org.elasticsearch.xpack.esql.plan.logical.Limit;
2424
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
25+
import org.elasticsearch.xpack.esql.plan.logical.join.InlineJoin;
2526
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
2627
import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier;
2728
import org.elasticsearch.xpack.esql.planner.PlannerUtils;
@@ -39,15 +40,18 @@ public final class PruneColumns extends Rule<LogicalPlan, LogicalPlan> {
3940
public LogicalPlan apply(LogicalPlan plan) {
4041
// track used references
4142
var used = plan.outputSet().asBuilder();
43+
// 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+
var inlineJoinRightOutput = new ArrayList<Attribute>();
4246
Holder<Boolean> forkPresent = new Holder<>(false);
4347

4448
// while going top-to-bottom (upstream)
4549
var pl = plan.transformDown(p -> {
46-
// Note: It is NOT required to do anything special for binary plans like JOINs. It is perfectly fine that transformDown descends
47-
// first into the left side, adding all kinds of attributes to the `used` set, and then descends into the right side - even
48-
// though the `used` set will contain stuff only used in the left hand side. That's because any attribute that is used in the
49-
// left hand side must have been created in the left side as well. Even field attributes belonging to the same index fields will
50-
// have different name ids in the left and right hand sides - as in the extreme example
50+
// Note: It is NOT required to do anything special for binary plans like JOINs, except INLINESTATS. It is perfectly fine that
51+
// transformDown descends first into the left side, adding all kinds of attributes to the `used` set, and then descends into
52+
// the right side - even though the `used` set will contain stuff only used in the left hand side. That's because any attribute
53+
// that is used in the left hand side must have been created in the left side as well. Even field attributes belonging to the
54+
// same index fields will have different name ids in the left and right hand sides - as in the extreme example
5155
// `FROM lookup_idx | LOOKUP JOIN lookup_idx ON key_field`.
5256

5357
// skip nodes that simply pass the input through
@@ -63,14 +67,18 @@ public LogicalPlan apply(LogicalPlan plan) {
6367
return p;
6468
}
6569

70+
if (p instanceof InlineJoin ij) {
71+
inlineJoinRightOutput.addAll(ij.right().outputSet());
72+
}
73+
6674
// remember used
6775
boolean recheck;
6876
// analyze the unused items against dedicated 'producer' nodes such as Eval and Aggregate
6977
// perform a loop to retry checking if the current node is completely eliminated
7078
do {
7179
recheck = false;
7280
if (p instanceof Aggregate aggregate) {
73-
var remaining = removeUnused(aggregate.aggregates(), used);
81+
var remaining = removeUnused(aggregate.aggregates(), used, inlineJoinRightOutput);
7482

7583
if (remaining != null) {
7684
if (remaining.isEmpty()) {
@@ -97,7 +105,7 @@ public LogicalPlan apply(LogicalPlan plan) {
97105
}
98106
}
99107
} else if (p instanceof Eval eval) {
100-
var remaining = removeUnused(eval.fields(), used);
108+
var remaining = removeUnused(eval.fields(), used, inlineJoinRightOutput);
101109
// no fields, no eval
102110
if (remaining != null) {
103111
if (remaining.isEmpty()) {
@@ -111,7 +119,7 @@ public LogicalPlan apply(LogicalPlan plan) {
111119
// Normally, pruning EsRelation has no effect because InsertFieldExtraction only extracts the required fields, anyway.
112120
// However, InsertFieldExtraction can't be currently used in LOOKUP JOIN right index,
113121
// it works differently as we extract all fields (other than the join key) that the EsRelation has.
114-
var remaining = removeUnused(esr.output(), used);
122+
var remaining = removeUnused(esr.output(), used, inlineJoinRightOutput);
115123
if (remaining != null) {
116124
p = new EsRelation(esr.source(), esr.indexPattern(), esr.indexMode(), esr.indexNameWithModes(), remaining);
117125
}
@@ -131,14 +139,15 @@ public LogicalPlan apply(LogicalPlan plan) {
131139
* Prunes attributes from the list not found in the given set.
132140
* Returns null if no changed occurred.
133141
*/
134-
private static <N extends NamedExpression> List<N> removeUnused(List<N> named, AttributeSet.Builder used) {
142+
private static <N extends NamedExpression> List<N> removeUnused(List<N> named, AttributeSet.Builder used, List<Attribute> exceptions) {
135143
var clone = new ArrayList<>(named);
136144
var it = clone.listIterator(clone.size());
137145

138146
// due to Eval, go in reverse
139147
while (it.hasPrevious()) {
140148
N prev = it.previous();
141-
if (used.contains(prev.toAttribute()) == false) {
149+
var attr = prev.toAttribute();
150+
if (used.contains(attr) == false && exceptions.contains(attr) == false) {
142151
it.remove();
143152
} else {
144153
used.addAll(prev.references());

0 commit comments

Comments
 (0)