Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

peter-toth
Copy link
Contributor

@peter-toth peter-toth commented Nov 28, 2024

Which issue does this PR close?

Part of #8913.

Rationale for this change

LogicalPlan::..._with_subqueries() methods don't handle correctly if an f_down closure/method returns TreeNodeRecursion::Jump. Currently if TreeNodeRecursion::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 when TreeNodeRecursion::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.

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Nov 28, 2024
@peter-toth
Copy link
Contributor Author

cc @alamb , @berkaysynnada

Copy link
Contributor

@alamb alamb left a 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(|| {
Copy link
Contributor

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?

Copy link
Contributor Author

@peter-toth peter-toth Nov 28, 2024

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 comments

@peter-toth
Copy link
Contributor Author

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

Thanks so much @alamb for verifying this! Currently we don't return jump anywhere in ..._with_subqueries(), but I plan on opening a proof of concept PR that will use it extensively and that's how I noticed the bug.

Copy link
Contributor

@berkaysynnada berkaysynnada left a 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.

@peter-toth
Copy link
Contributor Author

peter-toth commented Nov 29, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants