2010YOUY01 commented on PR #27393:
URL: https://github.com/apache/datafusion/pull/27393#issuecomment-6868985044

   @tschwarzinger thanks for the review!
   
   Regarding extra projection: it's a great point, we could do it in a 
follow-up PR
   
   > I think we have code that solves this problem in 
`datafusion_physical_optimizer::output_requirements::require_top_ordering`. It 
looks at `maintains_input_order` and keeps the sort requirement. Maybe we 
should re-use that piece of code? Then we would also have consistency between 
`OutputRequirements` and `DataSinkExec`.
   
   I think the challenge is, for the below plan shape:
   ```
   DataSinkExec
     SubPlan
   ```
   - Derived output ordering of subplan is expected to be wrong before 
`EnsureRequirements` rule is finished
   - If DataSinkExec tries to set `required_input_ordering` after 
`EnsureRequirements`, the subplan might even get removed in an unintended way. 
   
   I suspect we need some deep plumbing into `EnsureRequirements`'s sorting 
part, to solve it more elegantly.
   
   > The only downside I see is that custom sort operators no longer work for 
`COPY` (but they have a hard time in general considering the output 
requirements case IIRC). So I think we should address that once a use case 
comes along with another planner info `established_output_ordering` or 
something like that.
   
   Question: any case that still won't work with explicitly extra sort, like
   ```
   # This initial plan shape keeps written file order, while avoids re-sorting 
during later optimizations
   DataSinkExec
     SortExec             // explicit order by 'k'
       CustomExec     // existing order by 'k'
   ```
   
   ----
   
   So I think the future work should be implementing a general helper to detect 
explicit sorts like
   ```
   DataSinkExec
     SortExec
   
   DataSinkExec
     Projection
       SortExec
   
   ... and other possible shapes
   ```


-- 
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]

Reply via email to