ryux1 commented on code in PR #24958:
URL: https://github.com/apache/datafusion/pull/24958#discussion_r3944295482
##########
datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/sort_pushdown.rs:
##########
@@ -79,6 +79,22 @@ impl Default for ParentRequirements {
pub type SortPushDown = PlanContext<ParentRequirements>;
+/// Number of input rows `plan` needs from its children in order to produce
+/// its own `fetch` rows. This is `plan.fetch()` for every operator except
+/// [`GlobalLimitExec`], which discards `skip` rows first and therefore needs
+/// `skip + fetch` input rows. Using the bare `fetch` there would turn
+/// `LIMIT 10 OFFSET 5` into `TopK(10)` below the limit, i.e. 5 result rows.
+///
+/// Note this is distinct from the fetch a parent imposes on `plan`'s *output*
+/// (`ParentRequirements::fetch`), for which `plan.fetch()` is the right bound.
+fn input_fetch(plan: &Arc<dyn ExecutionPlan>) -> Option<usize> {
+ let fetch = plan.fetch()?;
+ let skip = plan
+ .downcast_ref::<GlobalLimitExec>()
+ .map_or(0, |limit| limit.skip());
+ Some(fetch + skip)
Review Comment:
`GlobalLimitExec::new` accepts arbitrary `usize` values, so this addition
can overflow when `skip + fetch > usize::MAX`: debug builds panic, while
optimized builds wrap and may turn the pushed sort into e.g. `TopK(fetch=0)`,
producing incorrect results. DataFusion's `combine_limit` uses `saturating_add`
for the analogous limit composition. Could this use
`fetch.saturating_add(skip)` as well, with a unit test covering `skip =
usize::MAX, fetch = 1` (or an equivalent overflow boundary)?
--
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]
