You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "sgrebnov (via GitHub)" <gi...@apache.org> on 2026/04/20 09:06:38 UTC
[PR] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
sgrebnov opened a new pull request, #21744:
URL: https://github.com/apache/datafusion/pull/21744
## Which issue does this PR close?
When `PushdownSort` removes a `SortExec` because a source returns `Exact` (guaranteeing ordering), any `fetch` (LIMIT) on the `SortExec` is silently dropped if the underlying plan does not support `with_fetch()`.
For example, `ProjectionExec` supports `try_pushdown_sort` (delegating to its child) but does not implement `with_fetch()`. A plan like `SortExec(fetch=10) → ProjectionExec → source` that gets sort-eliminated loses the limit.
## What changes are included in this PR?
In the `Exact` branch of `PushdownSort`, when the eliminated `SortExec` carried a `fetch`:
1. Try `with_fetch()` on the pushed-down source first
2. If `with_fetch()` returns `None`, fall back to wrapping with `GlobalLimitExec`
## Are these changes tested?
Yes. Three new unit tests:
- `test_sort_pushdown_exact_no_fetch_no_limit` — Exact elimination without fetch: no limit wrapper added
- `test_sort_pushdown_exact_preserves_fetch_with_global_limit` — Exact elimination with fetch, source does NOT support `with_fetch()`: `GlobalLimitExec` wrapper added
- `test_sort_pushdown_exact_preserves_fetch_with_source_support` — Exact elimination with fetch, source supports `with_fetch()`: limit pushed into source directly
## Are there any user-facing changes?
No.
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "sgrebnov (via GitHub)" <gi...@apache.org>.
sgrebnov commented on code in PR #21744:
URL: https://github.com/apache/datafusion/pull/21744#discussion_r3128933657
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -149,14 +152,19 @@ impl PhysicalOptimizerRule for PushdownSort {
match sort_input.try_pushdown_sort(required_ordering)? {
SortOrderPushdownResult::Exact { inner } => {
// Data source guarantees perfect ordering - remove the Sort operator.
- // Preserve the fetch (LIMIT) from the original SortExec so the
- // data source can stop reading early.
- let inner = if let Some(fetch) = sort_exec.fetch() {
- inner.with_fetch(Some(fetch)).unwrap_or(inner)
+ //
+ // If the SortExec carried a fetch (LIMIT), we must preserve it.
+ // First try pushing the limit into the source via `with_fetch()`.
+ // If the source doesn't support `with_fetch`, fall back to
+ // wrapping with GlobalLimitExec.
+ if let Some(fetch) = sort_exec.fetch() {
+ let inner = inner.with_fetch(Some(fetch)).unwrap_or_else(|| {
+ Arc::new(GlobalLimitExec::new(inner, 0, Some(fetch)))
+ });
+ Ok(Transformed::yes(inner))
} else {
- inner
- };
- Ok(Transformed::yes(inner))
+ Ok(Transformed::yes(inner))
Review Comment:
@martin-g, @alamb - wanted to double-check we want `Transformed::no` here - it seems `Transformed::yes` is correct here — in the Exact branch we always replace the `SortExec` with `inner` (return new plan from `try_pushdown_sort` result), so the plan has changed regardless of whether a fetch/limit is applied. Could you please revisit? Thanks!
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "sgrebnov (via GitHub)" <gi...@apache.org>.
sgrebnov commented on code in PR #21744:
URL: https://github.com/apache/datafusion/pull/21744#discussion_r3129017418
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -105,7 +106,9 @@ impl PhysicalOptimizerRule for PushdownSort {
match sort_input.try_pushdown_sort(required_ordering)? {
SortOrderPushdownResult::Exact { inner } => {
let inner = if let Some(fetch) = sort_child.fetch() {
- inner.with_fetch(Some(fetch)).unwrap_or(inner)
+ inner.with_fetch(Some(fetch)).unwrap_or_else(|| {
+ Arc::new(GlobalLimitExec::new(inner, 0, Some(fetch)))
Review Comment:
@martin-g - great point, thank you. Updated to use `LocalLimitExec`. This is consistent with how `enforce_sorting` handles this.
https://github.com/apache/datafusion/blob/main/datafusion/physical-optimizer/src/enforce_sorting/mod.rs#L585-L594
```
// If the sort has a fetch, we need to add a limit:
if properties.output_partitioning().partition_count() == 1 {
let mut global_limit =
GlobalLimitExec::new(Arc::clone(sort_input), 0, Some(fetch));
global_limit.set_required_ordering(required_ordering);
Arc::new(global_limit)
} else {
let mut local_limit = LocalLimitExec::new(Arc::clone(sort_input), fetch);
local_limit.set_required_ordering(required_ordering);
Arc::new(local_limit)
}
```
Note: I didn't add set_required_ordering here because the Exact result means the source's plan properties already guarantee the ordering, but happy to add it for consistency / if required - please let me know.
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "sgrebnov (via GitHub)" <gi...@apache.org>.
sgrebnov commented on PR #21744:
URL: https://github.com/apache/datafusion/pull/21744#issuecomment-4302624831
> LGTM, would be better if there are some slt tests
@xudong963 — There's an existing SLT test ([sort_pushdown.slt Test 1.3](https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/sort_pushdown.slt#L57-L65)) that already covers the Exact pushdown + LIMIT case where the source supports `with_fetch()`. The specific fallback path this PR fixes (`GlobalLimitExec`/`LocalLimitExec` wrapping when the source doesn't support `with_fetch()`) can't be triggered through SLT because all built-in data sources support `with_fetch()`. Adding a custom test-only table provider to the SLT harness just for this seems like overkill, so I'd propose keeping the unit tests with the configurable `TestScan`. WDYT?
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "sgrebnov (via GitHub)" <gi...@apache.org>.
sgrebnov commented on code in PR #21744:
URL: https://github.com/apache/datafusion/pull/21744#discussion_r3128933657
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -149,14 +152,19 @@ impl PhysicalOptimizerRule for PushdownSort {
match sort_input.try_pushdown_sort(required_ordering)? {
SortOrderPushdownResult::Exact { inner } => {
// Data source guarantees perfect ordering - remove the Sort operator.
- // Preserve the fetch (LIMIT) from the original SortExec so the
- // data source can stop reading early.
- let inner = if let Some(fetch) = sort_exec.fetch() {
- inner.with_fetch(Some(fetch)).unwrap_or(inner)
+ //
+ // If the SortExec carried a fetch (LIMIT), we must preserve it.
+ // First try pushing the limit into the source via `with_fetch()`.
+ // If the source doesn't support `with_fetch`, fall back to
+ // wrapping with GlobalLimitExec.
+ if let Some(fetch) = sort_exec.fetch() {
+ let inner = inner.with_fetch(Some(fetch)).unwrap_or_else(|| {
+ Arc::new(GlobalLimitExec::new(inner, 0, Some(fetch)))
+ });
+ Ok(Transformed::yes(inner))
} else {
- inner
- };
- Ok(Transformed::yes(inner))
+ Ok(Transformed::yes(inner))
Review Comment:
@martin-g, @alamb - wanted to double-check we want `Transformed::no` here - it seems `Transformed::yes` is correct here — in the Exact branch we always replace the `SortExec` with `inner`, so the plan has changed regardless of whether a fetch/limit is applied. Could you please revisit? Thanks!
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #21744:
URL: https://github.com/apache/datafusion/pull/21744#discussion_r3121002266
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -149,14 +152,19 @@ impl PhysicalOptimizerRule for PushdownSort {
match sort_input.try_pushdown_sort(required_ordering)? {
SortOrderPushdownResult::Exact { inner } => {
// Data source guarantees perfect ordering - remove the Sort operator.
- // Preserve the fetch (LIMIT) from the original SortExec so the
- // data source can stop reading early.
- let inner = if let Some(fetch) = sort_exec.fetch() {
- inner.with_fetch(Some(fetch)).unwrap_or(inner)
+ //
+ // If the SortExec carried a fetch (LIMIT), we must preserve it.
+ // First try pushing the limit into the source via `with_fetch()`.
+ // If the source doesn't support `with_fetch`, fall back to
+ // wrapping with GlobalLimitExec.
+ if let Some(fetch) = sort_exec.fetch() {
+ let inner = inner.with_fetch(Some(fetch)).unwrap_or_else(|| {
+ Arc::new(GlobalLimitExec::new(inner, 0, Some(fetch)))
+ });
+ Ok(Transformed::yes(inner))
} else {
- inner
- };
- Ok(Transformed::yes(inner))
+ Ok(Transformed::yes(inner))
Review Comment:
I agree -- we should probaby return `Transformed::no` here
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #21744:
URL: https://github.com/apache/datafusion/pull/21744#discussion_r3129107504
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -149,14 +152,19 @@ impl PhysicalOptimizerRule for PushdownSort {
match sort_input.try_pushdown_sort(required_ordering)? {
SortOrderPushdownResult::Exact { inner } => {
// Data source guarantees perfect ordering - remove the Sort operator.
- // Preserve the fetch (LIMIT) from the original SortExec so the
- // data source can stop reading early.
- let inner = if let Some(fetch) = sort_exec.fetch() {
- inner.with_fetch(Some(fetch)).unwrap_or(inner)
+ //
+ // If the SortExec carried a fetch (LIMIT), we must preserve it.
+ // First try pushing the limit into the source via `with_fetch()`.
+ // If the source doesn't support `with_fetch`, fall back to
+ // wrapping with GlobalLimitExec.
+ if let Some(fetch) = sort_exec.fetch() {
+ let inner = inner.with_fetch(Some(fetch)).unwrap_or_else(|| {
+ Arc::new(GlobalLimitExec::new(inner, 0, Some(fetch)))
+ });
+ Ok(Transformed::yes(inner))
} else {
- inner
- };
- Ok(Transformed::yes(inner))
+ Ok(Transformed::yes(inner))
Review Comment:
I revisited and I confirm that I was wrong! `Transformed::yes` is correct!
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #21744:
URL: https://github.com/apache/datafusion/pull/21744
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "sgrebnov (via GitHub)" <gi...@apache.org>.
sgrebnov commented on code in PR #21744:
URL: https://github.com/apache/datafusion/pull/21744#discussion_r3129017418
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -105,7 +106,9 @@ impl PhysicalOptimizerRule for PushdownSort {
match sort_input.try_pushdown_sort(required_ordering)? {
SortOrderPushdownResult::Exact { inner } => {
let inner = if let Some(fetch) = sort_child.fetch() {
- inner.with_fetch(Some(fetch)).unwrap_or(inner)
+ inner.with_fetch(Some(fetch)).unwrap_or_else(|| {
+ Arc::new(GlobalLimitExec::new(inner, 0, Some(fetch)))
Review Comment:
@martin-g - great point, thank you. Updated to use `LocalLimitExec`
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #21744:
URL: https://github.com/apache/datafusion/pull/21744#discussion_r3110784460
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -149,14 +152,19 @@ impl PhysicalOptimizerRule for PushdownSort {
match sort_input.try_pushdown_sort(required_ordering)? {
SortOrderPushdownResult::Exact { inner } => {
// Data source guarantees perfect ordering - remove the Sort operator.
- // Preserve the fetch (LIMIT) from the original SortExec so the
- // data source can stop reading early.
- let inner = if let Some(fetch) = sort_exec.fetch() {
- inner.with_fetch(Some(fetch)).unwrap_or(inner)
+ //
+ // If the SortExec carried a fetch (LIMIT), we must preserve it.
+ // First try pushing the limit into the source via `with_fetch()`.
+ // If the source doesn't support `with_fetch`, fall back to
+ // wrapping with GlobalLimitExec.
+ if let Some(fetch) = sort_exec.fetch() {
+ let inner = inner.with_fetch(Some(fetch)).unwrap_or_else(|| {
+ Arc::new(GlobalLimitExec::new(inner, 0, Some(fetch)))
+ });
+ Ok(Transformed::yes(inner))
} else {
- inner
- };
- Ok(Transformed::yes(inner))
+ Ok(Transformed::yes(inner))
Review Comment:
Why this (old code) returns `Transformed::yes` ?
If there is no fetch/limit then there is no transformation.
##########
datafusion/physical-optimizer/src/pushdown_sort.rs:
##########
@@ -105,7 +106,9 @@ impl PhysicalOptimizerRule for PushdownSort {
match sort_input.try_pushdown_sort(required_ordering)? {
SortOrderPushdownResult::Exact { inner } => {
let inner = if let Some(fetch) = sort_child.fetch() {
- inner.with_fetch(Some(fetch)).unwrap_or(inner)
+ inner.with_fetch(Some(fetch)).unwrap_or_else(|| {
+ Arc::new(GlobalLimitExec::new(inner, 0, Some(fetch)))
Review Comment:
Line 102 mentions multi-partitioning (`sort_child.preserve_partitioning()`) but GlobalLimitExec requires single partitioning - https://docs.rs/datafusion-physical-plan/53.1.0/src/datafusion_physical_plan/limit.rs.html#170
--
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] Fix PushdownSort dropping LIMIT when eliminating SortExec [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #21744:
URL: https://github.com/apache/datafusion/pull/21744#issuecomment-4320022931
Thanks everyone. If it turns out we can make some slt tests perhaps we can add them as a follow on PR
--
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