Repository navigation
feat: report statistics of materialized CTE scans - #26271
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26271 +/- ##
==========================================
+ Coverage 82.89% 82.92% +0.02%
==========================================
Files 1154 1155 +1
Lines 457630 458327 +697
Branches 457630 458327 +697
==========================================
+ Hits 379370 380055 +685
+ Misses 55256 55222 -34
- Partials 23004 23050 +46 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2273e33 to
f5ab4f7
Compare
Hmm not so sure about this. We should consider if this makes sense. |
f5ab4f7 to
a442b87
Compare
db48d4e to
19fa23e
Compare
asolimando
left a comment
There was a problem hiding this comment.
LGTM, it feels like landing #26424 would make our life easier on a few places (I left two comments), but nothing major/blocking.
Happy to see better CTE support landing, thanks for your work!
| id: u64, | ||
| name: String, | ||
| schema: SchemaRef, | ||
| /// The statistics of all partitions of the body. |
There was a problem hiding this 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 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?
| /// | ||
| /// 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 |
There was a problem hiding this 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?
| /// 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 |
There was a problem hiding this comment.
We could again wait for #26424, which will give us all the missing information to handle the case with multiple partitions
| query TT | ||
| EXPLAIN ANALYZE WITH c AS MATERIALIZED (SELECT k, v FROM t) | ||
| SELECT count(*) FROM ( | ||
| SELECT sum(k) FROM ( |
There was a problem hiding this comment.
Q: IIUC this query change is needed to avoid AggregateStatistics answering the count(*) part with the Exact row count, right?
A `MaterializedCteScanExec` is a leaf, and the CTE body is a child of `MaterializedCteExec`, so the scan reported unknown statistics. `JoinSelection` could then build a hash table on a large table and not on a small CTE. `optimize_physical_plan` now binds the scans and gives each scan the statistics of the CTE body, before the optimizer runs. A body that reads another CTE gets the statistics of that CTE first. A rewrite of the body does not change its rows, so these statistics stay correct. The EXPLAIN paths use the same function, so they show the same plan. Closes apache#26230.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
a95795d to
d01f5cf
Compare
| where | ||
| F: FnMut(&dyn ExecutionPlan, &dyn PhysicalOptimizerRule), | ||
| { | ||
| let plan = bind_materialized_cte_scans(plan, session_state)?; |
There was a problem hiding this comment.
Since the scans now carry exact statistics, AggregateStatistics often folds a count(*) over a scan into a constant and drops the scan. That scan was already bound here, though, and bind pushed its partition count into scan_partitions. So the replay reservation still includes readers that never run. With target_partitions = 1 and memory_limit = '300K', this fails while asking for 576 KB, nine scans' worth, even though only the sum scan is left:
WITH g AS MATERIALIZED (SELECT value AS x FROM generate_series(1, 200000))
SELECT count(*) FROM g UNION ALL SELECT count(*) FROM g UNION ALL SELECT count(*) FROM g
UNION ALL SELECT count(*) FROM g UNION ALL SELECT count(*) FROM g UNION ALL SELECT count(*) FROM g
UNION ALL SELECT count(*) FROM g UNION ALL SELECT count(*) FROM g UNION ALL SELECT sum(x) FROM g;Main reserves the same amount for this query, so it isn't a regression, and a follow-up is fine. Could the partition counts be registered from the scans in the final plan instead, for example after the optimizers or when MaterializedCteExec::execute sets the body?
|
@asolimando Thank you for the review. I agree with your point about the statistics. I propose this:
This way, the scan always reports the correct properties of the body. #26428 does not need its check and its fallback. The extra planning time is small, about 0.04 ms for one CTE (measured with #26428). I will keep the statistics and the properties in separate commits. Do you agree? |
|
Merging to keep things moving. Thank you everyone |
|
@alamb i think there’s still some discussion to be had here. merging now might cause some churn. i’m happy to handle it that way, up to you. this is all new / unstable code so im not too worried either way. |
Sorry -- I clicked the button. Should we revert this? I am trying to work through our PR backlog but I maybe was overzealous |
|
no worries, i think it’s fine. it only impacts people who explicitly upped to materialize CTE, which is a new unstable feature. |
…27005) ## Which issue does this PR close? - Related to apache#26424 and apache#26428. No issue of its own: this PR only adds benchmarks. ## Rationale for this change `MATERIALIZED` CTEs (apache#26271) are planned differently from inlined CTEs: each body is optimized on its own before the rest of the plan. apache#26428 adds more work to that step. No planner benchmark plans a materialized CTE today, so there is no standard way to see what this costs, or to compare it with inlining. ## What changes are included in this PR? New `sql_planner` cases, which plan a query from SQL to a physical plan. The PostgreSQL dialect is used, because sqlparser only parses `[NOT] MATERIALIZED` for that dialect. For each kind of CTE (unmarked, `MATERIALIZED`, `NOT MATERIALIZED`) there are three cases: - `physical_cte_<kind>_one_ref`: the CTE is read once. - `physical_cte_<kind>_two_refs`: the CTE is joined with itself. - `physical_cte_<kind>_chain_6`: six CTEs, each joining the previous one with itself. `physical_cte_mixed` has one CTE of each kind, each read twice. Results on `main` (Criterion means): | Case | unmarked | `MATERIALIZED` | `NOT MATERIALIZED` | | --- | --- | --- | --- | | one reference | 242 µs | 264 µs | 245 µs | | two references | 474 µs | 360 µs | 481 µs | | chain of 6 | 8.25 ms | 0.92 ms | 8.33 ms | `physical_cte_mixed` takes 1.36 ms. An inlined chain copies the first CTE 32 times. So it takes about 9 times as long to plan as the materialized chain. ## What is the testing strategy for this PR? This PR has only benchmarks. They run with: ```bash cargo bench -p datafusion --bench sql_planner -- physical_cte ``` The `sql_planner` benchmark needs ClickBench data. The checked-in fixture works: put `datafusion/core/tests/data/clickbench_hits_10.parquet` in a directory, and set `CLICKBENCH_DATA_PATH` to that directory. ## Are there any user-facing changes? No. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
Which issue does this PR close?
Rationale for this change
A reference to a
MATERIALIZEDCTE reported unknown statistics. A query could then get a worse plan than the same query with the CTE inlined.Before, the hash table is built on the 100000 rows of
huge:After, it is built on the 5 rows of the CTE:
What changes are included in this PR?
The scan is a leaf and the body is a child of
MaterializedCteExec, so the planner copies the statistics to the scan inDefaultPhysicalPlanner::optimize_physical_plan, before the optimizer runs. The optimizer runs one time, as before. The EXPLAIN paths use the same function.Each scan gets the statistics of the body, computed with
StatisticsContext. A body that reads another CTE gets the statistics of that CTE first. A rewrite of the body does not change its rows, so the statistics stay correct. The statistics of one scan partition are known only when the scan has one partition. The statistics API is not changed.Other plan changes that follow from the statistics: an exact row count of the body can answer
count(*), and the body then does not run. One pinned plan incte_materialized.slthas a new join order.The ordering and the hash partitioning of the scans are a follow-up: #26424.
The scan gets the complete statistics of the body, not only the row count: column min, max, null count, distinct count and byte size. Thus the outer query uses the column statistics of the CTE. For example, a filter above the scan derives its bounds from the column bounds of the body, the same as in the inlined plan. The statistics of one partition are given only when the scan has one partition. Otherwise they are unknown, because scan partitions do not map to body partitions.
Comparison with PostgreSQL and DuckDB. This is what PostgreSQL and DuckDB do. PostgreSQL gives a CTE scan the row estimate of the CTE plan (allpaths.c). DuckDB copies the statistics of the body to each reference (propagate_cte.cpp).
Are these changes tested?
Yes.
cte_materialized.slt: the join-order plan, the statistics in EXPLAIN,count(*)from exact statistics, a chain of two CTEs, a scalar subquery and a view. A unit test for the statistics of a scan.Are there any user-facing changes?
Plans of queries with
MATERIALIZEDCTEs can change. New public functions:MaterializedCteScanExec::with_body_statisticsandMaterializedCteExec::body.