Repository navigation
Conversation
github-actions
Bot
added
core
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #26428 +/- ##
========================================
Coverage 82.89% 82.90%
========================================
Files 1154 1154
Lines 457630 458240 +610
Branches 457630 458240 +610
========================================
+ Hits 379370 379892 +522
- Misses 55256 55269 +13
- Partials 23004 23079 +75 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
adriangb
force-pushed
the
mcte-body-first-properties
branch
2 times, most recently
from
October 17, 2026 21:58
61a82b2 to
ba2c820
Compare
adriangb
force-pushed
the
mcte-body-first-properties
branch
from
October 28, 2026 00:05
ba2c820 to
e59b111
Compare
adriangb
marked this pull request as ready for review
October 28, 2026 00:06
Move the optimizer loop into run_physical_optimizers, so that the planner can run it on more than one plan. Add materialized_ctes and as_materialized_cte, and use them in MaterializedCteBinder. No change in behavior. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Add MaterializedCteScanExec::with_body_properties, which gives a scan the partition count, the output ordering and the hash partitioning of a CTE body. Partition p of such a scan replays partition p of the body, and the scan returns an internal error if the partition counts differ at execution. Add MaterializedCteExec::register and MaterializedCteBuffer::name for the planner. The planner does not use these yet. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… body (RED) Add sqllogictests and a view test that expect a scan of a MATERIALIZED CTE to report the ordering and the hash partitioning of its body, so that the continuation does not sort or repartition again. These tests fail until the planner copies the properties of the body to the scans. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…(GREEN) optimize_physical_plan now optimizes each MATERIALIZED CTE body one time, on its own, as the child of a MaterializedCteExec, after the bodies that it reads or defines. Each scan then gets the statistics, the partition count, the output ordering and the hash partitioning of its optimized body. While the rest of the plan is optimized, each MaterializedCteExec is replaced by a planning-only node whose only child is the continuation, so the optimizers cannot change the body and the properties of the scans stay correct. Closes apache#26424. Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb
force-pushed
the
mcte-body-first-properties
branch
from
October 28, 2026 00:18
e59b111 to
efa27b5
Compare
Contributor
Author
|
cc @asolimando |
Member
|
I started checking and it looks good to me, there are a couple of points to discuss but the direction is good. I was a bit optimistic I could review by EOD today, but it's more realistic I will manage to provide a proper review by the end of this week, thanks for the ping, Adrian! |
asolimando
reviewed
Nov 1, 2026
asolimando
left a comment
asolimando
left a comment
Member
There was a problem hiding this comment.
Nice approach, Adrian, the design makes sense to me. I wish DF had support for DAG plans, it would make reasoning on CTEs and diamond plans much easier.
A few comments inline, the only one I'd like to settle before merging is the safety check in execute (either the check or a doc note), as it can lead to wrong results.
| let output = std::future::poll_fn(|cx| output.get_shared(cx)).await?; | ||
| // The ordering and the hash partitioning of the scan are only | ||
| // correct when one scan partition replays one body partition. | ||
| if mirrors_body && output.partitions.len() != output_partitions { |
Member
There was a problem hiding this comment.
Here we check only the partition count, but if you have a body with the same count but a different ordering or hash keys, it goes through with wrong results. I have checked with a variant of mirror_scan_rejects_other_body_partition_count: same buffer, unsorted body, the scan still reports [a ASC] and returns [3, 1, 2].
I don't think the problem can reproduce in DF core, but projects doing post-planning rewrites like Ballista or datafusion distributed could, we should probably handle this to be on the safe side.
A cheap fix would be checking the buffer's body in execute has the same hash partitioning, and an ordering that satisfies the scan's.
If you prefer not to fix it, I think we should at least put a note on MaterializedCteExec::new's documentation saying the body must keep the copied properties.
WDYT?
Member
There was a problem hiding this comment.
This points to the issue this PR closes, but statistics are still only known for single-partition scans. Shall we file a new issue and repoint it?
| if !same_columns { | ||
| return None; | ||
| } | ||
| let ordering = body.output_ordering().cloned(); |
Member
There was a problem hiding this comment.
Nit: this keeps only the first ordering, and drops the equivalences and constants of the body, but they would still be valid for the scan. Copying the body’s equivalence properties (with the scan schema) would preserve them.
| fn fmt_as(&self, t: DisplayFormatType, f: &mut fmt::Formatter) -> fmt::Result { | ||
| match t { | ||
| DisplayFormatType::Default | DisplayFormatType::Verbose => { | ||
| write!(f, "MaterializedCteExec: name={}", self.buffer.name()) |
Member
There was a problem hiding this comment.
I understand we don't want to leak the freezing of CTEs in plans, but plan display is the quickest debugging tool: in EXPLAIN VERBOSE the body disappears after the first rule while the node still shows as a regular MaterializedCteExec, so it looks like the body was dropped, which can be confusing.
Is there a way we can mark the node as frozen, or still show its body, somehow?
…es its scans report A scan that has the properties of its body checked only the partition count when it executed. A rewrite of the plan after planning that kept the count but changed the ordering or the hash keys of the body gave wrong results. The scan now also checks that the final body has its hash partitioning, orderings, equivalences, constants and constraints, and returns an internal error otherwise. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ts scans A scan with the properties of its body reported only the first output ordering of the body. It now reports all equivalence properties of the body (orderings, equivalences and constants), moved to the scan schema. Co-Authored-By: Claude Opus 5.5 <[email protected]>
While the optimizers run on the rest of the plan, each optimized CTE body is hidden in a planning-only `FrozenMaterializedCteExec`. That node called itself `MaterializedCteExec`, so in `EXPLAIN VERBOSE` the body seemed to disappear after the first rule. The node now shows its own name, like `OutputRequirementExec`, and the plans that the observer sees show the body as its first child. Only the copy shown to the observer has the body as a child; the optimizers still cannot reach it. Planning without an observer does not make the copy. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A scan of a materialized CTE with more than one partition reported unknown statistics for each partition. The planner now computes the statistics of each partition of the optimized body, with the same statistics context as the statistics of the whole body, and copies them to the scans. Partition `p` of a scan with `n` partitions merges the statistics of the body partitions `p, p + n, ...` that it replays. `MaterializedCteScanExec::with_body_statistics` (not released yet) takes the statistics of each body partition. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Add sql_planner cases that plan a CTE read once, read twice, and a chain of six CTEs that each read the previous one twice, for each kind of CTE, plus one query that mixes all three. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Contributor
Author
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing mcte-body-first-properties (4824866) to 985bbbf (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing mcte-body-first-properties (4824866) to 985bbbf (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
asolimando
approved these changes
Nov 2, 2026
asolimando
left a comment
asolimando
left a comment
Member
There was a problem hiding this comment.
Thanks Adrian, I saw you addressed all my comments and went even further, with the full equivalence properties checked at execution and per-partition statistics, LGTM!
galaxytemple
pushed a commit
to galaxytemple/datafusion
that referenced
this pull request
Nov 3, 2026
…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]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Follow-up to #26271, which is merged. Implements the plan in #26271 (comment).
Rationale for this change
A reference to a
MATERIALIZEDCTE reports no ordering and unknown partitioning. The continuation then sorts or repartitions rows that the body already sorted or partitioned:Before:
After:
On 10 million rows with 1 million groups (12 partitions,
datafusion-cli, release build, median of 30 runs), the query takes about 35 ms instead of about 41 ms.What changes are included in this PR?
DefaultPhysicalPlanner::optimize_physical_plannow does these steps for a plan with materialized CTEs:MaterializedCteExecwith an empty continuation, so the body has the same parent as in the full plan. A body that reads or defines another CTE goes after that CTE.MaterializedCteExecis replaced by a private planning-only node,FrozenMaterializedCteExec, whose only child is the continuation. So the optimizers cannot see or change the optimized body. Then theMaterializedCteExecnodes are put back.The body never changes after the scans copy its properties, so there is no check of the final plan and no fallback plan.
A scan with the properties of the body replays one body partition per partition. At execution it checks that the body has the partitioning and equivalence properties the scan reports. If they differ, it returns an internal error rather than wrong results. This check is cheap and guards against a future change that breaks the planning-time guarantee.
Partition
pof a scan withnpartitions reports the merged statistics of the body partitions it replays:p,p + n, and so on.A scan that comes from a separate planning run, such as a view, is bound again, so it loses the properties from that run.
EXPLAIN VERBOSEshows the frozen body. In eachphysical_plan after <rule>step, the CTE shows asFrozenMaterializedCteExec: name=c, with the optimized body as its first child. So the output is clear that rules do not touch the body in that run. The final plan showsMaterializedCteExecwith its body and continuation, as before. This is similar toOutputRequirementExec, which also shows only in the per-rule steps. The display copy is built only whenEXPLAIN VERBOSEasks for each step, so normal planning does not pay for it.Planning time. Measured with new
sql_plannerbenchmarks that are part of this PR. Each kind of CTE is planned read once, read twice, and as a chain of six CTEs that each read the previous one twice. Results are Criterion means:mainMATERIALIZED, one referenceMATERIALIZED, two referencesMATERIALIZED, chain of 6NOT MATERIALIZED, one referenceNOT MATERIALIZED, two referencesNOT MATERIALIZED, chain of 6With
EXPLAINindatafusion-cli(12 partitions, 2000 queries per file), the per-partition statistics add about 3%: 0.254 to 0.263 ms for one CTE, and 0.975 to 1.004 ms for a chain of 6.Limits:
EnforceSortingremoves anORDER BYat the top of a body withoutLIMIT, as before. So the scan has no ordering in that case.Comparison with PostgreSQL and DuckDB.
CTEnode with the body and the continuation as children, the same shape as the final plan here.Are these changes tested?
Yes.
cte_materialized.slt:ORDER BYwithoutLIMITEXPLAIN VERBOSEshows the frozen body in every changed stepAre there any user-facing changes?
Plans of queries with
MATERIALIZEDCTEs can change.MaterializedCteScanExecshowspartitioning=andoutput_ordering=in EXPLAIN when it has them.EXPLAIN VERBOSEshowsFrozenMaterializedCteExecin the per-rule steps.New public functions:
MaterializedCteScanExec::with_body_properties,MaterializedCteExec::registerandMaterializedCteBuffer::name.MaterializedCteScanExec::with_body_statisticsnow also takes the statistics of each body partition. It is new in #26271 and not released yet. Scan statistics now come from the optimized body.🤖 Generated with Claude Code