Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move EnforceSorting into datafusion-physical-optimizer crate #14185

Closed
Tracked by #11502
alamb opened this issue Jan 18, 2025 · 8 comments · Fixed by #14219
Closed
Tracked by #11502

Move EnforceSorting into datafusion-physical-optimizer crate #14185

alamb opened this issue Jan 18, 2025 · 8 comments · Fixed by #14219
Assignees

Comments

@alamb
Copy link
Contributor

alamb commented Jan 18, 2025

Is your feature request related to a problem or challenge?

Historically DataFusion was one (very) large crate datafusion, and as it grew bigger we extracted various functionality into separate crates. This leads to both faster compile times (as the crates can be compiled in parallel) as well easier to navigate code (as the crates force a cleaner dependency separation)

One project we have not yet completed is to extract the physical optimizer passes out #11502

Describe the solution you'd like

Extract the ProjectionPushdown from the datafusion core crate to the datafusion-physical-optimizer crate

Describe alternatives you've considered

Move the code

I believe you will have to move two other modules:

I would recomend the following structure:

datafusion/physical-optimizer/src/enforce_sorting/mod.rs (old enforce_sorting.rs)
datafusion/physical-optimizer/src/enforce_sorting/replace_with_order_preserving_variants.rs 
datafusion/physical-optimizer/src/enforce_sorting/sort_pushdown.rs 

Notes that due to dependencies (e.g. on SessionContext or functions), you may have to move the tests into the core_integration tests here:

Additional context

Here are some example PRs

I think this is likely not a good first issue as it will invovle substantial code

@buraksenn
Copy link
Contributor

take

@logan-keede
Copy link

logan-keede commented Jan 20, 2025

oops, We have a PR clash. @buraksenn Is your PR complete? I can try and merge it into mine, or the other way around.

@buraksenn
Copy link
Contributor

buraksenn commented Jan 21, 2025

@logan-keede I just saw this comment. I had a test error that I'm fixing now. I think it will be complete in an hour

@logan-keede
Copy link

@buraksenn can you push it directly to #14190, I have already merged your previous commits so it should not cause you much trouble?
Thanks

@buraksenn
Copy link
Contributor

Sorry I could not look into this afterwards and currently working. I think my PR is ready for merge so maybe we can merge that one and then rebease from main. Otherwise, I can look into this tomorrow

@logan-keede
Copy link

No worries, I did it myself.

@alamb
Copy link
Contributor Author

alamb commented Jan 21, 2025

Sorry I could not look into this afterwards and currently working. I think my PR is ready for merge so maybe we can merge that one and then rebease from main. Otherwise, I can look into this tomorrow

Thanks everyone -- I just approved the following PR and hopefully we can merge it tomorrow

@berkaysynnada
Copy link
Contributor

I am taking a final look on #14219, and #14190, and I'll merge them if not see a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants