aectaan commented on code in PR #23288:
URL: https://github.com/apache/datafusion/pull/23288#discussion_r3520648239
##########
datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/sort_pushdown.rs:
##########
@@ -356,15 +356,28 @@ fn pushdown_requirement_to_children(
return Ok(None);
};
match determine_children_requirement(&parent_required, &child_req,
child_plan) {
- RequirementsCompatibility::Satisfy =>
Ok(Some(vec![Some(child_req)])),
+ RequirementsCompatibility::Satisfy => {
Review Comment:
It's linked. Without it suggested changes broke some tests.
Let's take as an example existing one:
```sql
select a,
rank() over (order by 1 RANGE BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING) rnk
from (select 1 a union select 2 a) q ORDER BY a
```
Prior this patch it had the next plan:
```
01)ProjectionExec: expr=[a@0 as a, rank() ORDER BY [Int64(1) ASC NULLS LAST]
RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING@1 as rnk]
02)--SortExec: expr=[a@0 ASC NULLS LAST], preserve_partitioning=[false]
03)----BoundedWindowAggExec: wdw=[rank() ORDER BY [Int64(1) ASC NULLS LAST]
RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING: Field { "rank()
ORDER BY [Int64(1) ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING": UInt64 }, frame: RANGE BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING], mode=[Sorted]
04)------CoalescePartitionsExec
05)--------AggregateExec: mode=FinalPartitioned, gby=[a@0 as a], aggr=[]
06)----------RepartitionExec: partitioning=Hash([a@0], 2), input_partitions=2
07)------------AggregateExec: mode=Partial, gby=[a@0 as a], aggr=[],
ordering_mode=Sorted
08)--------------UnionExec
09)----------------ProjectionExec: expr=[1 as a]
10)------------------PlaceholderRowExec
11)----------------ProjectionExec: expr=[2 as a]
12)------------------PlaceholderRowExec
```
but if we apply sort pushdown without discussed fix, it becomes
```
01)ProjectionExec: expr=[a@0 as a, rank() ORDER BY [Int64(1) ASC NULLS LAST]
RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING@1 as rnk]
02)--BoundedWindowAggExec: wdw=[rank() ORDER BY [Int64(1) ASC NULLS LAST]
RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING: Field { "rank()
ORDER BY [Int64(1) ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING": UInt64 }, frame: RANGE BETWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING], mode=[Sorted]
03)----CoalescePartitionsExec
04)------AggregateExec: mode=FinalPartitioned, gby=[a@0 as a], aggr=[]
05)--------RepartitionExec: partitioning=Hash([a@0], 2), input_partitions=2
06)----------AggregateExec: mode=Partial, gby=[a@0 as a], aggr=[],
ordering_mode=Sorted
07)------------UnionExec
08)--------------ProjectionExec: expr=[1 as a]
09)----------------PlaceholderRowExec
10)--------------ProjectionExec: expr=[2 as a]
11)----------------PlaceholderRowExec
```
every part of union is sorted, but not the whole union.
With discussed fix it stays correct/
--
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]
