Fix LogicalPlan::..._with_subqueries methods

[peter-toth]

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.

[peter-toth]

cc @alamb , @berkaysynnada

[alamb]

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

[alamb]

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?

[peter-toth]

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.

[alamb]

💯 comments

[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

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.

[berkaysynnada]

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]

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.

[alamb]

Thanks @peter-toth and @berkaysynnada -- love it

[peter-toth]

Thanks for the review @alamb and @berkaysynnada!