Move `EnforceDistribution` into `datafusion-physical-optimizer` crate #14190

logan-keede · 2025-01-18T22:08:21Z

Which issue does this PR close?

Closes #14186 #14185 #14184.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Fix circular dependency

alamb

Thank you @logan-keede

datafusion/physical-optimizer/Cargo.toml

alamb · 2025-01-18T22:52:39Z

datafusion/physical-optimizer/src/utils.rs

+use datafusion_physical_plan::repartition::RepartitionExec;
+use datafusion_physical_plan::sorts::sort::SortExec;
+use datafusion_physical_plan::sorts::sort_preserving_merge::SortPreservingMergeExec;
+// use datafusion_physical_plan::union::UnionExec;


perhaps we can clean these up too

I kept it because I thought it would need to be moved here sooner or later, but sure it can be cleaned.
PS: you are talking about comments, right?

… into move_crates

logan-keede · 2025-01-20T22:59:31Z

This PR now fixes #14185 #14186 #14184.
moving tests to core_integration tests and organising it into suggested structure is still pending, but for now cargo build works.

…m/buraksenn/datafusion into moves_crates_test

berkaysynnada · 2025-01-21T11:30:27Z

Hi @logan-keede. Thank you for working on this exhaustive issue. The join_selection rule has been moved to the physical_optimizer crate, but enforce_sorting, enforce_distribution, and other utilities are still left in core for now. This will leave our fork in a very unstable state (and perhaps many others as well).

I’d like to push this PR (and eventually all rules) to the finish line as soon as possible. Are you facing any challenges that are slowing progress? Or would you mind if I push some commits to help move things forward?

logan-keede · 2025-01-21T12:59:38Z

@berkaysynnada This PR is pretty much in its last stretch, I was facing some problems with tests, but I think I have got it now, additionally @buraksenn should be pushing some commits to fix EnforceSorting tests soon.

I do not mind if you still want to help. I would certainly ask you if I get stuck again.

Thanks,
Logan

…m/buraksenn/datafusion into moves_crates_test

logan-keede · 2025-01-21T16:36:50Z

To move tests to datafusion/core/tests/physical_optimizer,
I choose to make a few required function public, I am not sure if that is detrimental to code quality or not.
Alternatively, we could keep those tests with the original file(EnforceDistribution).
Except that, I think it is mostly complete.
Thanks,
Logan

logan-keede · 2025-01-21T22:35:05Z

cc @alamb @xudong963

xudong963 · 2025-01-22T10:08:02Z

datafusion-testing

datafusion-testing is using the commit: 36283d195c728f26b16b517ba999fd62509b6649 (https://github.com/apache/datafusion-testing/commits/36283d195c728f26b16b517ba999fd62509b6649/)

xudong963 · 2025-01-22T10:09:14Z

datafusion/core/src/physical_optimizer/enforce_sorting.rs

dup with: https://github.com/apache/datafusion/pull/14219/files and #14219 is ready to merge. Maybe you can do a rebase after it merges.

dup with: https://github.com/apache/datafusion/pull/14219/files and #14219 is ready to merge. Maybe you can do a rebase after it merges.

I'll get these PR's, and resolve any conflicts.

berkaysynnada · 2025-01-22T11:59:46Z

@logan-keede I've resolved the conflicts and made some further simplifications to remove duplicated utilities. If you're okay with it, I'll send a few commits directly to your branch and then merge this PR.

Thanks for the collaboration!

berkaysynnada

I think it is ready. I’ve spent quite some time on this, and with the completion of #14235, we'll have completed the entire migration.

alamb

Thank you so much @berkaysynnada for pushing this along and @logan-keede for starting the process. Let's merge this PR in asap to avoid accumulating conflicts

alamb · 2025-01-22T22:25:09Z

datafusion/physical-optimizer/tests/enforce_sorting.rs

@@ -0,0 +1,861 @@
+// Licensed to the Apache Software Foundation (ASF) under one


I like how you have split out the tests here

alamb · 2025-01-22T22:25:40Z

datafusion/physical-optimizer/Cargo.toml

@@ -41,6 +41,7 @@ datafusion-common = { workspace = true, default-features = true }
 datafusion-execution = { workspace = true }
 datafusion-expr = { workspace = true }
 datafusion-expr-common = { workspace = true, default-features = true }
+datafusion-functions-aggregate = { workspace = true }


I would like to avoid this dependency if possible -- I will like to make a follow on PR to remove it -- similar to how I did in #14134

However when I just used the stub a few tests started failing on me

alamb · 2025-01-22T23:02:40Z

Here is a PR that proposes to consolidate the tests:

consolidate physical_optimizer tests into core/tests/physical_optimizer #14244

logan-keede added 2 commits January 19, 2025 03:27

moving enforce_distribution

1f12ffe

formatting fix

b58f9d6

github-actions bot added optimizer Optimizer rules core Core DataFusion crate labels Jan 18, 2025

Update Cargo.toml

0d571a7

Fix circular dependency

alamb changed the title ~~Move crates~~ Move `Encrates Jan 18, 2025

alamb changed the title ~~Move `Encrates~~ Move EnforceDistribution into datafusion-physical-optimizer crate Jan 18, 2025

alamb reviewed Jan 18, 2025

View reviewed changes

pulll source

24ab913

logan-keede mentioned this pull request Jan 19, 2025

Move ProjectionPushdown into datafusion-physical-optimizer crate #14184

Closed

move_tests to core integration tests

a7f8927

xudong963 self-requested a review January 20, 2025 06:37

logan-keede requested a review from alamb January 20, 2025 22:24

logan-keede and others added 5 commits January 21, 2025 04:04

remove tests from enforce_distribution.rs

0296d04

Merge branch 'move_crates' of https://github.com/logan-keede/datafusion…

1d94407

… into move_crates

passes lint

7cd4b31

forgotten license header

eb8ac39

move enforce_sorting

8103f00

buraksenn and others added 4 commits January 21, 2025 11:07

merge main

460c92d

import order

fef3c6e

Merge branch 'move-enforce-sorting-to-new-crate' of https://github.co…

7c1dad6

…m/buraksenn/datafusion into moves_crates_test

merge fixes + formatting

dcbc0e1

logan-keede added 2 commits January 21, 2025 17:32

fix: forgotten license

1f116e4

fix: cargo fmt

5093e61

logan-keede mentioned this pull request Jan 21, 2025

Move EnforceSorting into datafusion-physical-optimizer crate #14185

Closed

fix tests

9c40e49

Merge branch 'move-enforce-sorting-to-new-crate' of https://github.co…

3d78ef2

…m/buraksenn/datafusion into moves_crates_test

logan-keede added 2 commits January 21, 2025 21:29

fix: ci tests

9d19863

fix: Cargo.toml formatting

eba718a

xudong963 reviewed Jan 22, 2025

View reviewed changes

berkaysynnada mentioned this pull request Jan 22, 2025

Move EnforceSorting into datafusion-physical-optimizer crate #14219

Merged

berkaysynnada added 2 commits January 22, 2025 14:37

Merge branch 'apache_main' into move_crates

8926f90

further removals

afb20f8

berkaysynnada added 7 commits January 22, 2025 16:35

Further migrations and simplificaitons

f04332d

Fix failing tests

20d47d3

Final pass

b13ba8e

Update datafusion-testing

9e1dfbb

Update test_utils.rs

b7373fb

fix the dep

a8fe5ca

Update Cargo.toml

99a419f

berkaysynnada approved these changes Jan 22, 2025

View reviewed changes

alamb approved these changes Jan 22, 2025

View reviewed changes

alamb merged commit 2938fb1 into apache:main Jan 22, 2025
27 checks passed

This was referenced Jan 22, 2025

Remove dependency on physical-optimizer on functions-aggregates #14243

Closed

consolidate physical_optimizer tests into core/tests/physical_optimizer #14244

Merged

alamb mentioned this pull request Jan 22, 2025

Jan 18, 2025: This week(s) in DataFusion #14179

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `EnforceDistribution` into `datafusion-physical-optimizer` crate #14190

Move `EnforceDistribution` into `datafusion-physical-optimizer` crate #14190

logan-keede commented Jan 18, 2025 •

edited

Loading

alamb left a comment

alamb Jan 18, 2025

logan-keede Jan 18, 2025 •

edited

Loading

logan-keede commented Jan 20, 2025

berkaysynnada commented Jan 21, 2025

logan-keede commented Jan 21, 2025

logan-keede commented Jan 21, 2025

logan-keede commented Jan 21, 2025

xudong963 Jan 22, 2025

xudong963 Jan 22, 2025

berkaysynnada Jan 22, 2025

berkaysynnada commented Jan 22, 2025

berkaysynnada left a comment

alamb left a comment

alamb Jan 22, 2025

alamb Jan 22, 2025

alamb commented Jan 22, 2025

		@@ -0,0 +1,861 @@
		// Licensed to the Apache Software Foundation (ASF) under one

Move EnforceDistribution into datafusion-physical-optimizer crate #14190

Move EnforceDistribution into datafusion-physical-optimizer crate #14190

Conversation

logan-keede commented Jan 18, 2025 • edited Loading

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?

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 18, 2025

Choose a reason for hiding this comment

logan-keede Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

logan-keede commented Jan 20, 2025

berkaysynnada commented Jan 21, 2025

logan-keede commented Jan 21, 2025

logan-keede commented Jan 21, 2025

logan-keede commented Jan 21, 2025

xudong963 Jan 22, 2025

Choose a reason for hiding this comment

xudong963 Jan 22, 2025

Choose a reason for hiding this comment

berkaysynnada Jan 22, 2025

Choose a reason for hiding this comment

berkaysynnada commented Jan 22, 2025

berkaysynnada left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 22, 2025

Choose a reason for hiding this comment

alamb Jan 22, 2025

Choose a reason for hiding this comment

alamb commented Jan 22, 2025

Move `EnforceDistribution` into `datafusion-physical-optimizer` crate #14190

Move `EnforceDistribution` into `datafusion-physical-optimizer` crate #14190

logan-keede commented Jan 18, 2025 •

edited

Loading

logan-keede Jan 18, 2025 •

edited

Loading