feat: add sort pushdown benchmark and SLT tests#21213
feat: add sort pushdown benchmark and SLT tests#21213zhuqi-lucas wants to merge 1 commit intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new dedicated benchmark to measure the impact of sort pushdown / sort elimination, enabling baseline vs “declared sorted” comparisons ahead of the core optimization work.
Changes:
- Introduce a new
sort-pushdownbenchmark module with fourORDER BY l_orderkeyquery variants (with/withoutLIMIT, narrow vsSELECT *). - Register the new benchmark module and
dfbenchsubcommand. - Extend
bench.shwith data/run entries forsort_pushdownandsort_pushdown_sorted.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
benchmarks/src/sort_pushdown.rs |
New benchmark implementation and CLI options for running the sort-pushdown scenarios |
benchmarks/src/lib.rs |
Exposes the new sort_pushdown module |
benchmarks/src/bin/dfbench.rs |
Adds the sort-pushdown subcommand wiring |
benchmarks/bench.sh |
Adds script commands to generate data and run the new benchmark variants |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //! Measured on 300k rows, 8 non-overlapping sorted parquet files, single partition, | ||
| //! debug build (results vary by hardware; relative speedup is the key metric): |
There was a problem hiding this comment.
The reference-results section claims the benchmark was run on "8 non-overlapping sorted parquet files" / single partition, but ./bench.sh data sort_pushdown currently just calls data_tpch (tpchgen-cli --parts=1) and ./bench.sh run sort_pushdown* doesn't force --partitions 1. Either adjust the reference-results text to match the default dataset/run configuration, or update the bench script/data generation to actually produce multi-part sorted inputs and set partitions explicitly.
| //! Measured on 300k rows, 8 non-overlapping sorted parquet files, single partition, | |
| //! debug build (results vary by hardware; relative speedup is the key metric): | |
| //! Measured on approximately 300k rows using the default dataset prepared via | |
| //! `./bench.sh data sort_pushdown`, debug build (results vary by hardware; the | |
| //! relative speedup is the key metric): |
| sort_pushdown|sort_pushdown_sorted) | ||
| # same data as for tpch | ||
| data_tpch "1" "parquet" | ||
| ;; |
There was a problem hiding this comment.
sort_pushdown|sort_pushdown_sorted reuses data_tpch "1" "parquet", which currently generates parquet with --parts=1. That means the benchmark will typically run against a single lineitem parquet file, not multiple non-overlapping files as described. If the intent is to benchmark cross-file sort elimination, consider generating TPCH parquet with multiple parts for this benchmark (and documenting that the files are expected to be sorted by l_orderkey).
| RESULTS_FILE="${RESULTS_DIR}/sort_pushdown.json" | ||
| echo "Running sort pushdown benchmark (no WITH ORDER)..." | ||
| debug_run $CARGO_COMMAND --bin dfbench -- sort-pushdown --iterations 5 --path "${TPCH_DIR}" -o "${RESULTS_FILE}" ${QUERY_ARG} ${LATENCY_ARG} | ||
| } |
There was a problem hiding this comment.
The new sort-pushdown benchmark is described as single-partition in the module docs, but the bench runner doesn't pass --partitions here (so it will use DataFusion's default target partitions). If you want stable, comparable results and to match the documented plan shape (no SPM for single partition), consider passing --partitions 1 (or documenting the expected/default partitions) in these runner functions.
| common: CommonOpt, | ||
|
|
||
| /// Sort pushdown query number. If not specified, runs all queries | ||
| #[arg(short, long)] |
There was a problem hiding this comment.
--query accepts any usize, but the code later does query_id - 1 / indexes into QUERIES, which will panic for --query 0 or any value > 4. Consider adding a clap value range (1..=4) and/or validating query_id in run() and returning a DataFusion error instead of panicking.
| #[arg(short, long)] | |
| #[arg(short, long, value_parser = clap::value_parser!(usize).range(1..=4))] |
Add benchmark and integration tests for sort pushdown optimization as a precursor to the core optimization PR (apache#21182). Benchmark: new `sort-pushdown` subcommand with 4 queries testing sort elimination (ASC full scan, ASC LIMIT, wide full, wide LIMIT). SLT tests (5 new groups): - Test A: Non-overlapping files + WITH ORDER → Sort eliminated - Test B: Overlapping files → SortExec retained - Test C: LIMIT queries (ASC sort elimination + DESC reverse scan) - Test D: target_partitions=2 → SPM + per-partition sort elimination - Test E: Inferred ordering from Parquet metadata (no WITH ORDER) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dc77327 to
b41eddf
Compare
Which issue does this PR close?
Related to #17348
Precursor to #21182
Rationale for this change
Add benchmark and integration tests for sort pushdown optimization, split out from #21182 per reviewer request. This allows comparing benchmark results before and after the optimization lands, and the SLT diff in #21182 will clearly show which test expectations changed due to the optimization.
What changes are included in this PR?
Benchmark
New
sort-pushdownbenchmark subcommand with 4 queries testing sort elimination:ORDER BY l_orderkey ASC(full scan)ORDER BY l_orderkey ASC LIMIT 100SELECT * ORDER BY l_orderkey ASC(wide)SELECT * ORDER BY l_orderkey ASC LIMIT 100(wide)Usage:
```bash
./bench.sh data sort_pushdown
./bench.sh run sort_pushdown # baseline
./bench.sh run sort_pushdown_sorted # with sort elimination
```
SLT Integration Tests (5 new groups)
Files Changed
Test plan
🤖 Generated with Claude Code