-
Notifications
You must be signed in to change notification settings - Fork 25.2k
ESQL: Add optimization to purge join on null merge key #127583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
1605c8f
d0ed566
43922e8
8a484b8
c82823f
33c0575
95037e9
d7dc024
a81cefd
099b965
936874f
d52e609
11d5cc4
1b0eaf1
1fdd95f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
pr: 127583 | ||
summary: Add optimization to purge join on null merge key | ||
area: ES|QL | ||
type: enhancement | ||
issues: | ||
- 125577 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
package org.elasticsearch.xpack.esql.optimizer.rules.logical.local; | ||
|
||
import org.elasticsearch.xpack.esql.core.expression.Alias; | ||
import org.elasticsearch.xpack.esql.core.expression.AttributeMap; | ||
import org.elasticsearch.xpack.esql.core.expression.AttributeSet; | ||
import org.elasticsearch.xpack.esql.core.expression.Expression; | ||
import org.elasticsearch.xpack.esql.core.expression.Expressions; | ||
import org.elasticsearch.xpack.esql.core.expression.Literal; | ||
import org.elasticsearch.xpack.esql.optimizer.rules.logical.OptimizerRules; | ||
import org.elasticsearch.xpack.esql.plan.logical.Eval; | ||
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; | ||
import org.elasticsearch.xpack.esql.plan.logical.Project; | ||
import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; | ||
import org.elasticsearch.xpack.esql.plan.logical.join.Join; | ||
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
import static org.elasticsearch.xpack.esql.core.expression.Expressions.isGuaranteedNull; | ||
import static org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes.INNER; | ||
import static org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes.LEFT; | ||
|
||
/** | ||
* The rule matches a plan pattern having a Join on top of a Project and/or Eval. It then checks if the join's performed on a field which | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking for this pattern is restrictive IMO, but it's also not what the rule actually does. Leftover? Also, this is repeating some of the logic of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is more restrictive than I'd wish for it to be more effective, indeed, but I found no other way to detect
No, I think that's actually what the rule does; but maybe I misunderstood the question? :)
The refs collection in this rule is more restrictive than that in
Right; that's because it can only apply this rule on a specific tree pattern; if there were more nodes in-between (of different types), the rule wouldn't work.
The tree traversing would be different, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, you are correct; I didn't notice the break in the switch below and thought we'd just go over all children but looking only for evals and projections in it.
My point is that both rules look for chains of aliases and propagate the result into a command that (transitively) depneds on a literal. I think we should have only 1 way to do this; if the approach is correct for PropagateEvalFoldables, it should also be correct here - and if it's not, then PropagateEvalFoldables probably has a bug and we need to find a different solution. Conceptually, the difference is that PropagateEvalFoldables just places a literal in the downstream command - while we rather wish to prune it - but that could be solved either by to place a literal into the join config (which I think is interesting anyway because that allows even more optimizations).
bpintea marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* is aliased to null (in type or value); if that's the case, it prunes the join, replacing it with an Eval - returning aliases to null | ||
* for all the fields added in by the right side of the Join - plus a Project on top of it. | ||
* The rule can apply on the coordinator already, but it's more likely to be effective on the data nodes, where null aliasing is inserted | ||
* due to locally missing fields. This rule relies on that behavior -- see {@link ReplaceMissingFieldWithNull}. | ||
*/ | ||
public class PruneJoinOnNullMatchingField extends OptimizerRules.OptimizerRule<Join> { | ||
|
||
@Override | ||
protected LogicalPlan rule(Join join) { | ||
var joinType = join.config().type(); | ||
if ((joinType == INNER || joinType == LEFT) == false) { // other types will have different replacement logic | ||
return join; | ||
} | ||
AttributeMap.Builder<Expression> attributeMapBuilder = AttributeMap.builder(); | ||
loop: for (var child = join.left();; child = ((UnaryPlan) child).child()) { // cast is safe as both Project and Eval are UnaryPlans | ||
switch (child) { | ||
case Project project -> project.projections().forEach(projection -> { | ||
if (projection instanceof Alias alias) { | ||
attributeMapBuilder.put(alias.toAttribute(), alias.child()); | ||
} | ||
}); | ||
case Eval eval -> eval.fields().forEach(alias -> attributeMapBuilder.put(alias.toAttribute(), alias.child())); | ||
default -> { | ||
break loop; | ||
} | ||
} | ||
} | ||
for (var attr : AttributeSet.of(join.config().matchFields())) { | ||
var resolved = attributeMapBuilder.build().resolve(attr); | ||
if (resolved != null && isGuaranteedNull(resolved)) { | ||
return replaceJoin(join); | ||
} | ||
} | ||
return join; | ||
} | ||
|
||
private static LogicalPlan replaceJoin(Join join) { | ||
var joinRightOutput = join.rightOutputFields(); | ||
if (joinRightOutput.isEmpty()) { // can be empty when the join key is null and the other right side entries pruned (by an agg) | ||
bpintea marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return join.left(); | ||
} | ||
List<Alias> aliases = new ArrayList<>(joinRightOutput.size()); | ||
// TODO: cache aliases by type, à la ReplaceMissingFieldWithNull#missingToNull (tho lookup indices won't have Ks of fields) | ||
bpintea marked this conversation as resolved.
Show resolved
Hide resolved
|
||
joinRightOutput.forEach(a -> aliases.add(new Alias(a.source(), a.name(), Literal.of(a, null), a.id()))); | ||
var eval = new Eval(join.source(), join.left(), aliases); | ||
return new Project(join.source(), eval, join.computeOutput(join.left().output(), Expressions.asAttributes(aliases))); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly related, but not sure why we wouldn't include the id, it's easier to track which exactly reference points to an alias.