-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix LogicalPlan::..._with_subqueries
methods
#13589
base: main
Are you sure you want to change the base?
Conversation
cc @alamb , @berkaysynnada |
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.
Thank you @peter-toth
I verified test coverage by running the test without the code changes and it failed
assertion failed: !filter_found
thread 'logical_plan::plan::tests::test_with_subqueries_jump' panicked at datafusion/expr/src/logical_plan/plan.rs:4179:9:
assertion failed: !filter_found
stack backtrace:
0: rust_begin_unwind
at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:662:5
1: core::panicking::panic_fmt
at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:74:14
2: core::panicking::panic
at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:148:5
3: datafusion_expr::logical_plan::plan::tests::test_with_subqueries_jump
}) | ||
f(node)?.visit_children(|| { | ||
node.apply_subqueries(|c| apply_with_subqueries_impl(c, f))? | ||
.visit_sibling(|| { |
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.
i don't fully understand this -- the .visit_sibling
call looks like it is in th visit_children
closure now rather than being chained. Is that intended?
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.
Yeah, it might look a bit weird at first, but actually the subquery expression plans in a LogicalPlan
node and the children of that node are kind of siblings (in terms of recursion) and all of them are kind of children of the node.
So the .visit_sibling()
continuation from .apply_subqueries()
(that visits the subquery plans of a node) to .apply_children()
(that visits the children of a node) makes sense and it needs to happen in a .visit_children()
continuation.
|
||
#[test] | ||
fn test_with_subqueries_jump() { | ||
// The plan contains a `Project` node above a `Filter` node so returning |
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.
💯 comments
Thanks so much @alamb for verifying this! Currently we don't return jump anywhere in |
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.
Thank you @peter-toth. I really like the tests!
I have one minor suggestion: if it doesn’t require much effort, could we also add a test to ensure that jump does not continue to visit subqueries also? Even though this behavior was not wrong and hasn’t changed with this PR, it would be good to have a test guarding it.
Thanks, good idea! Added in 542c104. |
739c356
to
542c104
Compare
Which issue does this PR close?
Part of #8913.
Rationale for this change
LogicalPlan::..._with_subqueries()
methods don't handle correctly if anf_down
closure/method returnsTreeNodeRecursion::Jump
. Currently ifTreeNodeRecursion::Jump
is returned then the node's subquery expression plans are not visited but the node's children are.What changes are included in this PR?
This PR fixes
LogicalPlan::_with_subqueries()
behaviour and whenTreeNodeRecursion::Jump
is returned then neither the node's subquery expression plans nor the node's children are visited.Are these changes tested?
Yes, added new tests.
Are there any user-facing changes?
No.