Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods #13467

peter-toth · 2024-11-18T15:38:27Z

Which issue does this PR close?

Part of #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 arbitrary sibling elements that functions can be applied on and mapped:

/// [`TreeNodeContainer`] 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 TreeNodeContainer<'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>>;
}

As example for the new trait usage, this is how Expr::map_children() handled Expr::Case before this PR:

Expr::Case(Case {
    expr,           // Option<Box<Expr>>,
    when_then_expr, // Vec<(Box<Expr>, Box<Expr>)>
    else_expr,      // Option<Box<Expr>>,
}) => map_until_stop_and_collect!(
    transform_option_box(expr, &mut f),
    when_then_expr,
    when_then_expr
        .into_iter()
        .map_until_stop_and_collect(|(when, then)| {
            map_until_stop_and_collect!(
                transform_box(when, &mut f),
                then,
                transform_box(then, &mut f)
            )
        }),
        else_expr,
        transform_option_box(else_expr, &mut f)
)?...

and this is how it is handled with this PR:

Expr::Case(Case {
    expr,           // Option<Box<Expr>>,
    when_then_expr, // Vec<(Box<Expr>, Box<Expr>)>
    else_expr,      // Option<Box<Expr>>,
}) => (expr, when_then_expr, else_expr).map_elements(f)?...

Please see the type of the Case struct fields in comments.
Let's focus on when_then_expr: Vec<(Box<Expr>, Box<Expr>)> first. The composable nature of TreeNodeContainer and the blanket implementation we provide for Box, 2-tuple and Vec makes it possible to just call when_then_expr. map_elements(f) to map all the expressions in when_then_expr.
But we can go further and replace the map_until_stop_and_collect macro to deal with all 3 fields of Case. We can put them into a 3-tuple and can call .map_elements() as we have blanket implementation for 3-tuple as well. (Note that we can't use Vec for this as the type of the 3 fields differ.)

What changes are included in this PR?

This PR:

Gets rid of map_until_stop_and_collect macro and many transform and rewrite helper methods.
Adds the TreeNodeContainer trait and blanket implementations for Box, Option, Vec, tuples, ...
Defines the trivial TreeNodeContainer 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.

…til_stop_and_collect` macro, simplify apply and map logic with `Container`s where possible

findepi · 2024-11-18T17:16:28Z

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?

peter-toth · 2024-11-18T20:32:02Z

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.

peter-toth · 2024-11-18T20:32:52Z

cc @alamb, @berkaysynnada

alamb

Thank you @peter-toth - I think this looks like a really nice improvement in the code 🙏

I have some comment suggestions, but nothing that is required.

The only thing I want to do prior to approving this PR is to run the planning benchmarks and make sure we didn't introduce any regressions. I am running them now.

alamb · 2024-11-18T21:15:07Z

datafusion/common/src/tree_node.rs

+        f: F,
+    ) -> Result<TreeNodeRecursion>;
+
+    fn map_elements<F: FnMut(T) -> Result<Transformed<T>>>(


Could we also add some documentation here about what map_elements is (perhaps just a map back to TreeNode::map_children?)

Sure, I added comments in 2097bc7 to clarify how these methods are used.

alamb · 2024-11-18T21:15:24Z

datafusion/common/src/tree_node.rs

+/// 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>>(


Could we also add some documentation here about what apply_elements is (perhaps just a map back to TreeNode::apply?)

alamb · 2024-11-18T21:16:09Z

datafusion/common/src/tree_node.rs

+        let mut tnr = TreeNodeRecursion::Continue;
+        for c in self {
+            tnr = c.apply_elements(&mut f)?;
+            match tnr {


this is definitely a much easier to understand formulation

alamb · 2024-11-18T21:18:58Z

datafusion/common/src/tree_node.rs

+    }
+}
+
+impl<'a, T: 'a, C0: Container<'a, T>, C1: Container<'a, T>> Container<'a, T>


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 structs. But that is just a personal style preference

I agree with @alamb. We can provide them when they are really necessary -- AFAIK there is not any tuple-based trees in DF.

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.

alamb · 2024-11-18T21:20:54Z

datafusion/common/src/tree_node.rs

+/// [`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 {


I think it is a matter of preference, but what would you think about calling this something less generic, such as TreeNodeContainer ?

Yeah, indeed that's a better name for container. Let me change it.

Renamed in e5aff72.

alamb · 2024-11-18T21:21:21Z

datafusion/expr/src/logical_plan/ddl.rs

+        self.function_body.apply_elements(f)
+    }
+
+    fn map_elements<F: FnMut(Expr) -> Result<Transformed<Expr>>>(


this is very cool

alamb · 2024-11-18T21:22:58Z

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| {


Is the difference between map_children and map_elements that map_elements doesn't also apply the function to input ?

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.

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 Vecs, Options, tuples, Boxes and Arcs.

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.

I clarified this in 2097bc7.

alamb · 2024-11-18T21:23:47Z

datafusion/expr/src/logical_plan/tree_node.rs

            // 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...


🎉 for removing the clone

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...

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?

alamb · 2024-11-18T21:24:21Z

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),


it is nice to avoid creating vecs here as well

berkaysynnada

LGTM. Very nice clean-up @peter-toth, thank you. I've just one minor suggestion.

berkaysynnada · 2024-11-19T07:12:37Z

datafusion/common/src/tree_node.rs

+}
+
+impl<'a, T: 'a, C: Container<'a, T>> Container<'a, T> for Box<C> {
+    fn apply_elements<F: FnMut(&'a T) -> Result<TreeNodeRecursion>>(


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>>;

Good idea, I will fix it soon.

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.

berkaysynnada · 2024-11-19T07:18:27Z

datafusion/common/src/tree_node.rs

+    }
+}
+
+impl<'a, T: 'a, C0: Container<'a, T>, C1: Container<'a, T>> Container<'a, T>


I agree with @alamb. We can provide them when they are really necessary -- AFAIK there is not any tuple-based trees in DF.

berkaysynnada · 2024-11-19T07:35:02Z

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| {


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.

alamb

I ran the benchmarks

Details

++ critcmp main containers
group                                         containers                             main
-----                                         ----------                             ----
logical_aggregate_with_join                   1.01  1498.8±18.08µs        ? ?/sec    1.00  1488.6±12.63µs        ? ?/sec
logical_select_all_from_1000                  1.01      5.3±0.03ms        ? ?/sec    1.00      5.3±0.03ms        ? ?/sec
logical_select_one_from_700                   1.01  1189.8±13.27µs        ? ?/sec    1.00  1178.1±17.27µs        ? ?/sec
logical_trivial_join_high_numbered_columns    1.00  1160.8±17.11µs        ? ?/sec    1.00  1160.6±37.41µs        ? ?/sec
logical_trivial_join_low_numbered_columns     1.00  1138.4±12.08µs        ? ?/sec    1.00  1142.6±17.28µs        ? ?/sec
physical_intersection                         1.01      2.5±0.02ms        ? ?/sec    1.00      2.4±0.02ms        ? ?/sec
physical_join_consider_sort                   1.01      3.3±0.02ms        ? ?/sec    1.00      3.3±0.02ms        ? ?/sec
physical_join_distinct                        1.00  1126.9±15.23µs        ? ?/sec    1.02  1146.2±18.69µs        ? ?/sec
physical_many_self_joins                      1.03     17.6±0.11ms        ? ?/sec    1.00     17.2±0.20ms        ? ?/sec
physical_plan_clickbench_all                  1.00    228.6±1.85ms        ? ?/sec    1.00    229.6±1.93ms        ? ?/sec
physical_plan_clickbench_q1                   1.00      3.3±0.05ms        ? ?/sec    1.00      3.3±0.03ms        ? ?/sec
physical_plan_clickbench_q10                  1.00      4.4±0.05ms        ? ?/sec    1.02      4.5±0.11ms        ? ?/sec
physical_plan_clickbench_q11                  1.00      4.5±0.09ms        ? ?/sec    1.01      4.6±0.09ms        ? ?/sec
physical_plan_clickbench_q12                  1.00      4.7±0.06ms        ? ?/sec    1.01      4.7±0.08ms        ? ?/sec
physical_plan_clickbench_q13                  1.00      4.2±0.05ms        ? ?/sec    1.01      4.3±0.07ms        ? ?/sec
physical_plan_clickbench_q14                  1.00      4.5±0.07ms        ? ?/sec    1.02      4.6±0.10ms        ? ?/sec
physical_plan_clickbench_q15                  1.00      4.4±0.06ms        ? ?/sec    1.04      4.5±0.10ms        ? ?/sec
physical_plan_clickbench_q16                  1.00      3.8±0.05ms        ? ?/sec    1.00      3.8±0.07ms        ? ?/sec
physical_plan_clickbench_q17                  1.00      3.9±0.06ms        ? ?/sec    1.00      3.9±0.07ms        ? ?/sec
physical_plan_clickbench_q18                  1.00      3.6±0.04ms        ? ?/sec    1.00      3.6±0.06ms        ? ?/sec
physical_plan_clickbench_q19                  1.00      4.6±0.08ms        ? ?/sec    1.00      4.6±0.09ms        ? ?/sec
physical_plan_clickbench_q2                   1.00      3.6±0.05ms        ? ?/sec    1.00      3.6±0.05ms        ? ?/sec
physical_plan_clickbench_q20                  1.00      3.4±0.05ms        ? ?/sec    1.00      3.4±0.06ms        ? ?/sec
physical_plan_clickbench_q21                  1.00      3.6±0.05ms        ? ?/sec    1.01      3.6±0.06ms        ? ?/sec
physical_plan_clickbench_q22                  1.00      4.6±0.05ms        ? ?/sec    1.01      4.7±0.09ms        ? ?/sec
physical_plan_clickbench_q23                  1.00      5.1±0.07ms        ? ?/sec    1.00      5.1±0.08ms        ? ?/sec
physical_plan_clickbench_q24                  1.00      5.8±0.08ms        ? ?/sec    1.01      5.9±0.12ms        ? ?/sec
physical_plan_clickbench_q25                  1.00      4.0±0.06ms        ? ?/sec    1.01      4.0±0.07ms        ? ?/sec
physical_plan_clickbench_q26                  1.00      3.7±0.08ms        ? ?/sec    1.00      3.7±0.05ms        ? ?/sec
physical_plan_clickbench_q27                  1.00      4.0±0.06ms        ? ?/sec    1.01      4.0±0.05ms        ? ?/sec
physical_plan_clickbench_q28                  1.00      4.8±0.07ms        ? ?/sec    1.01      4.8±0.08ms        ? ?/sec
physical_plan_clickbench_q29                  1.00      5.9±0.09ms        ? ?/sec    1.00      5.9±0.09ms        ? ?/sec
physical_plan_clickbench_q3                   1.00      3.5±0.05ms        ? ?/sec    1.00      3.5±0.05ms        ? ?/sec
physical_plan_clickbench_q30                  1.00     16.9±0.20ms        ? ?/sec    1.02     17.2±0.18ms        ? ?/sec
physical_plan_clickbench_q31                  1.00      4.9±0.06ms        ? ?/sec    1.01      4.9±0.08ms        ? ?/sec
physical_plan_clickbench_q32                  1.00      4.9±0.08ms        ? ?/sec    1.00      4.9±0.08ms        ? ?/sec
physical_plan_clickbench_q33                  1.00      4.4±0.06ms        ? ?/sec    1.00      4.4±0.07ms        ? ?/sec
physical_plan_clickbench_q34                  1.00      3.9±0.05ms        ? ?/sec    1.01      4.0±0.07ms        ? ?/sec
physical_plan_clickbench_q35                  1.00      4.1±0.10ms        ? ?/sec    1.00      4.1±0.07ms        ? ?/sec
physical_plan_clickbench_q36                  1.00      5.3±0.08ms        ? ?/sec    1.00      5.3±0.09ms        ? ?/sec
physical_plan_clickbench_q37                  1.00      5.3±0.09ms        ? ?/sec    1.01      5.4±0.11ms        ? ?/sec
physical_plan_clickbench_q38                  1.01      5.4±0.14ms        ? ?/sec    1.00      5.3±0.09ms        ? ?/sec
physical_plan_clickbench_q39                  1.00      4.8±0.07ms        ? ?/sec    1.01      4.9±0.08ms        ? ?/sec
physical_plan_clickbench_q4                   1.00      3.3±0.05ms        ? ?/sec    1.00      3.3±0.06ms        ? ?/sec
physical_plan_clickbench_q40                  1.02      5.6±0.26ms        ? ?/sec    1.00      5.5±0.11ms        ? ?/sec
physical_plan_clickbench_q41                  1.00      5.2±0.10ms        ? ?/sec    1.00      5.2±0.06ms        ? ?/sec
physical_plan_clickbench_q42                  1.00      5.0±0.06ms        ? ?/sec    1.00      5.0±0.07ms        ? ?/sec
physical_plan_clickbench_q43                  1.00      5.1±0.09ms        ? ?/sec    1.00      5.1±0.10ms        ? ?/sec
physical_plan_clickbench_q44                  1.01      3.5±0.06ms        ? ?/sec    1.00      3.5±0.05ms        ? ?/sec
physical_plan_clickbench_q45                  1.00      3.5±0.05ms        ? ?/sec    1.00      3.5±0.06ms        ? ?/sec
physical_plan_clickbench_q46                  1.00      4.1±0.08ms        ? ?/sec    1.00      4.1±0.05ms        ? ?/sec
physical_plan_clickbench_q47                  1.00      4.8±0.08ms        ? ?/sec    1.00      4.8±0.06ms        ? ?/sec
physical_plan_clickbench_q48                  1.00      5.4±0.08ms        ? ?/sec    1.00      5.4±0.10ms        ? ?/sec
physical_plan_clickbench_q49                  1.00      5.7±0.08ms        ? ?/sec    1.00      5.7±0.10ms        ? ?/sec
physical_plan_clickbench_q5                   1.00      3.6±0.05ms        ? ?/sec    1.00      3.5±0.06ms        ? ?/sec
physical_plan_clickbench_q6                   1.01      3.6±0.05ms        ? ?/sec    1.00      3.6±0.04ms        ? ?/sec
physical_plan_clickbench_q7                   1.00      4.1±0.05ms        ? ?/sec    1.00      4.1±0.08ms        ? ?/sec
physical_plan_clickbench_q8                   1.00      3.8±0.06ms        ? ?/sec    1.00      3.8±0.07ms        ? ?/sec
physical_plan_clickbench_q9                   1.00      4.2±0.07ms        ? ?/sec    1.01      4.2±0.09ms        ? ?/sec
physical_plan_tpcds_all                       1.00   1361.4±2.60ms        ? ?/sec    1.02  1383.3±11.34ms        ? ?/sec
physical_plan_tpch_all                        1.00     89.2±0.45ms        ? ?/sec    1.01     90.2±0.49ms        ? ?/sec
physical_plan_tpch_q1                         1.00      3.2±0.02ms        ? ?/sec    1.01      3.2±0.02ms        ? ?/sec
physical_plan_tpch_q10                        1.00      4.3±0.03ms        ? ?/sec    1.01      4.4±0.04ms        ? ?/sec
physical_plan_tpch_q11                        1.00      3.9±0.03ms        ? ?/sec    1.01      4.0±0.03ms        ? ?/sec
physical_plan_tpch_q12                        1.00      3.1±0.05ms        ? ?/sec    1.02      3.2±0.04ms        ? ?/sec
physical_plan_tpch_q13                        1.00      2.4±0.02ms        ? ?/sec    1.01      2.5±0.02ms        ? ?/sec
physical_plan_tpch_q14                        1.00      2.8±0.02ms        ? ?/sec    1.01      2.8±0.02ms        ? ?/sec
physical_plan_tpch_q16                        1.00      3.9±0.03ms        ? ?/sec    1.02      4.0±0.05ms        ? ?/sec
physical_plan_tpch_q17                        1.00      3.7±0.14ms        ? ?/sec    1.00      3.7±0.03ms        ? ?/sec
physical_plan_tpch_q18                        1.00      4.1±0.02ms        ? ?/sec    1.01      4.1±0.03ms        ? ?/sec
physical_plan_tpch_q19                        1.00      5.9±0.03ms        ? ?/sec    1.02      6.1±0.05ms        ? ?/sec
physical_plan_tpch_q2                         1.00      7.3±0.05ms        ? ?/sec    1.02      7.4±0.06ms        ? ?/sec
physical_plan_tpch_q20                        1.00      4.7±0.04ms        ? ?/sec    1.02      4.8±0.04ms        ? ?/sec
physical_plan_tpch_q21                        1.00      6.0±0.05ms        ? ?/sec    1.02      6.1±0.05ms        ? ?/sec
physical_plan_tpch_q22                        1.00      3.6±0.03ms        ? ?/sec    1.02      3.7±0.03ms        ? ?/sec
physical_plan_tpch_q3                         1.00      3.2±0.02ms        ? ?/sec    1.01      3.3±0.03ms        ? ?/sec
physical_plan_tpch_q4                         1.00      2.6±0.02ms        ? ?/sec    1.01      2.6±0.02ms        ? ?/sec
physical_plan_tpch_q5                         1.00      4.4±0.02ms        ? ?/sec    1.02      4.4±0.05ms        ? ?/sec
physical_plan_tpch_q6                         1.00  1858.8±17.14µs        ? ?/sec    1.01  1881.0±17.95µs        ? ?/sec
physical_plan_tpch_q7                         1.00      5.7±0.04ms        ? ?/sec    1.01      5.7±0.04ms        ? ?/sec
physical_plan_tpch_q8                         1.00      6.7±0.04ms        ? ?/sec    1.01      6.8±0.07ms        ? ?/sec
physical_plan_tpch_q9                         1.00      5.3±0.06ms        ? ?/sec    1.01      5.3±0.06ms        ? ?/sec
physical_select_aggregates_from_200           1.00     26.7±0.13ms        ? ?/sec    1.05     28.0±0.19ms        ? ?/sec
physical_select_all_from_1000                 1.01     41.4±0.15ms        ? ?/sec    1.00     41.1±0.17ms        ? ?/sec
physical_select_one_from_700                  1.02      3.4±0.02ms        ? ?/sec    1.00      3.3±0.04ms        ? ?/sec
physical_theta_join_consider_sort             1.01      3.7±0.02ms        ? ?/sec    1.00      3.7±0.02ms        ? ?/sec
physical_unnest_to_join                       1.01      3.4±0.02ms        ? ?/sec    1.00      3.3±0.02ms        ? ?/sec
with_param_values_many_columns                1.00    159.4±1.06µs        ? ?/sec    1.05    168.0±0.96µs        ? ?/sec

And my conclusion is that this branch shows no significant difference in planning performance.

Thanks @peter-toth and @berkaysynnada

peter-toth · 2024-11-19T14:04:14Z

~~Let me check a few more things before we merge this PR. There might be a way to make this even simpler.~~

NVM.

peter-toth · 2024-11-20T08:57:12Z

Thanks for the review @alamb , @berkaysynnada , @findepi!

Add Container trait and its blanket implementations, remove `map_un…

2036c09

…til_stop_and_collect` macro, simplify apply and map logic with `Container`s where possible

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules common Related to common crate labels Nov 18, 2024

fix clippy

166e5e5

alamb reviewed Nov 18, 2024

View reviewed changes

berkaysynnada approved these changes Nov 19, 2024

View reviewed changes

alamb approved these changes Nov 19, 2024

View reviewed changes

peter-toth added 3 commits November 19, 2024 13:24

rename Container to TreeNodeContainer

e5aff72

add docs to containers

2097bc7

clarify when we need a temporary TreeNodeRefContainer

e9b470a

code and docs cleanup

28e29a8

berkaysynnada merged commit aef232b into apache:main Nov 20, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods #13467

Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods #13467

peter-toth commented Nov 18, 2024 •

edited

Loading

findepi commented Nov 18, 2024

peter-toth commented Nov 18, 2024

peter-toth commented Nov 18, 2024 •

edited

Loading

alamb left a comment

alamb Nov 18, 2024

peter-toth Nov 19, 2024

alamb Nov 18, 2024

alamb Nov 18, 2024

alamb Nov 18, 2024

berkaysynnada Nov 19, 2024

peter-toth Nov 19, 2024 •

edited

Loading

alamb Nov 18, 2024

peter-toth Nov 19, 2024

peter-toth Nov 19, 2024

alamb Nov 18, 2024

alamb Nov 18, 2024

berkaysynnada Nov 19, 2024

peter-toth Nov 19, 2024 •

edited

Loading

peter-toth Nov 19, 2024

alamb Nov 18, 2024

peter-toth Nov 19, 2024

peter-toth Nov 19, 2024

alamb Nov 18, 2024

berkaysynnada left a comment

berkaysynnada Nov 19, 2024

peter-toth Nov 19, 2024

peter-toth Nov 19, 2024 •

edited

Loading

berkaysynnada Nov 19, 2024

berkaysynnada Nov 19, 2024

alamb left a comment •

edited

Loading

peter-toth commented Nov 19, 2024 •

edited

Loading

peter-toth commented Nov 20, 2024

Add Container trait and to simplify Expr and LogicalPlan apply and map methods #13467

Add Container trait and to simplify Expr and LogicalPlan apply and map methods #13467

Conversation

peter-toth commented Nov 18, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

findepi commented Nov 18, 2024

peter-toth commented Nov 18, 2024

peter-toth commented Nov 18, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-toth Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-toth Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkaysynnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-toth Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment • edited Loading

Choose a reason for hiding this comment

peter-toth commented Nov 19, 2024 • edited Loading

peter-toth commented Nov 20, 2024

Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods #13467

Add `Container` trait and to simplify `Expr` and `LogicalPlan` apply and map methods #13467

peter-toth commented Nov 18, 2024 •

edited

Loading

peter-toth commented Nov 18, 2024 •

edited

Loading

peter-toth Nov 19, 2024 •

edited

Loading

peter-toth Nov 19, 2024 •

edited

Loading

peter-toth Nov 19, 2024 •

edited

Loading

alamb left a comment •

edited

Loading

peter-toth commented Nov 19, 2024 •

edited

Loading