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.
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -233,6 +233,10 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send +
Sync {
/// DataFusion uses this metadata to apply certain optimizations
/// such as automatically repartitioning correctly.
///
+ /// This describes row sequence, not how input expressions map to outputs.
+ /// Custom nodes that support sort pushdown should implement
+ /// [`Self::try_pushdown_sort`]; returning `true` here alone does not opt
in.
Review Comment:
Given the issue with the validated fallback (see other comment) I'd favor
moving forward with the conservative approach as it fixes a real bug, and
leaving a more generic safe fallback for a follow-up PR. WDYT? It's a tradeoff
between correctness and optimizations (no guarantee we can come up with a good
follow-up for DF56...)
--
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]
