kosiew commented on code in PR #21122:
URL: https://github.com/apache/datafusion/pull/21122#discussion_r3050242932
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -2898,7 +2920,11 @@ impl DefaultPhysicalPlanner {
.into_iter()
.map(|(expr, alias)| ProjectionExpr { expr, alias })
.collect();
- Ok(Arc::new(ProjectionExec::try_new(proj_exprs, input_exec)?))
+ let mut proj_exec = ProjectionExec::try_new(proj_exprs,
input_exec)?;
Review Comment:
@asolimando
Thanks for the detailed explanation. I understand the non-breaking-change
constraint here, and the code comments on
`ProjectionExprs::with_expression_analyzer_registry` /
`FilterExecBuilder::with_expression_analyzer_registry` do make the current
boundary clearer. I still think this remains a blocking limitation for the PR
as it stands, though, because the feature is exposed behind
`datafusion.optimizer.enable_expression_analyzer = true` yet its effect still
depends on whether later physical rewrites rebuild the operator. Several common
paths still create fresh `ProjectionExec::try_new(...)` nodes without a
registry (`datafusion/physical-plan/src/projection.rs`,
`datafusion/physical-optimizer/src/projection_pushdown.rs`,
`datafusion/physical-optimizer/src/aggregate_statistics.rs`, etc.), so
expression stats revert to `unknown` after optimization.
If carrying the registry through optimizer-created operators is not
possible in this PR without the broader operator-level design from `#21443`, I
think the safer options would be either:
- narrow the current scope/docs so users do not read this as
generally-enabled projection/filter expression analysis across the final
physical plan, or
- treat the operator-level propagation work as required before we rely on
this config as a general optimizer feature.
I do not have a clean non-breaking propagation approach to suggest beyond
the operator-level registry direction you mentioned, but I wanted to be
explicit that I still see the current plan-shape dependency as an unresolved
product/correctness concern rather than just a documentation gap.
--
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]
