neilconway commented on code in PR #23677:
URL: https://github.com/apache/datafusion/pull/23677#discussion_r3608505517
##########
datafusion/physical-optimizer/src/output_requirements.rs:
##########
Review Comment:
Not yours, but this comment should be generalized to `SortExec` +
`SortPreservingMergeExec`.
##########
datafusion/physical-optimizer/src/output_requirements.rs:
##########
@@ -387,6 +381,26 @@ fn require_top_ordering_helper(
plan: Arc<dyn ExecutionPlan>,
) -> Result<(Arc<dyn ExecutionPlan>, bool)> {
let mut children = plan.children();
+
+ // `ScalarSubqueryExec` is a multi-child but order-transparent root: child
0 is
+ // the main input (it copies that child's `PlanProperties` and reports
+ // `maintains_input_order()[0] == true` with no required input ordering),
while
+ // the remaining children are subquery plans that don't contribute to
output
+ // ordering. The generic `children.len() != 1` guard below would stop the
search
+ // at this node and lose the query's global ORDER BY (the top `SortExec`
lives
+ // below the main input), so descend through child 0 and reattach the rest.
+ if plan.downcast_ref::<ScalarSubqueryExec>().is_some() {
+ let (new_main, is_changed) =
+ require_top_ordering_helper(Arc::clone(children[0]))?;
+ if is_changed {
+ let mut new_children: Vec<Arc<dyn ExecutionPlan>> =
+ children.iter().map(|&c| Arc::clone(c)).collect();
+ new_children[0] = new_main;
+ return Ok((plan.with_new_children(new_children)?, true));
+ }
+ return Ok((plan, false));
+ }
Review Comment:
What if we added a helper like
```rust
fn output_requirement_child(plan: &dyn ExecutionPlan) -> Option<usize> {
let children = plan.children();
if children.len() == 1 {
Some(0)
} else if plan.is::<ScalarSubqueryExec>() {
Some(0)
} else {
None
}
}
```
That might isolate the subquery-specific logic to a single place?
##########
datafusion/physical-optimizer/src/output_requirements.rs:
##########
Review Comment:
This rule is not actually idempotent, because we only check for an existing
wrapper at the root of the plan, but wrappers might get added below the root of
the plan, so in some cases we'll add a redundant `OutputRequirementExec`.
Should we update the logic to add a top-level `OutputRequirementExec` to be
more intelligent?
--
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]
