Implement ExecutionPlan::map_expressions #20899

Activity

1. LiaCastaneda

   added

   enhancementNew feature or requestNew feature or request

   on Mar 12, 2026
2. LiaCastaneda

   added the

   Feature

   issue type on Mar 12, 2026
3. LiaCastaneda commented on Mar 13, 2026

   

   LiaCastaneda

   on Mar 13, 2026

   ContributorAuthor

   More actions

   Bringing some discussions from the apply_expressions PR:

   I think we can reuse the properties of the rest of the plan (avoiding ::new()), similar to how LogicalPlan::map_expressions does it.

   I created this issue #20899. I haven't started working on it yet and probably won't have much time this week, so I'll likely give it a try next week, but feel free to take it if you'd like'

   Actually, now that I think about it, there are some cases where we would need to recompute properties right? for example, if a user changes an expression from a > something to a < something. How do we specify in this API whether we want to recompute properties or not? should map_expressions have a recompute_properties: bool argument? 🤔

   From @askalt

   Yes, it may be useful to explicitly ask for properties re-computation. And it seems for me that by default the safest option is to force properties to be re-computed.

   Another way to satisfy it is to introduce "args struct" like:

   struct MapExpressionsArgs<'a> {
   f: &'a dyn FnMut(&Arc) -> Result<Arc>,
   preserve_properties: bool,
   }
   Like is done here:

   datafusion/datafusion/catalog/src/table.rs

   Lines 366 to 372 in 8d9b080

   /// Arguments for scanning a table with [TableProvider::scan_with_args].
   #[derive(Debug, Clone, Default)]
   pub struct ScanArgs<'a> {
   filters: Option<&'a [Expr]>,
   projection: Option<&'a [usize]>,
   limit: Option,
   }
   to not add a bool argument each time when the method semantics is extended. But maybe this is overkill here and bool parameter will be enough.
4. LiaCastaneda commented on Mar 13, 2026

   

   LiaCastaneda

   on Mar 13, 2026

   Last edited by LiaCastaneda

   ContributorAuthor

   More actions

   I think the only thing that feels weird to me is that implementors will receive preserve_properties and will have to do something different based on its value... 🤔

   if preserve_properties {
       recreate node using new::
   } else {
       recreate node not using new::
   }
   

   which adds boilerplate to every ExecutionPlan implementation since this method would be required.

   I see other execution plan nodes have separate functions for explicitly not recomputing properties, for example with_new_children_and_same_properties
5. askalt commented on Mar 13, 2026

   

   askalt

   on Mar 13, 2026

   Last edited by askalt

   Contributor

   More actions

   Implementing two methods with the similar semantic seems hard to maintain. What if we inject recompute properties behavior to the closure return type with a lightweight structure?

   pub enum RecomputePropertiesBehavior {
       Preserve,
       Recompute,
   }
   
   pub struct MappedExpr {
       pub properties_behavior: RecomputePropertiesBehavior,
       pub expr: Arc<dyn PhysicalExpr>,
   }
   
   pub trait ExecutionPlan {
      ...
   
       fn map_expressions(
           self: Arc<Self>,
           f: &dyn FnMut(Arc<dyn PhysicalExpr>) -> Result<Transformed<MappedExpr>>,
       ) -> Result<Transformed<Arc<dyn ExecutionPlan>>>;
   }

   And then if at least one returned expression has RecomputePropertiesBehavior::Recompute -- the implementation will recompute them and leave as is otherwise.
6. LiaCastaneda commented on Mar 15, 2026

   

   LiaCastaneda

   on Mar 15, 2026

   ContributorAuthor

   More actions

   Implementing two methods with the similar semantic seems hard to maintain. What if we inject recompute properties behavior to the closure return type with a lightweight structure?

   mmm i think one limitation would be that the Recompute path assumes each ExecutionPlan implementor knows how to rebuild their own plan properties. For native nodes this might be fine since compute_properties is called internally, but for people implementing a custom ExecutionPlan, there is no standard public API to say "recompute my properties given my current state." They would have to call either their own ::new() constructor (which may do more than just recompute properties) or manually construct PlanProperties . Maybe it makes semse to make recompute_properties part of the executionPlan trait (probably optional to implement)?
7. askalt commented on Mar 17, 2026

   

   askalt

   on Mar 17, 2026

   Contributor

   More actions

   Maybe it makes semse to make recompute_properties part of the executionPlan trait (probably optional to implement)?

   Do not fully understand how this will help map_expressions implementation to recompute them. Each implementation could have its own PlanStruct::recompute_properties and call it (it's not necessary to make it as the trait method).

   There is an alternative, add ExecutionPlan::recompute_properties and force to implement it and map_expressions_preserve_properties. Then we can define:

   pub fn map_expressions(...) -> Result<Arc<dyn ExecutionPlan>> {
          let mapped = self.map_expressions_preserve_properties(...);
          mapped.recompute_properties()
   }

   Please note that recompute_properties currently cannot be expressed as just plan.with_new_children(plan.children()), as there is an optimization that does not recompute properties if children properties stay the same.

   👍React with 👍1Reacted by Lía Adriana
8. LiaCastaneda commented on Mar 17, 2026

   

   LiaCastaneda

   on Mar 17, 2026

   ContributorAuthor

   More actions

   Do not fully understand how this will help map_expressions implementation to recompute them. Each implementation could have its own PlanStruct::recompute_properties and call it (it's not necessary to make it as the trait method).

   I think it just gives users a uniform way to trigger property recomputation on any ExecutionPlan without knowing the concrete type, and avoids the need to downcast to specific nodes if we ever need to call it externally. I think you are right though, this could be a follow-up rather than a prerequisite. I wasn't aware that properties() is already part of the ExecutionPlan trait, which makes all of this more standard than I initially thought.
9. LiaCastaneda commented on Mar 17, 2026

   

   LiaCastaneda

   on Mar 17, 2026

   ContributorAuthor

   More actions

   made a rough draft here #20986 (mostly claude generated). I still need to review it in detail.

   👀React with 👀1Reacted by Albert Skalt