Dandandan opened a new pull request, #2343: URL: https://github.com/apache/datafusion-ballista/pull/2343
## Which issue does this PR close? None filed. Motivated by three plan defects found while comparing Ballista's TPC-H plans against Spark with AQE (#2338, #2342): each returned correct results, so nothing in the test suite objected. ## Rationale for this change Plan regressions are quiet. A rewrite that adds a stage boundary, loses a broadcast, or turns a partitioned read into a single-partition broadcast read still returns the right rows — the cost surfaces only in a benchmark someone runs later. The q7 defect in #2342 had an entire stage running in one task, and every existing test passed. The repo already snapshots physical plans in unit tests (#2297). What was missing is a place that pins the *end product* of both planners for a set of representative shapes, so the diff of such a change lands in review. ## What changes are included in this PR? `state/plan_snapshots.rs` plans five query shapes through `DefaultDistributedPlanner` and through `AdaptivePlanner`, snapshotting both: | case | what it holds in place | | --- | --- | | `two_stage_aggregate` | partial → one shuffle on the group key → final | | `fact_join_fact` | neither side broadcastable, so both shuffle on the join key | | `fact_join_dimension` | static shuffles the 25-row dimension; AQE replicates it and reads the fact side in place | | `chained_dimension_joins` | two dimension joins that belong in one stage | | `top_k` | the fetch reaching the per-partition sort | Two details worth a look: - **Tables declare statistics rather than holding rows**, reusing the existing `StatsTable` fixture, so the fact table can sit at 200 GB / 6 B rows — far past the 128 MB broadcast threshold — and the dimensions far under it, without materialising anything. Decisions like broadcast-vs-partitioned are a function of statistics, so this is the only way to pin them at realistic sizes. - **The static planner is handed the session's own `ConfigOptions`**, not `ConfigOptions::default()`. The default takes `target_partitions` from the core count, which put `RepartitionExec: partitioning=RoundRobinBatch(10)` in the first draft of these snapshots on my 10-core machine and would have failed CI elsewhere. `aqe::test` and its `stats_table` child become `pub(crate)` so the fixture is reachable from `state`; both are `#[cfg(test)]`, so nothing changes in a release build. The pairs also document where the two planners diverge by design — only AQE re-plans on runtime statistics — which currently lives in reviewers' heads. ## Are these changes tested? The five tests are the change. Full `ballista-scheduler` suite passes (367 + 25) and `cargo clippy --all-targets` is clean. Worth knowing before merge: these snapshots follow DataFusion's planner output, so a DataFusion upgrade will move them. That is the intent — the diff shows what the upgrade did to the distributed plans — but it does mean one more thing to re-accept on those PRs (`cargo insta review`). ## Are there any user-facing changes? No. Test-only. -- 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]
