Skip to content

Always use PartitionMode::Auto in planner - #15339

Merged
Dandandan merged 9 commits into
apache:mainfrom
Dandandan:partition_mode
Mar 26, 2025
Merged

Always use PartitionMode::Auto in planner#15339
Dandandan merged 9 commits into
apache:mainfrom
Dandandan:partition_mode

Conversation

@Dandandan

@Dandandan Dandandan commented Mar 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

This improves plans a bit when file level statistics are available.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions Bot added the core Core DataFusion crate label Mar 20, 2025
@Dandandan Dandandan mentioned this pull request Mar 20, 2025
Closed
@github-actions github-actions Bot added the optimizer Optimizer rules label Mar 21, 2025
@github-actions github-actions Bot added documentation Improvements or additions to documentation sql SQL Planner development-process Related to development process of DataFusion logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate catalog Related to the catalog crate common Related to common crate execution Related to the execution crate proto Related to proto crate functions Changes to functions implementation datasource Changes to the datasource crate ffi Changes to the ffi crate labels Mar 21, 2025
@github-actions github-actions Bot removed documentation Improvements or additions to documentation sql SQL Planner development-process Related to development process of DataFusion logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Mar 21, 2025
@github-actions github-actions Bot removed substrait Changes to the substrait crate catalog Related to the catalog crate common Related to common crate execution Related to the execution crate proto Related to proto crate functions Changes to functions implementation datasource Changes to the datasource crate ffi Changes to the ffi crate labels Mar 21, 2025
@github-actions github-actions Bot added the optimizer Optimizer rules label Mar 21, 2025
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Mar 21, 2025
ozankabak
ozankabak reviewed Mar 22, 2025
View reviewed changes

@ozankabak ozankabak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this - left one question inline

Comment thread datafusion/sqllogictest/test_files/explain_tree.slt
physical_plan
01)┌───────────────────────────┐
02)│ CoalesceBatchesExec │
02)│ ProjectionExec │

@ozankabak ozankabak Mar 22, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have this projection here? Shouldn't the built-in projection of hash join handle this?

@Dandandan Dandandan Mar 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably should, but somehow doesn't, probably because of some optimization order of some sorts.
Anyway, the effect of this is usually small.

@ozankabak ozankabak Mar 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like your change made some deficiency/bug surface. I wouldn't be surprised if it is a small logic error in projection pushdown rule itself (and not related to optimization order).

@Dandandan Dandandan Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah...

@Dandandan
Dandandan requested a review from ozankabak March 24, 2025 19:43
@Dandandan

Dandandan commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

FYI @ozankabak this is now ready

@ozankabak

ozankabak commented Mar 24, 2025

Copy link
Copy Markdown
Contributor

Great - will review carefully tomorrow 🚀

berkaysynnada
berkaysynnada approved these changes Mar 26, 2025
View reviewed changes

@berkaysynnada berkaysynnada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea makes sense to me, and most of the plan changes happen when MemoryExec is the source (which provides stats not from session_state.config().collect_statistics() but plan derived stats)

@Dandandan

Dandandan commented Mar 26, 2025

Copy link
Copy Markdown
Contributor Author

Idea makes sense to me, and most of the plan changes happen when MemoryExec is the source (which provides stats not from session_state.config().collect_statistics() but plan derived stats)

Yes, or statistics estimates based on file size.

@Dandandan
Dandandan merged commit 42a101e into apache:main Mar 26, 2025
ozankabak
ozankabak reviewed Mar 26, 2025
View reviewed changes

@ozankabak ozankabak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a look and it LGTM - thank you

qstommyshu pushed a commit to qstommyshu/datafusion that referenced this pull request Mar 27, 2025
@Dandandan @qstommyshu
Always use PartitionMode::Auto in planner (apache#15339)
9dc3264 
* Always use PartitionMode::Auto

* Always use PartitionMode::Auto

* Fix test

* Fix test

* Fix tests

* Update plans

* Add changed splans
@goldmedal goldmedal mentioned this pull request Mar 27, 2025
Closed
nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025
@Dandandan @cipherstakes
Always use PartitionMode::Auto in planner (apache#15339)
7b38499 
* Always use PartitionMode::Auto

* Always use PartitionMode::Auto

* Fix test

* Fix test

* Fix tests

* Update plans

* Add changed splans
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ozankabak ozankabak ozankabak left review comments
+1 more reviewer
@berkaysynnada berkaysynnada berkaysynnada approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

core Core DataFusion crate optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Plan joins with PartitionMode::Auto by default

3 participants

@Dandandan @ozankabak @berkaysynnada