You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2026/03/31 02:41:35 UTC

[PR] feat: generate reversed-name data for sort pushdown benchmark [datafusion]

zhuqi-lucas opened a new pull request, #21266:
URL: https://github.com/apache/datafusion/pull/21266

   ## Which issue does this PR close?
   
   Related to https://github.com/apache/datafusion/issues/17348
   Precursor to https://github.com/apache/datafusion/pull/21182
   
   ## Rationale for this change
   
   The sort pushdown benchmark (#21213) uses TPC-H data where file names happen to match sort key order, so the optimization shows no difference vs. main ([comment](https://github.com/apache/datafusion/pull/21182#issuecomment-4158740710)).
   
   This PR generates custom benchmark data with **reversed file names** so the optimization is required to achieve sort elimination:
   
   ```
   c_high.parquet: l_orderkey 1-200k     (c sorts last alphabetically, but has lowest keys)
   b_mid.parquet:  l_orderkey 200k-400k
   a_low.parquet:  l_orderkey 400k+      (a sorts first alphabetically, but has highest keys)
   ```
   
   **On main (without optimization)**:
   - Alphabetical file order: `[a_low(400k+), b_mid(200k-400k), c_high(1-200k)]`
   - `validated_output_ordering()` sees files out of order → strips ordering
   - SortExec stays → slower
   
   **With optimization (#21182)**:
   - `sort_files_within_groups_by_statistics()` reorders to `[c_high, b_mid, a_low]`
   - Files non-overlapping → ordering valid → SortExec eliminated → faster
   
   ## What changes are included in this PR?
   
   - New `data_sort_pushdown` function in `bench.sh` that uses `datafusion-cli` to split TPC-H lineitem data into 3 sorted parquet files with reversed naming
   - Updated `run_sort_pushdown` / `run_sort_pushdown_sorted` to use the custom data path
   
   ## Test plan
   
   - [x] `cargo clippy -p datafusion-benchmarks` — 0 warnings
   - [x] Local benchmark shows sort elimination with optimization PR
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: generate reversed-name data for sort pushdown benchmark [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #21266:
URL: https://github.com/apache/datafusion/pull/21266


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: generate reversed-name data for sort pushdown benchmark [datafusion]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #21266:
URL: https://github.com/apache/datafusion/pull/21266#discussion_r3013156152


##########
benchmarks/bench.sh:
##########
@@ -1085,19 +1084,80 @@ run_external_aggr() {
 }
 
 # Runs the sort pushdown benchmark (without WITH ORDER)
-run_sort_pushdown() {
+# Generates sort pushdown benchmark data: multiple sorted parquet files with
+# reversed naming so alphabetical order does NOT match sort key order.
+# This forces the sort pushdown optimizer to reorder files by statistics.
+#
+# Files created (l_orderkey ranges):
+#   c_high.parquet: 1-200000       (c sorts last alphabetically, but has lowest keys)
+#   b_mid.parquet:  200001-400000
+#   a_low.parquet:  400001-600001  (a sorts first alphabetically, but has highest keys)
+data_sort_pushdown() {
+    SORT_PUSHDOWN_DIR="${DATA_DIR}/sort_pushdown/lineitem"
+    if [ -d "${SORT_PUSHDOWN_DIR}" ] && [ "$(ls -A ${SORT_PUSHDOWN_DIR}/*.parquet 2>/dev/null)" ]; then
+        echo "Sort pushdown data already exists at ${SORT_PUSHDOWN_DIR}"
+        return
+    fi
+
+    echo "Generating sort pushdown benchmark data..."
+
+    # First ensure we have TPC-H data to work with
+    data_tpch "1" "parquet"
+
     TPCH_DIR="${DATA_DIR}/tpch_sf1"
+    mkdir -p "${SORT_PUSHDOWN_DIR}"
+
+    # Build datafusion-cli if needed
+    $CARGO_COMMAND --bin datafusion-cli
+
+    DATAFUSION_CLI="./target/release/datafusion-cli"
+    if [ ! -f "$DATAFUSION_CLI" ]; then
+        DATAFUSION_CLI="./target/debug/datafusion-cli"

Review Comment:
   `$CARGO_COMMAND` defaults to `cargo run --release`, so `$CARGO_COMMAND --bin datafusion-cli` will *execute* the CLI (potentially starting an interactive session) rather than just building it. Also, the subsequent `./target/{release,debug}/datafusion-cli` paths are relative to `benchmarks/`, but Cargo outputs to `${DATAFUSION_DIR}/target/` by default, so this lookup will fail. Suggest building explicitly (e.g. `cargo build --release --bin datafusion-cli` in `${DATAFUSION_DIR}`) and setting `DATAFUSION_CLI` to `${DATAFUSION_DIR}/target/...`, then invoke it via `"${DATAFUSION_CLI}"`.
   ```suggestion
       (cd "$DATAFUSION_DIR" && cargo build --release --bin datafusion-cli)
   
       DATAFUSION_CLI="${DATAFUSION_DIR}/target/release/datafusion-cli"
       if [ ! -f "$DATAFUSION_CLI" ]; then
           DATAFUSION_CLI="${DATAFUSION_DIR}/target/debug/datafusion-cli"
   ```



##########
benchmarks/bench.sh:
##########
@@ -1085,19 +1084,80 @@ run_external_aggr() {
 }
 
 # Runs the sort pushdown benchmark (without WITH ORDER)
-run_sort_pushdown() {
+# Generates sort pushdown benchmark data: multiple sorted parquet files with
+# reversed naming so alphabetical order does NOT match sort key order.
+# This forces the sort pushdown optimizer to reorder files by statistics.
+#
+# Files created (l_orderkey ranges):
+#   c_high.parquet: 1-200000       (c sorts last alphabetically, but has lowest keys)
+#   b_mid.parquet:  200001-400000
+#   a_low.parquet:  400001-600001  (a sorts first alphabetically, but has highest keys)
+data_sort_pushdown() {
+    SORT_PUSHDOWN_DIR="${DATA_DIR}/sort_pushdown/lineitem"
+    if [ -d "${SORT_PUSHDOWN_DIR}" ] && [ "$(ls -A ${SORT_PUSHDOWN_DIR}/*.parquet 2>/dev/null)" ]; then

Review Comment:
   The existence check uses `ls` with an unquoted glob inside `$(...)` (`ls -A ${SORT_PUSHDOWN_DIR}/*.parquet`), which is brittle (breaks on spaces, and can behave oddly when the glob doesn’t match). Prefer a glob check that doesn’t invoke `ls`, e.g. `compgen -G "${SORT_PUSHDOWN_DIR}/*.parquet" > /dev/null` (or `find ... -name '*.parquet' -print -quit`).
   ```suggestion
       if [ -d "${SORT_PUSHDOWN_DIR}" ] && compgen -G "${SORT_PUSHDOWN_DIR}"/*.parquet > /dev/null; then
   ```



##########
benchmarks/bench.sh:
##########
@@ -1085,19 +1084,80 @@ run_external_aggr() {
 }
 
 # Runs the sort pushdown benchmark (without WITH ORDER)
-run_sort_pushdown() {
+# Generates sort pushdown benchmark data: multiple sorted parquet files with
+# reversed naming so alphabetical order does NOT match sort key order.
+# This forces the sort pushdown optimizer to reorder files by statistics.
+#
+# Files created (l_orderkey ranges):
+#   c_high.parquet: 1-200000       (c sorts last alphabetically, but has lowest keys)
+#   b_mid.parquet:  200001-400000
+#   a_low.parquet:  400001-600001  (a sorts first alphabetically, but has highest keys)
+data_sort_pushdown() {

Review Comment:
   The documented l_orderkey ranges don’t match the actual predicates: the comment says `a_low.parquet: 400001-600001`, but the query uses `WHERE l_orderkey > 400000` (no upper bound), which will include the rest of the dataset. Either update the comment to reflect the actual range or add an upper bound so the generated files match the stated ranges.



##########
benchmarks/bench.sh:
##########
@@ -1085,19 +1084,80 @@ run_external_aggr() {
 }
 
 # Runs the sort pushdown benchmark (without WITH ORDER)
-run_sort_pushdown() {
+# Generates sort pushdown benchmark data: multiple sorted parquet files with
+# reversed naming so alphabetical order does NOT match sort key order.
+# This forces the sort pushdown optimizer to reorder files by statistics.
+#
+# Files created (l_orderkey ranges):
+#   c_high.parquet: 1-200000       (c sorts last alphabetically, but has lowest keys)
+#   b_mid.parquet:  200001-400000
+#   a_low.parquet:  400001-600001  (a sorts first alphabetically, but has highest keys)
+data_sort_pushdown() {
+    SORT_PUSHDOWN_DIR="${DATA_DIR}/sort_pushdown/lineitem"
+    if [ -d "${SORT_PUSHDOWN_DIR}" ] && [ "$(ls -A ${SORT_PUSHDOWN_DIR}/*.parquet 2>/dev/null)" ]; then
+        echo "Sort pushdown data already exists at ${SORT_PUSHDOWN_DIR}"
+        return
+    fi
+
+    echo "Generating sort pushdown benchmark data..."
+
+    # First ensure we have TPC-H data to work with
+    data_tpch "1" "parquet"
+
     TPCH_DIR="${DATA_DIR}/tpch_sf1"
+    mkdir -p "${SORT_PUSHDOWN_DIR}"
+
+    # Build datafusion-cli if needed
+    $CARGO_COMMAND --bin datafusion-cli
+
+    DATAFUSION_CLI="./target/release/datafusion-cli"
+    if [ ! -f "$DATAFUSION_CLI" ]; then
+        DATAFUSION_CLI="./target/debug/datafusion-cli"
+    fi
+
+    echo "Creating sorted parquet files with reversed naming..."
+
+    $DATAFUSION_CLI --command "
+        CREATE EXTERNAL TABLE lineitem
+        STORED AS PARQUET
+        LOCATION '${TPCH_DIR}/lineitem/';
+
+        COPY (
+            SELECT * FROM lineitem
+            WHERE l_orderkey <= 200000
+            ORDER BY l_orderkey ASC
+        ) TO '${SORT_PUSHDOWN_DIR}/c_high.parquet';

Review Comment:
   The generated SQL relies on `COPY ... TO '<path>.parquet'` producing exactly one parquet file per range. If `datafusion.execution.target_partitions` is > 1, COPY may emit multiple output files/parts, which can defeat the intended “3 files with reversed names” setup. Consider setting `SET datafusion.execution.target_partitions = 1;` (and optionally other output settings) at the start of the `--command` SQL, similar to how sqllogictests force a single file group for ordering tests.



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: generate reversed-name data for sort pushdown benchmark [datafusion]

Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #21266:
URL: https://github.com/apache/datafusion/pull/21266#issuecomment-4174691492

   @adriangb Good suggestion! Reusing TPC-H data with multiple parts and renaming is much simpler. Plan:
   
   1. Generate TPC-H with `--parts=3` (files have non-overlapping l_orderkey ranges)
   2. Rename files so alphabetical order is reversed vs key order (e.g., part with highest keys → `a_xxx`, lowest → `c_xxx`)
   3. Benchmark uses `--sorted` to declare `WITH ORDER`
   
   No need for datafusion-cli or SQL splitting. Working on it.


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] feat: generate reversed-name data for sort pushdown benchmark [datafusion]

Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #21266:
URL: https://github.com/apache/datafusion/pull/21266#issuecomment-4175208553

   @adriangb Updated! Much simpler now — just `tpchgen --parts=3` + `mv` to rename files. No datafusion-cli needed.
   
   The reversed naming produces clear benchmark differences (release build, 6M rows):
   
   | Query | Main (ms) | With #21182 (ms) | Speedup |
   |-------|-----------|-------------------|---------|
   | Q1 ORDER BY full scan | 259 | 122 | **53%** |
   | Q2 ORDER BY LIMIT 100 | 80 | 9 | **89%** |
   | Q3 SELECT * ORDER BY | 700 | 353 | **50%** |
   | Q4 SELECT * LIMIT 100 | 342 | 24 | **93%** |


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org