adriangb commented on code in PR #24958:
URL: https://github.com/apache/datafusion/pull/24958#discussion_r3950720874
##########
datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/sort_pushdown.rs:
##########
@@ -364,7 +386,7 @@ fn pushdown_sorts_helper(
// For operators that can take a sort pushdown, continue with updated
// requirements. If this node already outputs single partition (e.g.
SPM),
// don't push SinglePartition to children.
- let current_fetch = sort_push_down.plan.fetch();
+ let current_fetch = input_fetch(&sort_push_down.plan);
Review Comment:
This value and the `min_fetch` call below are correct only because
`parent_fetch` is `None` here for a `GlobalLimitExec`. The check in
`can_push_fetch_through` makes sure of this.
If `parent_fetch` is `Some`, the correct value is `min(fetch, parent_fetch)
+ skip`. The current code gives `min(fetch + skip, parent_fetch)`. That value
is too small.
To make the code correct in all cases, give parent_fetch to input_fetch. In
input_fetch, apply min_fetch(plan.fetch(), parent_fetch) first, then add skip.
Then set child.data.fetch = current_fetch below and remove the second min_fetch.
A comment documenting why this is correct / how the invariant is enforced
would also suffice.
--
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]
