Copilot commented on code in PR #21213: URL: https://github.com/apache/datafusion/pull/21213#discussion_r3004119722
########## benchmarks/src/sort_pushdown.rs: ########## @@ -0,0 +1,306 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Benchmark for sort pushdown optimization. +//! +//! Tests performance of sort elimination when files are non-overlapping and +//! internally sorted (declared via `--sorted` / `WITH ORDER`). +//! +//! # Usage +//! +//! ```text +//! # Prepare sorted TPCH lineitem data (SF=1) +//! ./bench.sh data sort_pushdown +//! +//! # Baseline (no WITH ORDER, full SortExec) +//! ./bench.sh run sort_pushdown +//! +//! # With sort elimination (WITH ORDER, SortExec removed) +//! ./bench.sh run sort_pushdown_sorted +//! ``` +//! +//! # Reference Results +//! +//! Measured on 300k rows, 8 non-overlapping sorted parquet files, single partition, +//! debug build (results vary by hardware; relative speedup is the key metric): Review 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. ```suggestion //! 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): ``` ########## benchmarks/bench.sh: ########## @@ -1070,6 +1084,22 @@ run_external_aggr() { debug_run $CARGO_COMMAND --bin external_aggr -- benchmark --partitions 4 --iterations 5 --path "${TPCH_DIR}" -o "${RESULTS_FILE}" ${QUERY_ARG} } +# Runs the sort pushdown benchmark (without WITH ORDER) +run_sort_pushdown() { + TPCH_DIR="${DATA_DIR}/tpch_sf1" + 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} +} Review 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. ########## benchmarks/bench.sh: ########## @@ -309,6 +313,10 @@ main() { # same data as for tpch data_tpch "1" "parquet" ;; + sort_pushdown|sort_pushdown_sorted) + # same data as for tpch + data_tpch "1" "parquet" + ;; Review 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`). ########## benchmarks/src/sort_pushdown.rs: ########## @@ -0,0 +1,306 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Benchmark for sort pushdown optimization. +//! +//! Tests performance of sort elimination when files are non-overlapping and +//! internally sorted (declared via `--sorted` / `WITH ORDER`). +//! +//! # Usage +//! +//! ```text +//! # Prepare sorted TPCH lineitem data (SF=1) +//! ./bench.sh data sort_pushdown +//! +//! # Baseline (no WITH ORDER, full SortExec) +//! ./bench.sh run sort_pushdown +//! +//! # With sort elimination (WITH ORDER, SortExec removed) +//! ./bench.sh run sort_pushdown_sorted +//! ``` +//! +//! # Reference Results +//! +//! Measured on 300k rows, 8 non-overlapping sorted parquet files, single partition, +//! debug build (results vary by hardware; relative speedup is the key metric): +//! +//! ```text +//! Query | Description | baseline (ms) | sort eliminated (ms) | speedup +//! ------|----------------------|---------------|---------------------|-------- +//! Q1 | ASC full scan | 159 | 91 | 43% +//! Q2 | ASC LIMIT 100 | 36 | 12 | 67% +//! Q3 | ASC full (wide, *) | 487 | 333 | 31% +//! Q4 | ASC LIMIT 100 (wide) | 119 | 30 | 74% +//! ``` +//! +//! Key observations: +//! - **LIMIT queries benefit most** (67-74%): sort elimination + limit pushdown +//! means only the first few rows are read before stopping. +//! - **Full scans** (31-43%): saving comes from eliminating the O(n log n) sort +//! step entirely. +//! - **Wide projections** amplify the benefit: larger rows make sorting more +//! expensive, so eliminating it saves more. + +use clap::Args; +use futures::StreamExt; +use std::path::PathBuf; +use std::sync::Arc; + +use datafusion::datasource::TableProvider; +use datafusion::datasource::file_format::parquet::ParquetFormat; +use datafusion::datasource::listing::{ + ListingOptions, ListingTable, ListingTableConfig, ListingTableUrl, +}; +use datafusion::error::Result; +use datafusion::execution::SessionStateBuilder; +use datafusion::physical_plan::display::DisplayableExecutionPlan; +use datafusion::physical_plan::{displayable, execute_stream}; +use datafusion::prelude::*; +use datafusion_common::DEFAULT_PARQUET_EXTENSION; +use datafusion_common::instant::Instant; + +use crate::util::{BenchmarkRun, CommonOpt, QueryResult, print_memory_stats}; + +#[derive(Debug, Args)] +pub struct RunOpt { + /// Common options + #[command(flatten)] + common: CommonOpt, + + /// Sort pushdown query number. If not specified, runs all queries + #[arg(short, long)] Review 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. ```suggestion #[arg(short, long, value_parser = clap::value_parser!(usize).range(1..=4))] ``` -- 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]
