Uh oh!
There was an error while loading. Please reload this page.
feat: make sort pushdown BufferExec capacity configurable, default 1GB - #21426
Conversation
Replace hardcoded 64MB BUFFER_CAPACITY_AFTER_SORT_ELIMINATION with configurable `datafusion.execution.sort_pushdown_buffer_capacity`. Default 1GB — large enough to hold wide-row data without I/O stalls. This is strictly less memory than the SortExec it replaces, and actual usage is bounded by partition size and global memory pool limits. Closesapache#21417
There was a problem hiding this comment.
Pull request overview
This PR makes the per-partition BufferExec capacity used during sort-pushdown sort elimination configurable (defaulting from 64MB to 1GiB) to reduce I/O stalls for wide-row scans.
Changes:
- Add new config option
datafusion.execution.sort_pushdown_buffer_capacity(default 1GiB). - Replace the hardcoded buffer capacity in
PushdownSortwith the new config value. - Update SLT plan expectations to reflect the new default capacity.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
datafusion/common/src/config.rs | Introduces sort_pushdown_buffer_capacity execution config with documentation and default (1GiB). |
datafusion/physical-optimizer/src/pushdown_sort.rs | Uses ConfigOptions value when inserting BufferExec after sort elimination. |
datafusion/sqllogictest/test_files/sort_pushdown.slt | Updates plan assertions to expect 1GiB buffer capacity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
zhuqi-lucas
commented
Apr 7, 2026
run benchmarks |
zhuqi-lucas
commented
Apr 7, 2026
run benchmark sort_pushdown_sorted |
adriangbot
commented
Apr 7, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing feat/configurable-buffer-capacity (f944ac6) to cdfade5 (merge-base) diff using: tpch File an issue against this benchmark runner |
adriangbot
commented
Apr 7, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing feat/configurable-buffer-capacity (f944ac6) to cdfade5 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
adriangbot
commented
Apr 7, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing feat/configurable-buffer-capacity (f944ac6) to cdfade5 (merge-base) diff using: sort_pushdown_sorted File an issue against this benchmark runner |
adriangbot
commented
Apr 7, 2026
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing feat/configurable-buffer-capacity (f944ac6) to cdfade5 (merge-base) diff using: tpcds File an issue against this benchmark runner |
adriangbot
commented
Apr 7, 2026
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagesort_pushdown_sorted — base (merge-base)
sort_pushdown_sorted — branch
File an issue against this benchmark runner |
adriangbot
commented
Apr 7, 2026
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
adriangbot
commented
Apr 7, 2026
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
@adriangbot The regression above seems invalid for clickbench because this query has no sort?
SELECT"UserID"FROM hits WHERE"UserID" = 435090932899640449; |
Dandandan
commented
Apr 7, 2026
This query is just noisy sometimes. |
zhuqi-lucas
commented
Apr 7, 2026
Good to know, thanks @Dandandan ! |
zhuqi-lucas
commented
Apr 7, 2026
Faster for the real test: Benchmark sort_pushdown_sorted.json
--------------------
┏━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ HEAD ┃ feat_configurable-buffer-capacity ┃ Change ┃
┡━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ Q1 │ 124.55 / 124.90 ±0.30 / 125.34 ms │ 123.01 / 124.19 ±0.84 / 125.21 ms │ no change │
│ Q2 │ 2.60 / 3.00 ±0.54 / 4.03 ms │ 2.61 / 2.82 ±0.25 / 3.30 ms │ +1.07x faster │
│ Q3 │ 332.38 / 338.61 ±4.15 / 344.25 ms │ 309.52 / 311.42 ±2.00 / 315.22 ms │ +1.09x faster │
│ Q4 │ 5.65 / 6.16 ±0.86 / 7.88 ms │ 5.56 / 5.91 ±0.60 / 7.11 ms │ no change │
└───────┴───────────────────────────────────┴───────────────────────────────────┴───────────────┘ |
alamb
left a comment
There was a problem hiding this comment.
Makes sense to me -- thank you @zhuqi-lucas and @xudong963
| /// buffers the entire partition). The buffer respects the global memory | ||
| /// pool limit. Setting this to a large value is safe — actual memory | ||
| /// usage is bounded by partition size and global memory limits. | ||
| pub sort_pushdown_buffer_capacity: usize, default = 1024 * 1024 * 1024 |
There was a problem hiding this comment.
This PR increases the size buffer because
64MB was too small for wide-row scans (16-column TPC-H SELECT * queries showed I/O stalls)
To be clear to anyone reading this, what will be on main is still better than 53.0.0 because prior to #21182 DataFusion would have sorted the entire thing (rather than just buffering it)
A fixed size like this is likely to buffer more than required for narrow cases
I suspect a better solution than a fixed size buffer would be some calculation based on the actual size of the data (e.g. the number of rows to buffer). However, that is tricky to compute / constrain memory when large strings are involved.
We probably would need to have both a row limit and a memory cap and pick the smaller of the two.
We can perhaps do this as a follow on issue/PR
There was a problem hiding this comment.
Good point! A dynamic approach with both a row limit and a memory cap (whichever is reached first) would adapt better to different row widths. Created #21440 to track this.
adriangb
commented
Apr 7, 2026
Yeah anything <50ms is going to be noisy |
adriangb
commented
Apr 7, 2026
Thanks @zhuqi-lucas ! |
Uh oh!
There was an error while loading. Please reload this page.
Thanks @alamb@adriangb@Dandandan@xudong963 for review! |
apache#21426) ## Which issue does this PR close? Closesapache#21417 ## Rationale for this change apache#21182 introduced `BufferExec` between `SortPreservingMergeExec` and `DataSourceExec` when sort elimination removes a `SortExec`. The buffer capacity was hardcoded to 64MB, which can cause I/O stalls for wide-row full scans. ## What changes are included in this PR? - Add `datafusion.execution.sort_pushdown_buffer_capacity` config option (default 1GB) - Replace hardcoded `BUFFER_CAPACITY_AFTER_SORT_ELIMINATION` constant with the config value - Update SLT test expectations for new default capacity ## How are these changes justified? **Why 1GB default:** - This is a maximum, not pre-allocated — actual usage is bounded by partition data size - Strictly less memory than the `SortExec` it replaces (which buffers entire partition) - `BufferExec` integrates with `MemoryPool`, so global memory limits are respected - 64MB was too small for wide-row scans (16-column TPC-H `SELECT *` queries showed I/O stalls) **Why configurable:** - Different workloads have different optimal buffer sizes - Users with memory-constrained environments can reduce it - Users with wide tables or large row groups can increase it ## Are these changes tested? - Existing SLT Test G verifies `BufferExec` appears in plan with correct capacity - Config integration tested via existing config framework ## Are there any user-facing changes? New config option: `datafusion.execution.sort_pushdown_buffer_capacity` (default: 1GB)
apache#21426) ## Which issue does this PR close? Closesapache#21417 ## Rationale for this change apache#21182 introduced `BufferExec` between `SortPreservingMergeExec` and `DataSourceExec` when sort elimination removes a `SortExec`. The buffer capacity was hardcoded to 64MB, which can cause I/O stalls for wide-row full scans. ## What changes are included in this PR? - Add `datafusion.execution.sort_pushdown_buffer_capacity` config option (default 1GB) - Replace hardcoded `BUFFER_CAPACITY_AFTER_SORT_ELIMINATION` constant with the config value - Update SLT test expectations for new default capacity ## How are these changes justified? **Why 1GB default:** - This is a maximum, not pre-allocated — actual usage is bounded by partition data size - Strictly less memory than the `SortExec` it replaces (which buffers entire partition) - `BufferExec` integrates with `MemoryPool`, so global memory limits are respected - 64MB was too small for wide-row scans (16-column TPC-H `SELECT *` queries showed I/O stalls) **Why configurable:** - Different workloads have different optimal buffer sizes - Users with memory-constrained environments can reduce it - Users with wide tables or large row groups can increase it ## Are these changes tested? - Existing SLT Test G verifies `BufferExec` appears in plan with correct capacity - Config integration tested via existing config framework ## Are there any user-facing changes? New config option: `datafusion.execution.sort_pushdown_buffer_capacity` (default: 1GB)
Which issue does this PR close?
Closes#21417
Rationale for this change
#21182 introduced
BufferExecbetweenSortPreservingMergeExecandDataSourceExecwhen sort elimination removes aSortExec. The buffer capacity was hardcoded to 64MB, which can cause I/O stalls for wide-row full scans.What changes are included in this PR?
datafusion.execution.sort_pushdown_buffer_capacityconfig option (default 1GB)BUFFER_CAPACITY_AFTER_SORT_ELIMINATIONconstant with the config valueHow are these changes justified?
Why 1GB default:
SortExecit replaces (which buffers entire partition)BufferExecintegrates withMemoryPool, so global memory limits are respectedSELECT *queries showed I/O stalls)Why configurable:
Are these changes tested?
BufferExecappears in plan with correct capacityAre there any user-facing changes?
New config option:
datafusion.execution.sort_pushdown_buffer_capacity(default: 1GB)