You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "peter-toth (via GitHub)" <gi...@apache.org> on 2024/11/18 15:38:30 UTC
[PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
peter-toth opened a new pull request, #13467:
URL: https://github.com/apache/datafusion/pull/13467
## Which issue does this PR close?
Part of https://github.com/apache/datafusion/issues/8913.
## Rationale for this change
The current implementation of `LogicalPlan:apply_children()`, `LogicalPlan::map_children()`, `LogicalPlan::apply_expressions()`, `LogicalPlan::map_expressions()`, `Expr::apply_children()` and `Expr::map_children()` are confusing due the `map_until_stop_and_collect` macro. I think we can introduce a trait that can contain sibling arbitrary elements that can be applied on and mapped:
```
/// [`Container`] contains elements that a function can be applied on or mapped. The
/// elements of the container are siblings so the continuation rules are similar to
/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
pub trait Container<'a, T: 'a>: Sized {
fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
&'a self,
f: F,
) -> Result<TreeNodeRecursion>;
fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
self,
f: F,
) -> Result<Transformed<Self>>;
}
```
## What changes are included in this PR?
This PR:
- Gets rid of `map_until_stop_and_collect` macro.
- Adds the `Container` trait and blanket implementations for `Box`, `Option`, `Vec`, tuples, ...
- Defined the `Container` implementation for `Expr` and `LogicalPlan`.
- Simplifies the above mentioned `apply...` and `map...` methods.
## Are these changes tested?
Yes, with exitsing 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848073920
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ (*self).map_elements(f)?.map_data(|c| Ok(Self::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T> + Clone> Container<'a, T> for Arc<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ Arc::unwrap_or_clone(self)
+ .map_elements(f)?
+ .map_data(|c| Ok(Arc::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Option<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ match self {
+ Some(t) => t.apply_elements(f),
+ None => Ok(TreeNodeRecursion::Continue),
+ }
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ self.map_or(Ok(Transformed::no(None)), |c| {
+ c.map_elements(f)?.map_data(|c| Ok(Some(c)))
+ })
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Vec<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ mut f: F,
+ ) -> Result<TreeNodeRecursion> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ for c in self {
+ tnr = c.apply_elements(&mut f)?;
+ match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
+ TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
+ }
+ }
+ Ok(tnr)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ let mut transformed = false;
+ self.into_iter()
+ .map(|c| match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {
+ c.map_elements(&mut f).map(|result| {
+ tnr = result.tnr;
+ transformed |= result.transformed;
+ result.data
+ })
+ }
+ TreeNodeRecursion::Stop => Ok(c),
+ })
+ .collect::<Result<Vec<_>>>()
+ .map(|data| Transformed::new(data, transformed, tnr))
+ }
+}
+
+impl<'a, T: 'a, K: Eq + Hash, C: Container<'a, T>> Container<'a, T> for HashMap<K, C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ mut f: F,
+ ) -> Result<TreeNodeRecursion> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ for c in self.values() {
+ tnr = c.apply_elements(&mut f)?;
+ match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
+ TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
+ }
+ }
+ Ok(tnr)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ let mut transformed = false;
+ self.into_iter()
+ .map(|(k, c)| match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {
+ c.map_elements(&mut f).map(|result| {
+ tnr = result.tnr;
+ transformed |= result.transformed;
+ (k, result.data)
+ })
+ }
+ TreeNodeRecursion::Stop => Ok((k, c)),
+ })
+ .collect::<Result<HashMap<_, _>>>()
+ .map(|data| Transformed::new(data, transformed, tnr))
+ }
+}
+
+impl<'a, T: 'a, C0: Container<'a, T>, C1: Container<'a, T>> Container<'a, T>
Review Comment:
I updated the PR description with an example to show how the new container can be used. I tried to use the complex example of `Expr::Case`. Its fields contain `Box`, `Option`, `Vec` and 2-tuple as well.
The 3-tuple blanket implementation is not used in any `TreeNodes`, but it makes possible to get rid of the ugly `map_until_stop_and_collect` macro as we can put different typed fields into a 3-tupple and just call `apply_elements()` / `map_elements()` on it.
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -81,7 +79,7 @@ impl TreeNode for LogicalPlan {
expr,
input,
schema,
- }) => rewrite_arc(input, f)?.update_data(|input| {
+ }) => input.map_elements(f)?.update_data(|input| {
Review Comment:
> IIUC map_children defines how LogicalPlan's construct a tree topology, while map_elements is resolving the link between nodes.
Yes, the container trait supposes that the elements in it are sibling nodes. Basically this is why we can use it when we implement `apply_children()` / `map_children()`. The blanket implementations for the trait makes it very simple to use with `Expr` and `LogicalPlan` as those logical plan treenodes define their children as a composition of `Vec`s, `Option`s, tuples, `Box`es and `Arc`s.
Here in `LogicalPlan` a treenode has mostly only 1 child (except for `Join` and `Union` maybe) so input (`Arc<LogicalPlan>`) is just a single element container so this PR doesn't have much benefit, but I think it is worth to do it to allign `Expr` and `LogicalPlan`.
`TreeNode::map_children` and `TreeNodeContainer:: map_elements` are similar in a way that both apply `f` to the children/elements of a `TreeNode` / `TreeNodeContainer`. But we use `TreeNode` to define different kids of logical
and physical plan trees, while `TreeNodeContainer` can be used to simplify implementation of `apply_children` / `map_children()`.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#issuecomment-2484071423
cc @alamb
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1847285822
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
Review Comment:
Could we also add some documentation here about what apply_elements is (perhaps just a map back to `TreeNode::apply`?)
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
Review Comment:
Could we also add some documentation here about what map_elements is (perhaps just a map back to `TreeNode::map_children`?)
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ (*self).map_elements(f)?.map_data(|c| Ok(Self::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T> + Clone> Container<'a, T> for Arc<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ Arc::unwrap_or_clone(self)
+ .map_elements(f)?
+ .map_data(|c| Ok(Arc::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Option<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ match self {
+ Some(t) => t.apply_elements(f),
+ None => Ok(TreeNodeRecursion::Continue),
+ }
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ self.map_or(Ok(Transformed::no(None)), |c| {
+ c.map_elements(f)?.map_data(|c| Ok(Some(c)))
+ })
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Vec<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ mut f: F,
+ ) -> Result<TreeNodeRecursion> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ for c in self {
+ tnr = c.apply_elements(&mut f)?;
+ match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
+ TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
+ }
+ }
+ Ok(tnr)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ let mut transformed = false;
+ self.into_iter()
+ .map(|c| match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {
+ c.map_elements(&mut f).map(|result| {
+ tnr = result.tnr;
+ transformed |= result.transformed;
+ result.data
+ })
+ }
+ TreeNodeRecursion::Stop => Ok(c),
+ })
+ .collect::<Result<Vec<_>>>()
+ .map(|data| Transformed::new(data, transformed, tnr))
+ }
+}
+
+impl<'a, T: 'a, K: Eq + Hash, C: Container<'a, T>> Container<'a, T> for HashMap<K, C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ mut f: F,
+ ) -> Result<TreeNodeRecursion> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ for c in self.values() {
+ tnr = c.apply_elements(&mut f)?;
+ match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
+ TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
+ }
+ }
+ Ok(tnr)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ let mut transformed = false;
+ self.into_iter()
+ .map(|(k, c)| match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {
+ c.map_elements(&mut f).map(|result| {
+ tnr = result.tnr;
+ transformed |= result.transformed;
+ (k, result.data)
+ })
+ }
+ TreeNodeRecursion::Stop => Ok((k, c)),
+ })
+ .collect::<Result<HashMap<_, _>>>()
+ .map(|data| Transformed::new(data, transformed, tnr))
+ }
+}
+
+impl<'a, T: 'a, C0: Container<'a, T>, C1: Container<'a, T>> Container<'a, T>
Review Comment:
It took me a moment to realize this was the impl for `(a,b)` 2-tuples. Maybe we could make that clearer with some comments.
Likewise with the 3-tuple.
Though to be honest, it seems to me like it might be clearer if we didn't implement this trait for tuples, and instead made the places they are used, named `struct`s. But that is just a personal style preference
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -81,7 +79,7 @@ impl TreeNode for LogicalPlan {
expr,
input,
schema,
- }) => rewrite_arc(input, f)?.update_data(|input| {
+ }) => input.map_elements(f)?.update_data(|input| {
Review Comment:
Is the difference between `map_children` and `map_elements` that `map_elements` doesn't also apply the function to `input` ?
##########
datafusion/expr/src/logical_plan/ddl.rs:
##########
@@ -497,6 +522,29 @@ pub struct CreateFunctionBody {
pub function_body: Option<Expr>,
}
+impl<'a> Container<'a, Expr> for CreateFunctionBody {
+ fn apply_elements<F: FnMut(&'a Expr) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.function_body.apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(Expr) -> Result<Transformed<Expr>>>(
Review Comment:
this is very cool
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -423,54 +405,40 @@ impl LogicalPlan {
mut f: F,
) -> Result<TreeNodeRecursion> {
match self {
- LogicalPlan::Projection(Projection { expr, .. }) => {
- expr.iter().apply_until_stop(f)
- }
- LogicalPlan::Values(Values { values, .. }) => values
- .iter()
- .apply_until_stop(|value| value.iter().apply_until_stop(&mut f)),
+ LogicalPlan::Projection(Projection { expr, .. }) => expr.apply_elements(f),
+ LogicalPlan::Values(Values { values, .. }) => values.apply_elements(f),
LogicalPlan::Filter(Filter { predicate, .. }) => f(predicate),
LogicalPlan::Repartition(Repartition {
partitioning_scheme,
..
}) => match partitioning_scheme {
Partitioning::Hash(expr, _) | Partitioning::DistributeBy(expr) => {
- expr.iter().apply_until_stop(f)
+ expr.apply_elements(f)
}
Partitioning::RoundRobinBatch(_) => Ok(TreeNodeRecursion::Continue),
},
LogicalPlan::Window(Window { window_expr, .. }) => {
- window_expr.iter().apply_until_stop(f)
+ window_expr.apply_elements(f)
}
LogicalPlan::Aggregate(Aggregate {
group_expr,
aggr_expr,
..
- }) => group_expr
- .iter()
- .chain(aggr_expr.iter())
- .apply_until_stop(f),
+ }) => (group_expr, aggr_expr).apply_ref_elements(f),
// 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 { on, filter, .. }) => {
- on.iter()
- // TODO: why we need to create an `Expr::eq`? Cloning `Expr` is costly...
Review Comment:
🎉 for removing the clone
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ (*self).map_elements(f)?.map_data(|c| Ok(Self::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T> + Clone> Container<'a, T> for Arc<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ Arc::unwrap_or_clone(self)
+ .map_elements(f)?
+ .map_data(|c| Ok(Arc::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Option<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ match self {
+ Some(t) => t.apply_elements(f),
+ None => Ok(TreeNodeRecursion::Continue),
+ }
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ self.map_or(Ok(Transformed::no(None)), |c| {
+ c.map_elements(f)?.map_data(|c| Ok(Some(c)))
+ })
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Vec<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ mut f: F,
+ ) -> Result<TreeNodeRecursion> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ for c in self {
+ tnr = c.apply_elements(&mut f)?;
+ match tnr {
Review Comment:
this is definitely a much easier to understand formulation
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
Review Comment:
I think it is a matter of preference, but what would you think about calling this something less generic, such as `TreeNodeContainer` ?
##########
datafusion/expr/src/tree_node.rs:
##########
@@ -57,78 +57,50 @@ impl TreeNode for Expr {
| Expr::Negative(expr)
| Expr::Cast(Cast { expr, .. })
| Expr::TryCast(TryCast { expr, .. })
- | Expr::InSubquery(InSubquery{ expr, .. }) => vec![expr.as_ref()],
+ | Expr::InSubquery(InSubquery { expr, .. }) => expr.apply_elements(f),
Review Comment:
it is nice to avoid creating vecs here as well
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848256486
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -423,54 +405,40 @@ impl LogicalPlan {
mut f: F,
) -> Result<TreeNodeRecursion> {
match self {
- LogicalPlan::Projection(Projection { expr, .. }) => {
- expr.iter().apply_until_stop(f)
- }
- LogicalPlan::Values(Values { values, .. }) => values
- .iter()
- .apply_until_stop(|value| value.iter().apply_until_stop(&mut f)),
+ LogicalPlan::Projection(Projection { expr, .. }) => expr.apply_elements(f),
+ LogicalPlan::Values(Values { values, .. }) => values.apply_elements(f),
LogicalPlan::Filter(Filter { predicate, .. }) => f(predicate),
LogicalPlan::Repartition(Repartition {
partitioning_scheme,
..
}) => match partitioning_scheme {
Partitioning::Hash(expr, _) | Partitioning::DistributeBy(expr) => {
- expr.iter().apply_until_stop(f)
+ expr.apply_elements(f)
}
Partitioning::RoundRobinBatch(_) => Ok(TreeNodeRecursion::Continue),
},
LogicalPlan::Window(Window { window_expr, .. }) => {
- window_expr.iter().apply_until_stop(f)
+ window_expr.apply_elements(f)
}
LogicalPlan::Aggregate(Aggregate {
group_expr,
aggr_expr,
..
- }) => group_expr
- .iter()
- .chain(aggr_expr.iter())
- .apply_until_stop(f),
+ }) => (group_expr, aggr_expr).apply_ref_elements(f),
// 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 { on, filter, .. }) => {
- on.iter()
- // TODO: why we need to create an `Expr::eq`? Cloning `Expr` is costly...
Review Comment:
I don't know what to do with this. Based on the test results we don't need that extra `Expr::eq` node during tree visits, but some downstream projects might depend on 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848077369
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
Review Comment:
Yeah, indeed that's a better name for container. Let me change 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848079907
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
Review Comment:
Good idea, I will fix it soon.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848246474
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
Review Comment:
Actually, introducing `dyn FnMut` (trait objects) would mean dynamic dispatch and so come with performance penalty. Trait alias (https://doc.rust-lang.org/beta/unstable-book/language-features/trait-alias.html) is an unstalbe feature so I'm not sure we can do this yet.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848124393
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -423,54 +405,40 @@ impl LogicalPlan {
mut f: F,
) -> Result<TreeNodeRecursion> {
match self {
- LogicalPlan::Projection(Projection { expr, .. }) => {
- expr.iter().apply_until_stop(f)
- }
- LogicalPlan::Values(Values { values, .. }) => values
- .iter()
- .apply_until_stop(|value| value.iter().apply_until_stop(&mut f)),
+ LogicalPlan::Projection(Projection { expr, .. }) => expr.apply_elements(f),
+ LogicalPlan::Values(Values { values, .. }) => values.apply_elements(f),
LogicalPlan::Filter(Filter { predicate, .. }) => f(predicate),
LogicalPlan::Repartition(Repartition {
partitioning_scheme,
..
}) => match partitioning_scheme {
Partitioning::Hash(expr, _) | Partitioning::DistributeBy(expr) => {
- expr.iter().apply_until_stop(f)
+ expr.apply_elements(f)
}
Partitioning::RoundRobinBatch(_) => Ok(TreeNodeRecursion::Continue),
},
LogicalPlan::Window(Window { window_expr, .. }) => {
- window_expr.iter().apply_until_stop(f)
+ window_expr.apply_elements(f)
}
LogicalPlan::Aggregate(Aggregate {
group_expr,
aggr_expr,
..
- }) => group_expr
- .iter()
- .chain(aggr_expr.iter())
- .apply_until_stop(f),
+ }) => (group_expr, aggr_expr).apply_ref_elements(f),
// 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 { on, filter, .. }) => {
- on.iter()
- // TODO: why we need to create an `Expr::eq`? Cloning `Expr` is costly...
Review Comment:
Oops, I didn't want to remove the artificial `Expr::eq` between the left and right sides as some rules might depend on it. (Unfortunately that artificial node requires the clone...)
But actually it is a bit weird that removing it didn't cause any test failures...
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#issuecomment-2485810809
Let me check a few more things before we merge this PR. There might be a way to make this even simpler.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#issuecomment-2483643169
> This PR:
>
> * Gets rid of `map_until_stop_and_collect` macro
that's great
> Adds the Container trait and blanket implementations for Box, Option, Vec, tuples, ...
Do we need that?
What about calling this trait TreeNode or GraphNode and implementing it for our types only?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#issuecomment-2484069956
> > Adds the Container trait and blanket implementations for Box, Option, Vec, tuples, ...
>
> Do we need that?
>
> What about calling this trait TreeNode or GraphNode and implementing it for our types only?
Yes, we do. We already have the `TreeNode` trait with a well defined API. It has `TreeNode::apply_children()` / `TreeNode::map_children()` to let the tree implementations define how to visit/map the children of a node.
This new `Container` trait with the above mentioned blankets just make the implementation of that `apply_children()` / `map_children()` of logical plan trees (`Expr` and `LogicalPlan`) simpler.
We can actually move `Container` and its blanket implementations to `datafusion::expr` if that's cleaner.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada merged PR #13467:
URL: https://github.com/apache/datafusion/pull/13467
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848246474
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
Review Comment:
Actually, introducing `dyn FnMut` (trait objects) would mean dynamic dispatch and so come with performance penalty. We could use trait aliases (https://doc.rust-lang.org/beta/unstable-book/language-features/trait-alias.html) but it is an unstalbe feature so I'm not sure we can do this yet.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848246474
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
Review Comment:
Actually, introducing `dyn FnMut` (trait objects) would mean dynamic dispatch and so come with performance penalty. Trait aliase (https://doc.rust-lang.org/beta/unstable-book/language-features/trait-alias.html) is an unstalbe feature yet.
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
Review Comment:
Actually, introducing `dyn FnMut` (trait objects) would mean dynamic dispatch and so come with performance penalty. Trait alias (https://doc.rust-lang.org/beta/unstable-book/language-features/trait-alias.html) is an unstalbe feature yet.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848074244
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -81,7 +79,7 @@ impl TreeNode for LogicalPlan {
expr,
input,
schema,
- }) => rewrite_arc(input, f)?.update_data(|input| {
+ }) => input.map_elements(f)?.update_data(|input| {
Review Comment:
> IIUC map_children defines how LogicalPlan's construct a tree topology, while map_elements is resolving the link between nodes.
Yes, the container trait supposes that the elements in it are sibling nodes. Basically this is why we can use it when we implement `apply_children()` / `map_children()`. The blanket implementations for the trait makes it very simple to use with `Expr` and `LogicalPlan` as those logical plan treenodes define their children as a composition of `Vec`s, `Option`s, tuples, `Box`es and `Arc`s.
Here in `LogicalPlan` a treenode has mostly only 1 child (except for `Join` and `Union` maybe) so input (`Arc<LogicalPlan>`) is just a single element container so this PR doesn't have much benefit, but I think it is worth to allign `Expr` and `LogicalPlan` code.
`TreeNode::map_children` and `TreeNodeContainer:: map_elements` are similar in a way that both apply `f` to the children/elements of a `TreeNode` / `TreeNodeContainer`. But we use `TreeNode` to define different kids of logical
and physical plan trees, while `TreeNodeContainer` can be used to simplify implementation of `apply_children` / `map_children()`.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848246474
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
Review Comment:
Actually, introducing `dyn FnMut` (trait objects) would mean dynamic dispatch and so come with performance penalty. But trait alias (https://doc.rust-lang.org/beta/unstable-book/language-features/trait-alias.html) is an unstalbe feature so I'm not sure we can do this yet.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848715982
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -81,7 +79,7 @@ impl TreeNode for LogicalPlan {
expr,
input,
schema,
- }) => rewrite_arc(input, f)?.update_data(|input| {
+ }) => input.map_elements(f)?.update_data(|input| {
Review Comment:
I clarified this in https://github.com/apache/datafusion/pull/13467/commits/2097bc7603097f4106d68be9f1b6a281d4bdbb7e.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848373704
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
Review Comment:
Renamed in https://github.com/apache/datafusion/pull/13467/commits/e5aff72b1ea9af816b1465d3f8d09c0f42983a5b.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#issuecomment-2487938287
Thanks for the review @alamb , @berkaysynnada , @findepi!
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848073920
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ (*self).map_elements(f)?.map_data(|c| Ok(Self::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T> + Clone> Container<'a, T> for Arc<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ Arc::unwrap_or_clone(self)
+ .map_elements(f)?
+ .map_data(|c| Ok(Arc::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Option<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ match self {
+ Some(t) => t.apply_elements(f),
+ None => Ok(TreeNodeRecursion::Continue),
+ }
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ self.map_or(Ok(Transformed::no(None)), |c| {
+ c.map_elements(f)?.map_data(|c| Ok(Some(c)))
+ })
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Vec<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ mut f: F,
+ ) -> Result<TreeNodeRecursion> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ for c in self {
+ tnr = c.apply_elements(&mut f)?;
+ match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
+ TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
+ }
+ }
+ Ok(tnr)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ let mut transformed = false;
+ self.into_iter()
+ .map(|c| match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {
+ c.map_elements(&mut f).map(|result| {
+ tnr = result.tnr;
+ transformed |= result.transformed;
+ result.data
+ })
+ }
+ TreeNodeRecursion::Stop => Ok(c),
+ })
+ .collect::<Result<Vec<_>>>()
+ .map(|data| Transformed::new(data, transformed, tnr))
+ }
+}
+
+impl<'a, T: 'a, K: Eq + Hash, C: Container<'a, T>> Container<'a, T> for HashMap<K, C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ mut f: F,
+ ) -> Result<TreeNodeRecursion> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ for c in self.values() {
+ tnr = c.apply_elements(&mut f)?;
+ match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
+ TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
+ }
+ }
+ Ok(tnr)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ let mut transformed = false;
+ self.into_iter()
+ .map(|(k, c)| match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {
+ c.map_elements(&mut f).map(|result| {
+ tnr = result.tnr;
+ transformed |= result.transformed;
+ (k, result.data)
+ })
+ }
+ TreeNodeRecursion::Stop => Ok((k, c)),
+ })
+ .collect::<Result<HashMap<_, _>>>()
+ .map(|data| Transformed::new(data, transformed, tnr))
+ }
+}
+
+impl<'a, T: 'a, C0: Container<'a, T>, C1: Container<'a, T>> Container<'a, T>
Review Comment:
I updated the PR description with an example to show how the new container can be used. I tried to use the complex example of `Expr::Case` as its fields contain `Box`, `Option`, `Vec` and 2-tuple as well.
The 3-tuple blanket implementation is not used in any `TreeNodes`, but it makes possible to get rid of the ugly `map_until_stop_and_collect` macro as we can put different typed fields into a 3-tupple and just call `apply_elements()` / `map_elements()` on 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848074244
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -81,7 +79,7 @@ impl TreeNode for LogicalPlan {
expr,
input,
schema,
- }) => rewrite_arc(input, f)?.update_data(|input| {
+ }) => input.map_elements(f)?.update_data(|input| {
Review Comment:
> IIUC map_children defines how LogicalPlan's construct a tree topology, while map_elements is resolving the link between nodes.
Yes, the container trait supposes that the elements in it are sibling nodes. Basically this is why we can use it when we implement `apply_children()` / `map_children()`. The blanket implementations for the trait makes it very simple to use with `Expr` and `LogicalPlan` as those logical plan treenodes define their children as a composition of `Vec`s, `Option`s, tuples, `Box`es and `Arc`s.
Here in `LogicalPlan` a treenode has mostly only 1 child (except for `Join` and `Union` maybe) so input (`Arc<LogicalPlan>`) is just a single element container so this PR doesn't have much benefit, but I think it is worth to allign `Expr` and `LogicalPlan` code.
`TreeNode::map_children` and `TreeNodeContainer:: map_elements` are similar in a way that both apply `f` to their children / elements of a `TreeNode` / `TreeNodeContainer`. But we use `TreeNode` to define different kids of logical and physical plan trees, while `TreeNodeContainer` can be used to simplify implementation of `apply_children` / `map_children()` as children are siblings.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848074244
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -81,7 +79,7 @@ impl TreeNode for LogicalPlan {
expr,
input,
schema,
- }) => rewrite_arc(input, f)?.update_data(|input| {
+ }) => input.map_elements(f)?.update_data(|input| {
Review Comment:
> IIUC map_children defines how LogicalPlan's construct a tree topology, while map_elements is resolving the link between nodes.
Yes, the container trait supposes that the elements in it are sibling nodes. Basically this is why we can use it when we implement `apply_children()` / `map_children()`. The blanket implementations for the trait makes it very simple to use with `Expr` and `LogicalPlan` as those logical plan treenodes define their children as a composition of `Vec`s, `Option`s, tuples, `Box`es and `Arc`s.
Here in `LogicalPlan` a treenode has mostly only 1 child (except for `Join` and `Union` maybe) so input (`Arc<LogicalPlan>`) is just a single element container so this PR doesn't have much benefit, but I think it is worth to allign `Expr` and `LogicalPlan` code.
`TreeNode::map_children` and `TreeNodeContainer:: map_elements` are similar in a way that both apply `f` to their children / elements of a `TreeNode` / `TreeNodeContainer`. But we use `TreeNode` to define different kids of logical
and physical plan trees, while `TreeNodeContainer` can be used to simplify implementation of `apply_children` / `map_children()` as children are siblings.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1847754432
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
Review Comment:
Can we somehow simplify these declarations by aliasing like
`type ApplyFn<'a, T> = dyn FnMut(&'a T) -> Result<TreeNodeRecursion>;`
`type MapFn<T> = dyn FnMut(T) -> Result<Transformed<T>>;`
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>>;
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ (*self).map_elements(f)?.map_data(|c| Ok(Self::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T> + Clone> Container<'a, T> for Arc<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ self.as_ref().apply_elements(f)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ Arc::unwrap_or_clone(self)
+ .map_elements(f)?
+ .map_data(|c| Ok(Arc::new(c)))
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Option<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion> {
+ match self {
+ Some(t) => t.apply_elements(f),
+ None => Ok(TreeNodeRecursion::Continue),
+ }
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ f: F,
+ ) -> Result<Transformed<Self>> {
+ self.map_or(Ok(Transformed::no(None)), |c| {
+ c.map_elements(f)?.map_data(|c| Ok(Some(c)))
+ })
+ }
+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Vec<C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ mut f: F,
+ ) -> Result<TreeNodeRecursion> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ for c in self {
+ tnr = c.apply_elements(&mut f)?;
+ match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
+ TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
+ }
+ }
+ Ok(tnr)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ let mut transformed = false;
+ self.into_iter()
+ .map(|c| match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {
+ c.map_elements(&mut f).map(|result| {
+ tnr = result.tnr;
+ transformed |= result.transformed;
+ result.data
+ })
+ }
+ TreeNodeRecursion::Stop => Ok(c),
+ })
+ .collect::<Result<Vec<_>>>()
+ .map(|data| Transformed::new(data, transformed, tnr))
+ }
+}
+
+impl<'a, T: 'a, K: Eq + Hash, C: Container<'a, T>> Container<'a, T> for HashMap<K, C> {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ mut f: F,
+ ) -> Result<TreeNodeRecursion> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ for c in self.values() {
+ tnr = c.apply_elements(&mut f)?;
+ match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {}
+ TreeNodeRecursion::Stop => return Ok(TreeNodeRecursion::Stop),
+ }
+ }
+ Ok(tnr)
+ }
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
+ self,
+ mut f: F,
+ ) -> Result<Transformed<Self>> {
+ let mut tnr = TreeNodeRecursion::Continue;
+ let mut transformed = false;
+ self.into_iter()
+ .map(|(k, c)| match tnr {
+ TreeNodeRecursion::Continue | TreeNodeRecursion::Jump => {
+ c.map_elements(&mut f).map(|result| {
+ tnr = result.tnr;
+ transformed |= result.transformed;
+ (k, result.data)
+ })
+ }
+ TreeNodeRecursion::Stop => Ok((k, c)),
+ })
+ .collect::<Result<HashMap<_, _>>>()
+ .map(|data| Transformed::new(data, transformed, tnr))
+ }
+}
+
+impl<'a, T: 'a, C0: Container<'a, T>, C1: Container<'a, T>> Container<'a, T>
Review Comment:
I agree with @alamb. We can provide them when they are really necessary -- AFAIK there is not any tuple-based trees in DF.
##########
datafusion/expr/src/logical_plan/tree_node.rs:
##########
@@ -81,7 +79,7 @@ impl TreeNode for LogicalPlan {
expr,
input,
schema,
- }) => rewrite_arc(input, f)?.update_data(|input| {
+ }) => input.map_elements(f)?.update_data(|input| {
Review Comment:
`map_children` also does not apply the f to input. IIUC `map_children` defines how LogicalPlan's construct a tree topology, while `map_elements` is resolving the link between nodes.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13467:
URL: https://github.com/apache/datafusion/pull/13467#discussion_r1848396994
##########
datafusion/common/src/tree_node.rs:
##########
@@ -769,6 +770,263 @@ impl<T> Transformed<T> {
}
}
+/// [`Container`] contains elements that a function can be applied on or mapped. The
+/// elements of the container are siblings so the continuation rules are similar to
+/// [`TreeNodeRecursion::visit_sibling`] / [`Transformed::transform_sibling`].
+pub trait Container<'a, T: 'a>: Sized {
+ fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(
+ &'a self,
+ f: F,
+ ) -> Result<TreeNodeRecursion>;
+
+ fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(
Review Comment:
Sure, I added comments in https://github.com/apache/datafusion/pull/13467/commits/2097bc7603097f4106d68be9f1b6a281d4bdbb7e to clarify how these methods are used.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org