Simplify ProjectionPushdown and make it more general #8109
Conversation
|
|
||
| // If the projection does not narrow the the schema, we should not try | ||
| // to push it down | ||
| if projection.expr().len() >= projection.input().schema().fields().len() { |
There was a problem hiding this comment.
I pulled this out of the individual try_swapping_* calls
| { | ||
| return Ok(None); | ||
| } | ||
| let new_expr = expr |
There was a problem hiding this comment.
I rewrote this logic to use TreeNode recursion / rewriting rather than a manual recursion, which I think is both less code and is more general (handles all PhysicalExprs, not just the ones explicitly checked for here)
|
Some test is failing -- I will debug it later |
alamb
force-pushed
the
alamb/more_general
branch
from
ed89a21 to
b29b097
Compare
| /// Convenience utils for writing optimizers rule: recursively apply the given 'op' first to all of its | ||
| /// children and then itself(Postorder Traversal) using a mutable function, `F`. | ||
| /// When the `op` does not apply to a given node, it is left unchanged. | ||
| fn transform_up_mut<F>(self, op: &mut F) -> Result<Self> |
There was a problem hiding this comment.
mut variant is needed to support changing a variable in the closure (updating state)
There was a problem hiding this comment.
LGTM apart from one idiom suggestion. I'll let @berkaysynnada take a look and make sure all is right, then this will be good to go from our perspective. Thanks Andrew
|
Thanks @alamb, that function will possibly be used for future iterations as well. It's better now. |
Co-authored-by: Berkay Şahin <124376117+berkaysynnada@users.noreply.github.com> Co-authored-by: Mehmet Ozan Kabak <ozankabak@gmail.com>
Which issue does this PR close?
Follow on to #8073
Rationale for this change
I noticed some ways the code could be simplified and made more general while reviewing the PR,
What changes are included in this PR?
TreeNode::transform_downinstead of a manual tree walk to rewrite expressionsAre these changes tested?
Yes, by existing tests
Are there any user-facing changes?