Enforce sorting handle fetchable operators, add option to repartition based on row count estimates by mustafasrepo · Pull Request #11875 · apache/datafusion

mustafasrepo · 2024-08-07T14:51:03Z

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Recently @alihandroid added limit pushdown support for physical plan in the PR. After this PR, I recognized that EnforceSorting rule has some problems in handling operators with fetch. It sometimes loses fetch number during sort pushdown (Since LimitPushdown rule is after EnforceSorting we do not hit erroneous cases currently.). Hence, I used unit tests to trigger erroneous handlings.

Are these changes tested?

Yes, unit tests are added.

Are there any user-facing changes?

mustafasrepo · 2024-08-07T15:12:26Z


 assert_batches_sorted_eq!(
 [
- "+-----+-----+----+-------+",


Result of this test changes with this PR. I have analyzed the change, previously this tes was generating the following plan:
"ProjectionExec: expr=[c1@0 as c1, c2@1 as c2, c3@2 as c3, CAST(c2@1 AS Int8) + c3@2 as sum]", " RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1", " SortExec: expr=[c1@0 ASC,c2@1 ASC,c3@2 ASC], preserve_partitioning=[false]", " GlobalLimitExec: skip=0, fetch=1", " CoalescePartitionsExec", " CoalesceBatchesExec: target_batch_size=8192, fetch=1", " FilterExec: c2@1 = 3 AND c1@0 = a", " RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1", " CsvExec: file_groups={1 group: [[<PATH>]]}, projection=[c1, c2, c3], has_header=true",
After the changes in this PR, following plan is generated
"ProjectionExec: expr=[c1@0 as one, c2@1 as two, c3@2 as c3, CAST(c2@1 AS Int8) + c3@2 as total]", " RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1", " SortExec: TopK(fetch=1), expr=[c3@2 ASC], preserve_partitioning=[false]", " CoalescePartitionsExec", " CoalesceBatchesExec: target_batch_size=8192", " FilterExec: c2@1 = 3 AND c1@0 = a", " RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1", " CsvExec: file_groups={1 group: [[<PATH>]]}, projection=[c1, c2, c3], has_header=true",
I think the second plan generates a deterministic result. However, the query (dataframe query) is not deterministic as is.
With this observation, I have updated the place of the limit to make sure the query is deterministic after execution. With the change of the place of the limit, the following plan is generated:
"ProjectionExec: expr=[c1@0 as one, c2@1 as two, c3@2 as c3, CAST(c2@1 AS Int8) + c3@2 as total]", " GlobalLimitExec: skip=0, fetch=1", " SortPreservingMergeExec: [c1@0 ASC,c2@1 ASC,c3@2 ASC], fetch=1", " SortExec: TopK(fetch=1), expr=[c3@2 ASC], preserve_partitioning=[true]", " CoalesceBatchesExec: target_batch_size=8192", " FilterExec: c2@1 = 3 AND c1@0 = a", " RepartitionExec: partitioning=RoundRobinBatch(8), input_partitions=1", " CsvExec: file_groups={1 group: [[<PATH>]]}, projection=[c1, c2, c3], has_header=true",

I agree it also makes sense that the previous test did a sort right after a select + filter which will not produce a deterministic result. Doing the limit after the sort makes sense

ozankabak

LGTM

ozankabak · 2024-08-07T15:19:13Z

+05)--------ProjectionExec: expr=[]
+06)----------AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1], aggr=[]
+07)------------CoalesceBatchesExec: target_batch_size=4096
+08)--------------RepartitionExec: partitioning=Hash([c1@0], 2), input_partitions=2
+09)----------------AggregateExec: mode=Partial, gby=[c1@0 as c1], aggr=[]
+10)------------------ProjectionExec: expr=[c1@0 as c1]
+11)--------------------CoalesceBatchesExec: target_batch_size=4096
+12)----------------------FilterExec: c13@1 != C2GT5KVyOPZpgKVl110TyZO0NcJ434
+13)------------------------RepartitionExec: partitioning=RoundRobinBatch(2), input_partitions=1
+14)--------------------------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c1, c13], has_header=true
+


A better plan 🚀

ozankabak

This is looking very good!

For other reviewers: The optimizer removes the RR in some SLT tests because we estimate to have a single batch (the RR would be pointless). We are getting very smart 🚀

@alamb it would be great if you could take a look

alamb · 2024-08-08T12:46:59Z

@alamb it would be great if you could take a look

Will put it on my list for today

alamb

Thanks @mustafasrepo and @ozankabak -- I went through this PR carefully and I think it looks good to me.

I had some improvement suggestions but I don't think any are ncessary prior to merge

alamb · 2024-08-09T13:46:20Z


 assert_batches_sorted_eq!(
 [
- "+-----+-----+----+-------+",


I agree it also makes sense that the previous test did a sort right after a select + filter which will not produce a deterministic result. Doing the limit after the sort makes sense

ozankabak · 2024-08-09T17:56:33Z

Thanks for the review @alamb -- I will send one more commit and then this will be good to go.

ozankabak

After a careful study of the code, I have one issue in mind (for which I left an inline comment). We can merge the code after we make sure the plan change in question is not due to a regression.

ozankabak · 2024-08-09T20:38:57Z

-05)--------MemoryExec: partitions=4
+04)------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
+05)--------AggregateExec: mode=Partial, gby=[i@0 as i], aggr=[]
+06)----------MemoryExec: partitions=1


This is the only thing I don't understand here. I studied the rule logic but it is not clear to me why we don't use source output multi-partitioning but a RR later on.
Once we are sure this is not due to some regression, we can merge this PR

OK, I figured out what is going on here. With the optimizations we now do, the CREATE TABLE ... SELECT ... query doesn't create a multi-partition table (because it is not helpful). Therefore we see the RR in the downstream test. Reducing the batch size just before the test gives us the old plan. I updated the comment above accordingly.
So all is good, ready to go.

…artition based on row count estimates (apache#11875)" This reverts commit 79fa6f9.

mustafasrepo added 9 commits August 7, 2024 09:17

Tmp
510b16c

Minor changes
6ef4369

Minor changes
c3efafc

Minor changes
2bf220d

Implement top down recursion with delete check
eb83917

Minor changes
0b66b15

Minor changes
c769f9f

Address reviews
0ad7063

Update comments
3661f06

github-actionsBot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Aug 7, 2024

Minor changes
60967c1

mustafasrepo commented Aug 7, 2024
View reviewed changes

Make test deterministic
6b87c4c

ozankabak approved these changes Aug 7, 2024
View reviewed changes

Add fetch info to the statistics
8dd7e0a

ozankabak reviewed Aug 8, 2024
View reviewed changes

Comment threaddatafusion/core/src/physical_optimizer/enforce_sorting.rs Outdated

ozankabak reviewed Aug 8, 2024
View reviewed changes

Comment threaddatafusion/core/src/physical_optimizer/sort_pushdown.rs Outdated

ozankabak reviewed Aug 8, 2024
View reviewed changes

Comment threaddatafusion/core/src/physical_optimizer/sort_pushdown.rs Outdated

mustafasrepo added 4 commits August 8, 2024 14:37

Enforce distribution use inexact count estimate also.
15423ae

Minor changes
94fb83d

Minor changes
9053b9f

Minor changes
1171584

ozankabak approved these changes Aug 8, 2024
View reviewed changes

mustafasrepo added 4 commits August 9, 2024 15:37

Do not add unnecessary hash partitioning
711038d

Minor changes
7e598e5

Add config option to use inexact row number estimates during planning
12ad2c2

Update config
2e3cc5d

github-actionsBot added the documentation Improvements or additions to documentation label Aug 9, 2024

alamb changed the title ~~Enforce sorting handle fetchable operators.~~Enforce sorting handle fetchable operators, add option to repartition based on row count estimatesAug 9, 2024

mustafasrepo added 2 commits August 9, 2024 17:19

Minor changes
34af8ba

Minor changes
98760bc

alamb approved these changes Aug 9, 2024
View reviewed changes

ozankabakand others added 6 commits August 9, 2024 17:27

Final review
1e4dada

Address reviews
9fc4f3d

Add handling for sort removal with fetch
1116058

Fix linter errors
44dc292

Minor changes
c6d2de6

Update config
c7c85f4

Cleanup stats under fetch
7c8967d

ozankabak reviewed Aug 9, 2024
View reviewed changes

Update SLT comment
ed35660

ozankabak merged commit 79fa6f9 into apache:mainAug 10, 2024

wiedld pushed a commit to influxdata/arrow-datafusion that referenced this pull request Aug 15, 2024
Revert "Enforce sorting handle fetchable operators, add option to rep…
fa9b680
…artition based on row count estimates (apache#11875)" This reverts commit 79fa6f9.

Omega359 mentioned this pull request Nov 19, 2024
Order by is ignored #13483
Closed

Uh oh!

Conversation

mustafasrepo commented Aug 7, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

mustafasrepoAug 7, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alambAug 9, 2024

Choose a reason for hiding this comment

Uh oh!

ozankabak left a comment

Choose a reason for hiding this comment

Uh oh!

ozankabakAug 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ozankabak left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 8, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alambAug 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ozankabak commented Aug 9, 2024

Uh oh!

ozankabak left a comment

Choose a reason for hiding this comment

Uh oh!

ozankabakAug 9, 2024

Choose a reason for hiding this comment

Uh oh!

ozankabakAug 10, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mustafasrepo commented Aug 7, 2024•
edited
Loading

mustafasrepoAug 7, 2024•
edited
Loading