asolimando commented on code in PR #26271:
URL: https://github.com/apache/datafusion/pull/26271#discussion_r4311932284
##########
datafusion/physical-plan/src/materialized_cte.rs:
##########
@@ -801,19 +831,25 @@ impl ExecutionPlan for MaterializedCteScanExec {
Some(self.metrics.clone_inner())
}
- /// The statistics are unknown: a leaf cannot ask the body for its
- /// statistics through [`StatisticsArgs`].
+ /// Returns the statistics that the planner copied from the CTE body.
+ /// See [`Self::with_body_statistics`].
///
- /// This is a known limitation, and it can make a plan worse than the
- /// plan of the inlined CTE. For example, `JoinSelection` does not know
- /// that a scan is small, and can put the other, larger input of a hash
- /// join on the build side.
+ /// The statistics of one partition are known only when the scan has one
Review Comment:
We could again wait for https://github.com/apache/datafusion/issues/26424,
which will give us all the missing information to handle the case with multiple
partitions
##########
datafusion/physical-plan/src/materialized_cte.rs:
##########
@@ -663,6 +672,8 @@ pub struct MaterializedCteScanExec {
id: u64,
name: String,
schema: SchemaRef,
+ /// The statistics of all partitions of the body.
Review Comment:
Today we don't push-down filters, limit or other stats-affecting operators
down into CTEs (or, more probable, their scans), so copying the stats as-is is
safe, but if we introduce something like this [limit
pushdown](https://github.com/apache/datafusion/blob/19fa23e4cd56cc122c34faaccce848752085bf87/datafusion/datasource/src/memory.rs#L223-L230)
for `MemorySourceConfig` data source, that wouldn't be safe anymore, and I
have seen/fixed such cases elsewhere, very easy to miss.
So I wonder if we should state this invariant as a comment, in case someone
adds a similar pushdown later?
##########
datafusion/sqllogictest/test_files/cte_materialized.slt:
##########
@@ -93,24 +93,23 @@ SELECT k FROM c WHERE v2 < 30
# referenced three times, and each scan replays all 4 rows.
query TT
EXPLAIN ANALYZE WITH c AS MATERIALIZED (SELECT k, v FROM t)
-SELECT count(*) FROM (
+SELECT sum(k) FROM (
Review Comment:
Q: IIUC this query change is needed to avoid `AggregateStatistics` answering
the `count(*)` part with the `Exact` row count, right?
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -3340,7 +3408,11 @@ impl DefaultPhysicalPlanner {
/// Optimize a physical plan by applying each physical optimizer,
/// calling observer(plan, optimizer after each one)
- #[expect(clippy::needless_pass_by_value)]
+ ///
+ /// Before the optimizers run, each [`MaterializedCteScanExec`] is bound
+ /// to the buffer of its [`MaterializedCteExec`] and gets the statistics
+ /// of the CTE body. A rewrite of the body does not change its rows, so
Review Comment:
This is only strictly true for perfect statistics propagation (unhandled
cases today can turn stats into `Absent` for some rewrites) and for row count
at most, many column stats are not guaranteed to stay the same upon rewrites.
Consider a filter pushdown rewriting `Filter(Join(A,B))` into
`Join(Filter(A),B)`, the interval-derived min/max and NDV are very probably
going to be different.
It seems to me that we should optimize the body first, and then snapshot as
#26424 proposes for ordering/partitioning. Statistics are finally just another
metadata exactly like those and would benefit from the same treatment.
WDYT?
--
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]
