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: [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]
