gene-bordegaray commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3719496719
##########
datafusion/physical-plan/src/async_func.rs:
##########
Review Comment:
why do we just continue here and not apply on these?
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -165,6 +165,55 @@ where string types are preferred (`UNION`, `CASE
THEN/ELSE`, `NVL2`).
string-preferring behavior
- Crates that call `get_coerce_type_for_case_expression`
+### `ExecutionPlan::apply_expressions` is now a required method
Review Comment:
I think this should b in the 55.0.0 release
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -1337,6 +1338,20 @@ impl ExecutionPlan for RepartitionExec {
vec![&self.input]
}
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ match self.partitioning() {
+ Partitioning::Hash(exprs, _) =>
crate::apply_expression_roots(exprs, f),
Review Comment:
We do the apply_expression_roots here for the stored partitioning. But then
in something like the `StreamingTableExec` we drop certain expressions, what
are the exact rules around this?
##########
datafusion-examples/examples/relation_planner/table_sample.rs:
##########
@@ -749,6 +749,22 @@ impl ExecutionPlan for SampleExec {
Ok(Arc::new(stats))
}
+
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(
+ &Arc<dyn datafusion::physical_plan::PhysicalExpr>,
+ ) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ datafusion::physical_plan::apply_expression_roots(
+ self.cache
+ .output_ordering()
+ .into_iter()
+ .flatten()
+ .map(|sort_expr| &sort_expr.expr),
+ f,
+ )
Review Comment:
I think that would be too much indirection IMO, others might have some
opinions though
##########
datafusion/physical-plan/src/streaming.rs:
##########
Review Comment:
should we be applying expressions here too since they are owned by the
operator?
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -763,6 +764,32 @@ struct AggrDynFilter {
supported_accumulators_info: Vec<PerAccumulatorDynFilter>,
}
+fn plan_contains_expression_id(
+ plan: &Arc<dyn ExecutionPlan>,
+ expression_id: u64,
+) -> Result<bool> {
+ let mut found = false;
+ plan.apply(|node| {
+ node.apply_expressions(&mut |root| {
+ root.apply(|expr| {
Review Comment:
is there any way to eliminate this triple nesting we do.
From my understanding its:
1. walking the execution plan tree
2. Walk the expressions roots in one plan node
3. Walks inside the one expression
Maybe something like a `apply_plan_expressions` would make this easier to
read?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
