asolimando commented on code in PR #21122:
URL: https://github.com/apache/datafusion/pull/21122#discussion_r3269047077
##########
datafusion/physical-expr/src/projection.rs:
##########
@@ -125,12 +126,22 @@ impl From<ProjectionExpr> for (Arc<dyn PhysicalExpr>,
String) {
///
/// See [`ProjectionExprs::from_indices`] to select a subset of columns by
/// indices.
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone)]
pub struct ProjectionExprs {
/// [`Arc`] used for a cheap clone, which improves physical plan
optimization performance.
exprs: Arc<[ProjectionExpr]>,
+ /// Optional expression analyzer registry for statistics estimation
Review Comment:
I totally agree, since the design proposed in
https://github.com/apache/datafusion/issues/21443
(`StatisticsRegistry`, working at the relational operator level) and
https://github.com/apache/datafusion/issues/21120 (`ExpressionAnalyzer`,
working at the expression level) felt like a big architectural change, I
decided to start small with a fully opt-in design, which imposed temporary
suboptimal decisions like the one discussed here of embedding
`ExpressionAnalyzer` in the operators, and re-walking the plan to inject it.
I was waiting for https://github.com/apache/datafusion/pull/21815 to land,
hoping I could piggy back on that breaking change and propose something similar
to the `StatisticsArgs` you suggested, since we are actively working towards
that goal, I am happy to clean up and simplify the integration of both
`ExpressionAnalyzer` and `StatisticsRegistry` (merged via
https://github.com/apache/datafusion/pull/21483)
--
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]
