geoffreyclaude commented on code in PR #26118:
URL: https://github.com/apache/datafusion/pull/26118#discussion_r4367956053
##########
datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/sort_pushdown.rs:
##########
@@ -661,8 +623,21 @@ fn pushdown_requirement_to_children(
}
} else if let Some(hash_join) = plan.downcast_ref::<HashJoinExec>() {
handle_hash_join(hash_join, parent_required)
+ } else if plan.is::<BufferExec>()
+ || plan.is::<CooperativeExec>()
+ || plan.is::<OutputRequirementExec>()
+ || plan.is::<ScalarSubqueryExec>()
+ {
+ // These operators expose the first child's columns without
transformation.
+ // Scalar subquery children compute side inputs and do not contribute
rows.
+ let mut child_requirements = vec![None; plan.children().len()];
+ child_requirements[0] = Some(parent_required);
+ Ok(Some(child_requirements))
} else {
- handle_custom_pushdown(plan, parent_required, &maintains_input_order)
+ // Preserving row positions does not identify the input expressions
needed
Review Comment:
You’re right about the lost optimizations. The remaining concern is that
sorted output doesn’t prove the rewrite preserves the results. I explored this
further with my favorite AI agent and found a concrete counterexample (IIUC
your proposal!).
Consider a custom node that skips the first input row. Like `OFFSET 1`:
```
Input [2,1,3] → skip first → sort asc → [1,3]
Input [2,1,3] → sort asc → skip first → [2,3]
```
This node preserves row order and correctly reports sorted output when its
input is sorted. The probe would therefore accept the rewrite, although it
changes which rows are returned.
We need an explicit guarantee that reordering the node’s input is safe. I’d
keep the conservative fix here and explore a separate hook that lets custom
nodes permit pushdown for specific sort expressions.
--
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]
