[SPARK-22944][SQL] improve FoldablePropagation #20139
Conversation
|
Test build #85624 has finished for PR 20139 at commit
|
|
retest this please |
| * Other optimizations will take advantage of the propagated foldable expressions. | ||
| * | ||
| * Other optimizations will take advantage of the propagated foldable expressions. For example, | ||
| * This rule can optimize |
| j.transformExpressions(replaceFoldable) | ||
| case j @ Join(left, right, joinType, _) => | ||
| val newJoin = j.transformExpressions(replaceFoldable) | ||
| val missDerivedAttrsSet: AttributeSet = AttributeSet(joinType match { |
There was a problem hiding this comment.
The table expressions on either side of a left or right outer join are referred to as preserved and null-supplying. In a left outer join, the left-hand table expression is preserved and the right-hand table expression is null-supplying.
How about renaming missDerivedAttrsSet to nullSupplyingAttrsSet
There was a problem hiding this comment.
I'd like to keep the word miss as it's really a mistake. We should fix it eventually.
| // We can only propagate foldables for a subset of unary nodes. | ||
| case u: UnaryNode if !stop && canPropagateFoldables(u) => | ||
| case u: UnaryNode if canPropagateFoldables(u) => |
There was a problem hiding this comment.
For all these cases, how about adding another condition if foldableMap.nonEmpty && xyz?
| // and often reuses the underlying attributes; so we cannot assume that a column is still | ||
| // foldable after the expand has been applied. | ||
| // Similar to Join, Expand also miss-derives output attributes from child attributes, we | ||
| // should exclude them when propagating. |
There was a problem hiding this comment.
After #12496, the above statement is still true?
| case other => | ||
| stop = true | ||
| val childrenOutputSet = AttributeSet(other.children.flatMap(_.output)) | ||
| foldableMap = AttributeMap(foldableMap.baseMap.values.filterNot { |
There was a problem hiding this comment.
Can we directly set foldableMap to an empty map?
There was a problem hiding this comment.
we can't as it may not be empty. We may have new attributes produced by operators above those unsupported operators.
|
Test build #85627 has finished for PR 20139 at commit
|
|
Oops. I deleted my previous comment. Never mind. |
|
Thank you for pinging me. LGTM. |
|
Test build #85635 has finished for PR 20139 at commit
|
| val missDerivedAttrsSet = expand.child.outputSet | ||
| foldableMap = AttributeMap(foldableMap.baseMap.values.filterNot { | ||
| case (attr, _) => missDerivedAttrsSet.contains(attr) | ||
| }.toSeq) |
|
|
||
| test("Propagate above outer join") { | ||
| val left = LocalRelation('a.int).select('a, Literal(1).as('b)) | ||
| val right = LocalRelation('c.int).select('c, Literal(1).as("d")) |
|
LGTM |
|
Test build #85648 has finished for PR 20139 at commit
|
|
Thanks! Merged to master/2.3 |
## What changes were proposed in this pull request? `FoldablePropagation` is a little tricky as it needs to handle attributes that are miss-derived from children, e.g. outer join outputs. This rule does a kind of stop-able tree transform, to skip to apply this rule when hit a node which may have miss-derived attributes. Logically we should be able to apply this rule above the unsupported nodes, by just treating the unsupported nodes as leaf nodes. This PR improves this rule to not stop the tree transformation, but reduce the foldable expressions that we want to propagate. ## How was this patch tested? existing tests Author: Wenchen Fan <[email protected]> Closes #20139 from cloud-fan/foldable. (cherry picked from commit 7d045c5) Signed-off-by: gatorsmile <[email protected]>
What changes were proposed in this pull request?
FoldablePropagationis a little tricky as it needs to handle attributes that are miss-derived from children, e.g. outer join outputs. This rule does a kind of stop-able tree transform, to skip to apply this rule when hit a node which may have miss-derived attributes.Logically we should be able to apply this rule above the unsupported nodes, by just treating the unsupported nodes as leaf nodes. This PR improves this rule to not stop the tree transformation, but reduce the foldable expressions that we want to propagate.
How was this patch tested?
existing tests