You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "peter-toth (via GitHub)" <gi...@apache.org> on 2024/04/02 17:45:28 UTC
[PR] Consistent LogicalPlan Subquery handling [arrow-datafusion]
peter-toth opened a new pull request, #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913
## Which issue does this PR close?
Closes #.
## Rationale for this change
`TreeNode` APIs are inconsistent on `LogicaPlan` because `TreeNode:apply()` and `TreeNode::visit()` are recursing on subquery plans too, but `TreeNode::rewrite()` and `TreeNode::transform()` does rewrite/transform only the current level of the plan and doesn't touch subquery plans.
## What changes are included in this PR?
This PR:
- Introduces `LogicalPlan::apply_with_subqueries()`, `LogicalPlan::visit_with_subqueries()` to separate functionality with original the `TreeNode:apply()` and `TreeNode::visit()`. The new explicit `...with_subqueries()` versions do traverse into subquery plans, while the original don't.
- Adds `LogicalPlan::rewrite_with_subqueries()` to be able to rewrite `LogicalPlan`s with their subquery plans.
## Are these changes tested?
Yes, with Existing UTs.
## Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1548533715
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -370,6 +373,221 @@ impl LogicalPlan {
}
}
+ pub fn transform_expressions<F: FnMut(Expr) -> Result<Transformed<Expr>>>(
Review Comment:
Can we please add some comments explaining what this is doing (namely that it applying a function to effectively rewrite the expressions of the LogicalPlan node in place)? It is a very cool API to see actually, as it will hopefully save a bunch of copying
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1080,8 +1298,72 @@ impl LogicalPlan {
}
impl LogicalPlan {
+ pub fn apply_with_subqueries<F: FnMut(&Self) -> Result<TreeNodeRecursion>>(
+ &self,
+ f: &mut F,
+ ) -> Result<TreeNodeRecursion> {
+ // Compared to the default implementation, we need to invoke
+ // [`Self::apply_subqueries`] before visiting its children
+ handle_visit_recursion!(f(self)?, DOWN);
+ self.apply_subqueries(f)?;
+ self.apply_children(&mut |n| n.apply_with_subqueries(f))
+ }
+
+ pub fn visit_with_subqueries<V: TreeNodeVisitor<Node = Self>>(
+ &self,
+ visitor: &mut V,
+ ) -> Result<TreeNodeRecursion> {
+ // Compared to the default implementation, we need to invoke
+ // [`Self::visit_subqueries`] before visiting its children
+ match visitor.f_down(self)? {
+ TreeNodeRecursion::Continue => {
+ self.visit_subqueries(visitor)?;
+ handle_visit_recursion!(
+ self.apply_children(&mut |n| n.visit_with_subqueries(visitor))?,
+ UP
+ );
+ visitor.f_up(self)
+ }
+ TreeNodeRecursion::Jump => {
+ self.visit_subqueries(visitor)?;
Review Comment:
Does this treat subqueries as though they were siblings of the node (like if `TNR::Jump` is returned subquery children will still be visited?)
If so I think I would find that unexpected (I would expect that the subqueries are treated like additional children)
##########
datafusion/optimizer/src/analyzer/mod.rs:
##########
@@ -155,7 +155,7 @@ impl Analyzer {
/// Do necessary check and fail the invalid plan
fn check_plan(plan: &LogicalPlan) -> Result<()> {
- plan.apply(&mut |plan: &LogicalPlan| {
+ plan.apply_with_subqueries(&mut |plan: &LogicalPlan| {
Review Comment:
It is interesting that this didn't change any plans / tests.
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1080,8 +1298,72 @@ impl LogicalPlan {
}
impl LogicalPlan {
+ pub fn apply_with_subqueries<F: FnMut(&Self) -> Result<TreeNodeRecursion>>(
Review Comment:
Likewise here, can we please add docstrings?
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -370,6 +373,221 @@ impl LogicalPlan {
}
}
+ pub fn transform_expressions<F: FnMut(Expr) -> Result<Transformed<Expr>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ Ok(match self {
+ LogicalPlan::Projection(Projection {
+ expr,
+ input,
+ schema,
+ }) => expr
+ .into_iter()
+ .map_until_stop_and_collect(f)?
+ .update_data(|expr| {
+ LogicalPlan::Projection(Projection {
+ expr,
+ input,
+ schema,
+ })
+ }),
+ LogicalPlan::Values(Values { schema, values }) => values
+ .into_iter()
+ .map_until_stop_and_collect(|value| {
+ value.into_iter().map_until_stop_and_collect(&mut f)
+ })?
+ .update_data(|values| LogicalPlan::Values(Values { schema, values })),
+ LogicalPlan::Filter(Filter { predicate, input }) => f(predicate)?
+ .update_data(|predicate| {
+ LogicalPlan::Filter(Filter { predicate, input })
+ }),
+ LogicalPlan::Repartition(Repartition {
+ input,
+ partitioning_scheme,
+ }) => match partitioning_scheme {
+ Partitioning::Hash(expr, usize) => expr
+ .into_iter()
+ .map_until_stop_and_collect(f)?
+ .update_data(|expr| Partitioning::Hash(expr, usize)),
+ Partitioning::DistributeBy(expr) => expr
+ .into_iter()
+ .map_until_stop_and_collect(f)?
+ .update_data(Partitioning::DistributeBy),
+ Partitioning::RoundRobinBatch(_) => Transformed::no(partitioning_scheme),
+ }
+ .update_data(|partitioning_scheme| {
+ LogicalPlan::Repartition(Repartition {
+ input,
+ partitioning_scheme,
+ })
+ }),
+ LogicalPlan::Window(Window {
+ input,
+ window_expr,
+ schema,
+ }) => window_expr
+ .into_iter()
+ .map_until_stop_and_collect(f)?
+ .update_data(|window_expr| {
+ LogicalPlan::Window(Window {
+ input,
+ window_expr,
+ schema,
+ })
+ }),
+ LogicalPlan::Aggregate(Aggregate {
+ input,
+ group_expr,
+ aggr_expr,
+ schema,
+ }) => map_until_stop_and_collect!(
+ group_expr.into_iter().map_until_stop_and_collect(&mut f),
+ aggr_expr,
+ aggr_expr.into_iter().map_until_stop_and_collect(&mut f)
+ )?
+ .update_data(|(group_expr, aggr_expr)| {
+ LogicalPlan::Aggregate(Aggregate {
+ input,
+ group_expr,
+ aggr_expr,
+ schema,
+ })
+ }),
+
+ // There are two part of expression for join, equijoin(on) and non-equijoin(filter).
+ // 1. the first part is `on.len()` equijoin expressions, and the struct of each expr is `left-on = right-on`.
+ // 2. the second part is non-equijoin(filter).
+ LogicalPlan::Join(Join {
+ left,
+ right,
+ on,
+ filter,
+ join_type,
+ join_constraint,
+ schema,
+ null_equals_null,
+ }) => map_until_stop_and_collect!(
+ on.into_iter().map_until_stop_and_collect(
+ |on| map_until_stop_and_collect!(f(on.0), on.1, f(on.1))
+ ),
+ filter,
+ filter.map_or(Ok::<_, DataFusionError>(Transformed::no(None)), |e| {
Review Comment:
is this the magic required to handle `Option`?
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -370,6 +373,221 @@ impl LogicalPlan {
}
}
+ pub fn transform_expressions<F: FnMut(Expr) -> Result<Transformed<Expr>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ Ok(match self {
+ LogicalPlan::Projection(Projection {
+ expr,
+ input,
+ schema,
+ }) => expr
+ .into_iter()
+ .map_until_stop_and_collect(f)?
+ .update_data(|expr| {
Review Comment:
😍 -- that is quite a neat way to express this pattern and get the ownership sorted out 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2041026747
🤔 it appears this PR has some sort of infinite loop -- I will investigate
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1554990514
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -285,57 +287,57 @@ impl LogicalPlan {
/// Calls `f` on all expressions (non-recursively) in the current
/// logical plan node. This does not include expressions in any
/// children.
- pub fn inspect_expressions<F, E>(self: &LogicalPlan, mut f: F) -> Result<(), E>
- where
- F: FnMut(&Expr) -> Result<(), E>,
- {
+ pub fn apply_expressions<F: FnMut(&Expr) -> Result<TreeNodeRecursion>>(
Review Comment:
I've opened https://github.com/apache/arrow-datafusion/pull/9984 as I think we don't need that `inspect_expr_pre()`. This PR together with https://github.com/apache/arrow-datafusion/pull/9984 will remove all usecases of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2041493621
This is now ready for review again. The infinite recursion was due to an extra accidental. `apply()` call: https://github.com/apache/arrow-datafusion/pull/9913/commits/c72349217e0dde13d972e71fa99ae4e5e6025cdf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1554646887
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -285,57 +287,57 @@ impl LogicalPlan {
/// Calls `f` on all expressions (non-recursively) in the current
/// logical plan node. This does not include expressions in any
/// children.
- pub fn inspect_expressions<F, E>(self: &LogicalPlan, mut f: F) -> Result<(), E>
- where
- F: FnMut(&Expr) -> Result<(), E>,
- {
+ pub fn apply_expressions<F: FnMut(&Expr) -> Result<TreeNodeRecursion>>(
Review Comment:
Yes, we can do that. Let me file a new PR with this change tomorrow and then look into the infinite recursion issue of this PR (https://github.com/peter-toth/arrow-datafusion/pull/3). Until then I'm marking this PR as draft.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2040453797
@alamb , I've updated the PR. I will try to add tests for the `LogicalPlan::..._with_subqueries()` APIs in this PR or a in follow-up PR on Monday.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1555960937
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1079,57 +1317,178 @@ impl LogicalPlan {
}
}
+/// This macro is used to determine continuation during combined transforming
Review Comment:
Cool -- thanks -- I think that will be clearer as we consolidate the tree rewriting code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1555643418
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1079,57 +1317,178 @@ impl LogicalPlan {
}
}
+/// This macro is used to determine continuation during combined transforming
Review Comment:
Actually, I deliberately added `..._with_subqueries()` versions of all base `TreeNode` APIs to `LogicalPlan`. These 3 macros are used in `LogicalPlan::rewrite_with_subqueries()`, `LogicalPlan::transform_down_with_subqueries()` and `LogicalPlan::transform_up_with_subqueries()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2036368599
Let me update this PR with the latest `main` (including https://github.com/apache/arrow-datafusion/pull/9876) and address review feedbacks today. Until then I'm marking this PR as draft.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2042283796
Thanks again @peter-toth
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1554305613
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -285,57 +287,57 @@ impl LogicalPlan {
/// Calls `f` on all expressions (non-recursively) in the current
/// logical plan node. This does not include expressions in any
/// children.
- pub fn inspect_expressions<F, E>(self: &LogicalPlan, mut f: F) -> Result<(), E>
- where
- F: FnMut(&Expr) -> Result<(), E>,
- {
+ pub fn apply_expressions<F: FnMut(&Expr) -> Result<TreeNodeRecursion>>(
Review Comment:
This is an api change as it renames a public function. I couldn't find a way to get the error type to line up, so I think it is unavoidable.
I'll mark this PR as API change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1555004078
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -285,57 +287,57 @@ impl LogicalPlan {
/// Calls `f` on all expressions (non-recursively) in the current
/// logical plan node. This does not include expressions in any
/// children.
- pub fn inspect_expressions<F, E>(self: &LogicalPlan, mut f: F) -> Result<(), E>
- where
- F: FnMut(&Expr) -> Result<(), E>,
- {
+ pub fn apply_expressions<F: FnMut(&Expr) -> Result<TreeNodeRecursion>>(
Review Comment:
> I've opened #9984 as I think we don't need that `inspect_expr_pre()`. This PR together with #9984 will remove all usecases of it.
I merged #9984
In order to make this PR easier to merge (so it isn't an API change) I
1. pushed 7c90e70dc to restore `inspect_expressions` (by calling `apply_expressions`) and deprecated it.
2. Removed the API change
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -285,57 +287,57 @@ impl LogicalPlan {
/// Calls `f` on all expressions (non-recursively) in the current
/// logical plan node. This does not include expressions in any
/// children.
- pub fn inspect_expressions<F, E>(self: &LogicalPlan, mut f: F) -> Result<(), E>
- where
- F: FnMut(&Expr) -> Result<(), E>,
- {
+ pub fn apply_expressions<F: FnMut(&Expr) -> Result<TreeNodeRecursion>>(
Review Comment:
Sounds good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1555021187
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1079,57 +1317,178 @@ impl LogicalPlan {
}
}
+/// This macro is used to determine continuation during combined transforming
+/// traversals.
+macro_rules! handle_transform_recursion {
+ ($F_DOWN:expr, $F_CHILD:expr, $F_UP:expr) => {{
+ $F_DOWN?
+ .transform_children(|n| n.map_subqueries($F_CHILD))?
+ .transform_sibling(|n| n.map_children($F_CHILD))?
+ .transform_parent($F_UP)
+ }};
+}
+
+macro_rules! handle_transform_recursion_down {
+ ($F_DOWN:expr, $F_CHILD:expr) => {{
+ $F_DOWN?
+ .transform_children(|n| n.map_subqueries($F_CHILD))?
+ .transform_sibling(|n| n.map_children($F_CHILD))
+ }};
+}
+
+macro_rules! handle_transform_recursion_up {
+ ($SELF:expr, $F_CHILD:expr, $F_UP:expr) => {{
+ $SELF
+ .map_subqueries($F_CHILD)?
+ .transform_sibling(|n| n.map_children($F_CHILD))?
+ .transform_parent(|n| $F_UP(n))
+ }};
+}
+
impl LogicalPlan {
- /// applies `op` to any subqueries in the plan
- pub(crate) fn apply_subqueries<F>(&self, op: &mut F) -> Result<()>
- where
- F: FnMut(&Self) -> Result<TreeNodeRecursion>,
- {
- self.inspect_expressions(|expr| {
- // recursively look for subqueries
- inspect_expr_pre(expr, |expr| {
- match expr {
- Expr::Exists(Exists { subquery, .. })
- | Expr::InSubquery(InSubquery { subquery, .. })
- | Expr::ScalarSubquery(subquery) => {
- // use a synthetic plan so the collector sees a
- // LogicalPlan::Subquery (even though it is
- // actually a Subquery alias)
- let synthetic_plan = LogicalPlan::Subquery(subquery.clone());
- synthetic_plan.apply(op)?;
- }
- _ => {}
+ pub fn visit_with_subqueries<V: TreeNodeVisitor<Node = Self>>(
Review Comment:
as a follow on PR I also plan to add documentation to these functions. Perhaps I can do so as part of moving the code into its own module
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1555583500
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1079,57 +1317,178 @@ impl LogicalPlan {
}
}
+/// This macro is used to determine continuation during combined transforming
+/// traversals.
+macro_rules! handle_transform_recursion {
+ ($F_DOWN:expr, $F_CHILD:expr, $F_UP:expr) => {{
+ $F_DOWN?
+ .transform_children(|n| n.map_subqueries($F_CHILD))?
+ .transform_sibling(|n| n.map_children($F_CHILD))?
+ .transform_parent($F_UP)
+ }};
+}
+
+macro_rules! handle_transform_recursion_down {
+ ($F_DOWN:expr, $F_CHILD:expr) => {{
+ $F_DOWN?
+ .transform_children(|n| n.map_subqueries($F_CHILD))?
+ .transform_sibling(|n| n.map_children($F_CHILD))
+ }};
+}
+
+macro_rules! handle_transform_recursion_up {
+ ($SELF:expr, $F_CHILD:expr, $F_UP:expr) => {{
+ $SELF
+ .map_subqueries($F_CHILD)?
+ .transform_sibling(|n| n.map_children($F_CHILD))?
+ .transform_parent(|n| $F_UP(n))
+ }};
+}
+
impl LogicalPlan {
- /// applies `op` to any subqueries in the plan
- pub(crate) fn apply_subqueries<F>(&self, op: &mut F) -> Result<()>
- where
- F: FnMut(&Self) -> Result<TreeNodeRecursion>,
- {
- self.inspect_expressions(|expr| {
- // recursively look for subqueries
- inspect_expr_pre(expr, |expr| {
- match expr {
- Expr::Exists(Exists { subquery, .. })
- | Expr::InSubquery(InSubquery { subquery, .. })
- | Expr::ScalarSubquery(subquery) => {
- // use a synthetic plan so the collector sees a
- // LogicalPlan::Subquery (even though it is
- // actually a Subquery alias)
- let synthetic_plan = LogicalPlan::Subquery(subquery.clone());
- synthetic_plan.apply(op)?;
- }
- _ => {}
+ pub fn visit_with_subqueries<V: TreeNodeVisitor<Node = Self>>(
Review Comment:
Docs: https://github.com/apache/arrow-datafusion/pull/9996
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1554991056
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -285,57 +287,57 @@ impl LogicalPlan {
/// Calls `f` on all expressions (non-recursively) in the current
/// logical plan node. This does not include expressions in any
/// children.
- pub fn inspect_expressions<F, E>(self: &LogicalPlan, mut f: F) -> Result<(), E>
- where
- F: FnMut(&Expr) -> Result<(), E>,
- {
+ pub fn apply_expressions<F: FnMut(&Expr) -> Result<TreeNodeRecursion>>(
Review Comment:
As for https://github.com/apache/arrow-datafusion/blob/27bdf3c8c976874aae2417cb2aa435448832ecc0/datafusion/optimizer/src/analyzer/subquery.rs#L236-L241 I plan to check all `.expressions()` calls and see if it can be transformed to the better `.apply_expressions()` once this PR is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2041521091
I am not sure what you are planning on working on next @peter-toth but one thing I noticed is that keeping track of all the `LogicalPlan` tree node walking / rewriting is somewhat hard for me (and `datafusion/expr/src/logical_plan/plan.rs` is getting pretty large).
I wonder if we should move all the code into a single module (maybe alongside the `impl TreeNode for LogicalPlan`) ? Maybe somethign like `datafusion/expr/src/logical_plan/tree_node.rs` or `datafusion/expr/src/logical_plan/apply_visit.rs` 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2041556063
> I took the liberty of restring `inspect_expressions` and marking it deprecated and merging up from main.
Sure, thanks for doing that!
> Removed the API change
I was thinking about this a bit more and I unfortunately think this PR is still an API change. If some external code calls `.apply()` on a `LogicalPlan` then this PR might break it. This is because the implementation of `.apply()`will be de default implementation of `TreeNode::apply()` after this PR and no longer recurse into subqueries. The API user needs to change the code to `.apply_with_subqueries()` to get the old behaviour.
But I have no other idea to fix the current inconsistency between `.apply()`/`.visit()`, that do recurse into subqueries and `.rewrite()`/`.transform()`, that don't. (Well we could change `.rewrite()`/`.transform()` to do recurse into subqueries, but that too would be a breaking change.)
> I am not sure what you are planning on working on next @peter-toth but one thing I noticed is that keeping track of all the LogicalPlan tree node walking / rewriting is somewhat hard for me (and datafusion/expr/src/logical_plan/plan.rs is getting pretty large).
I agree. I'm happy to take that task, but I will be travelling from Tuesday till the end of the week, so most likely I can do that once I'm back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2035312114
> Thank you (again) @peter-toth -- this looks really nice as well
>
> I think there are is some more documentation we could add, but overall the only concern I have about this PR is the lack of test changes (like it seems to change the semantics of the analyzer / correlated expressions but no tests are changed). So this perhaps suggests we have a lack of test coverage in this area 🤔
I rephrased the PR description to be clear. This PR doesn't change the analyzer or anything that uses `LogicalPlan` APIs. This PR just moves the overriden `TreeNode` APIs (`apply()`/`visit()`) in `LogicalPlan` to under new names so as to be clear they recurse into subqueries. Also, this PR adds the missing `LogicalPlan::rewrite_with_subqueries()` that is capable to rewrite `LogicalPlan`s with their subqueries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2041039672
> > 🤔 it appears this PR has some sort of infinite loop -- I will investigate
>
> Thanks @alamb . Maybe because now we treat subqueries as additional children? I can also look into it later today or tomorrow...
Thanks @peter-toth -- yes I think you are correct. I debugged a bit and I found that calling apply_subqueries cased infinite recursion.
I removed the apply_subqueries call (see this https://github.com/peter-toth/arrow-datafusion/pull/3) but then some of the subquery plans don't work
So now I am confused -- should `apply` visit all subquery `LogicalPlan`s or not? Maybe it does make sense, as you said above, to have separate `apply` and `apply_subquery` calls 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1555004546
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -285,57 +287,57 @@ impl LogicalPlan {
/// Calls `f` on all expressions (non-recursively) in the current
/// logical plan node. This does not include expressions in any
/// children.
- pub fn inspect_expressions<F, E>(self: &LogicalPlan, mut f: F) -> Result<(), E>
- where
- F: FnMut(&Expr) -> Result<(), E>,
- {
+ pub fn apply_expressions<F: FnMut(&Expr) -> Result<TreeNodeRecursion>>(
Review Comment:
> I plan to check all .expressions() calls and see if it can be transformed to the better .apply_expressions() once this PR is merged.
This would be good -- I think there are some more (though https://github.com/apache/arrow-datafusion/pull/9974 did get merged)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2042466206
Thanks @alamb for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1550218443
##########
datafusion/optimizer/src/analyzer/mod.rs:
##########
@@ -155,7 +155,7 @@ impl Analyzer {
/// Do necessary check and fail the invalid plan
fn check_plan(plan: &LogicalPlan) -> Result<()> {
- plan.apply(&mut |plan: &LogicalPlan| {
+ plan.apply_with_subqueries(&mut |plan: &LogicalPlan| {
Review Comment:
Basically I just moved the code of `LogicalPlan:: apply()` (that overrided `TreeNode:: apply()`) to `LogicalPlan:: apply_with_subqueries()` and changed the `LogicalPlan:: apply()` calls to `LogicalPlan:: apply_with_subqueries()`. I updated the PR description to be clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1550241162
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1080,8 +1298,72 @@ impl LogicalPlan {
}
impl LogicalPlan {
+ pub fn apply_with_subqueries<F: FnMut(&Self) -> Result<TreeNodeRecursion>>(
+ &self,
+ f: &mut F,
+ ) -> Result<TreeNodeRecursion> {
+ // Compared to the default implementation, we need to invoke
+ // [`Self::apply_subqueries`] before visiting its children
+ handle_visit_recursion!(f(self)?, DOWN);
+ self.apply_subqueries(f)?;
+ self.apply_children(&mut |n| n.apply_with_subqueries(f))
+ }
+
+ pub fn visit_with_subqueries<V: TreeNodeVisitor<Node = Self>>(
+ &self,
+ visitor: &mut V,
+ ) -> Result<TreeNodeRecursion> {
+ // Compared to the default implementation, we need to invoke
+ // [`Self::visit_subqueries`] before visiting its children
+ match visitor.f_down(self)? {
+ TreeNodeRecursion::Continue => {
+ self.visit_subqueries(visitor)?;
+ handle_visit_recursion!(
+ self.apply_children(&mut |n| n.visit_with_subqueries(visitor))?,
+ UP
+ );
+ visitor.f_up(self)
+ }
+ TreeNodeRecursion::Jump => {
+ self.visit_subqueries(visitor)?;
Review Comment:
I just moved the code here from `LogicalPlan::visit()` and in this PR I didn't want to change how subqueries are handled. The new code I added in `LogicalPlan::rewrite_with_subqueries()` are in sync with this behaviour.
But this is a good question and I think you are right and I too would treat them as additional children. But that would be an API behavior change so I would rather change that in a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1554563916
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -285,57 +287,57 @@ impl LogicalPlan {
/// Calls `f` on all expressions (non-recursively) in the current
/// logical plan node. This does not include expressions in any
/// children.
- pub fn inspect_expressions<F, E>(self: &LogicalPlan, mut f: F) -> Result<(), E>
- where
- F: FnMut(&Expr) -> Result<(), E>,
- {
+ pub fn apply_expressions<F: FnMut(&Expr) -> Result<TreeNodeRecursion>>(
Review Comment:
Actually, I found another pattern here: https://github.com/apache/arrow-datafusion/blob/d201ec742b08fa2a0549fc27f0b39622d67391ab/datafusion/expr/src/utils.rs#L684
Maybe we can pull this part (add `apply_expressions` into a separate PR)? I think it would be valuable in its own -- for example it could be used to void the `expressions()` copy here:
https://github.com/apache/arrow-datafusion/blob/27bdf3c8c976874aae2417cb2aa435448832ecc0/datafusion/optimizer/src/analyzer/subquery.rs#L236-L241
It could also make this PR smaller
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2042283375
> I was thinking about this a bit more and I unfortunately think this PR is still an API change.
That makes sense -- I have added the label back.
> But I have no other idea to fix the current inconsistency between
I think it is inevitable that there are different calls for `LogicalPlan` for traversing with subqueries and without subqueries
> I agree. I'm happy to take that task,
Awesome -- thank you. I filed https://github.com/apache/arrow-datafusion/issues/9994 to track the idea. I may make a few small PRs in this direction to start the process too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2040625078
> @alamb, I've updated the PR. I will try to add tests for the `LogicalPlan::..._with_subqueries()` APIs in this PR or a in follow-up PR on Monday.
I think it would be great to do this as a follow up PR on Monday, however I think the utility of unit tests on this code is somewhat limited. It will be tested end to end once I switch the optimizers to using the TreeNode API in https://github.com/apache/arrow-datafusion/pull/9954.
I took the liberty of pushing a commit to this branch to fix clippy and plan to merge it in when CI passes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1555582454
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1079,57 +1317,178 @@ impl LogicalPlan {
}
}
+/// This macro is used to determine continuation during combined transforming
Review Comment:
I think this may be dead code -- can somone check https://github.com/apache/arrow-datafusion/pull/9997 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2041031636
> 🤔 it appears this PR has some sort of infinite loop -- I will investigate
Thanks @alamb . Maybe because now we treat subqueries as additional children? I can also look into it later today or tomorrow...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit. Rename `inspect_expressions` to `apply_expressions` [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1554990514
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -285,57 +287,57 @@ impl LogicalPlan {
/// Calls `f` on all expressions (non-recursively) in the current
/// logical plan node. This does not include expressions in any
/// children.
- pub fn inspect_expressions<F, E>(self: &LogicalPlan, mut f: F) -> Result<(), E>
- where
- F: FnMut(&Expr) -> Result<(), E>,
- {
+ pub fn apply_expressions<F: FnMut(&Expr) -> Result<TreeNodeRecursion>>(
Review Comment:
I've opened https://github.com/apache/arrow-datafusion/pull/9984 as I think we don't need that `inspect_expr_pre()`, this PR together with https://github.com/apache/arrow-datafusion/pull/9984 will remove all usecases of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #9913:
URL: https://github.com/apache/arrow-datafusion/pull/9913#issuecomment-2040026476
@alamb , while I was working on this I realized that currently `TreeNode` recursion definitions are a bit hard to follow and could be simplified, which will be useful this PR as well. I've opened https://github.com/apache/arrow-datafusion/pull/9965 to do that. Can we please review/merge that https://github.com/apache/arrow-datafusion/pull/9965 first, then I can wrap up this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org