geoffreyclaude commented on code in PR #26118:
URL: https://github.com/apache/datafusion/pull/26118#discussion_r4367965071
##########
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]
