-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Propagate table constraints through physical plans to optimize sort operations #14111
Merged
berkaysynnada
merged 43 commits into
apache:main
from
gokselk:feature/physical-planner-functional-dependence
Jan 16, 2025
Merged
Changes from all commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
d8333be
Add projection to `Constraints`
gokselk 30e5177
Add constraints support to `EquivalenceProperties`
gokselk 8f67f8f
Pass constraints to physical plan
gokselk 2fbe8c5
Add slt test for primary key sort optimization
gokselk 82361a7
Pass constraints to MemoryExec
gokselk b9717c7
Update properties.rs
berkaysynnada 3fc03e7
Simplify MemoryExec instantiation
gokselk 94da4a5
Rename EquivalenceProperties method name for clarity
gokselk 46b860f
Refactor projection handling in FileScanConfig
gokselk 05384e1
Bug fix
gokselk 7ecbfac
Display constraints on data sources
gokselk 0258a99
Bug fix and test improvements
gokselk acfb572
Use different schemas for tests
gokselk 4fbaaa3
Lint and visibility fix
gokselk b18b013
Merge branch 'apache_main' into feature/physical-planner-functional-d…
gokselk 966fd76
Fixes after merge
gokselk 97e4796
Review part 1
berkaysynnada 8e4d029
Merge branch 'apache_main' into feature/physical-planner-functional-d…
berkaysynnada ddd4d14
Update memory.rs
berkaysynnada 6297ec1
Merge branch 'apache_main' into feature/physical-planner-functional-d…
berkaysynnada bc72553
update dep
berkaysynnada b5d47c8
update proto
berkaysynnada 397297c
add aggregate distinct
berkaysynnada 56c053a
minor
berkaysynnada 95ba34f
Update order.slt
berkaysynnada f5c15ae
undo proto
berkaysynnada b2fdcc7
Update properties.rs
berkaysynnada 417370e
Merge remote-tracking branch 'upstream/main' into feature/physical-pl…
gokselk b0877a2
Move reserved entry
gokselk bbe35d4
Update `FileScanConfig` to return a single projected configuration ob…
gokselk cc3ca91
Improve constraint based ordering satisfaction logic
gokselk 8b0f340
Update datafusion/physical-plan/src/aggregates/mod.rs
gokselk a6c13de
Revert "Update `FileScanConfig` to return a single projected configur…
gokselk cda812f
Refactor MemoryExec constraints display
gokselk ab93279
Avoid unnecessary clone
gokselk 726737b
Refactor constraint based ordering satisfaction logic
gokselk 56e8054
Cargo fmt
gokselk e19c6a0
Revert "Avoid unnecessary clone"
gokselk 0a7a974
Avoid unnecessary clone
gokselk 8a2f372
Update properties.rs
berkaysynnada 5f9980a
Bug fix
gokselk d136860
Make `update_elements_with_matching_indices` take iterators for proj_…
gokselk 682e8a0
Revert "Make `update_elements_with_matching_indices` take iterators f…
gokselk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you refactor the
update_elements_with_matching_indices
function to take twoimpl Iterator
's (you probably need to replace the looping order to do that), this function can also acceptproj_indices
as animpl Iterator
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
update_elements_with_matching_indices
function uses.position()
onproj_indices
, which makes it necessary to clone it if we take it as animpl Iterator
. I think this defeats the whole purpose of this refactoring.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I had in mind was to swap the loop order (iterate on
proj_indices
on the outer loop). That may enable us to use animpl Iterator
forproj_indices
. We probably will need to keep the type ofentries
as a slice because it does not have an ordering (though we can enforce that in a future PR). Hadentries
was ordered, I think we could have also taken it in as animpl Iterator
-- but let's leave the latter for a future PRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We looked into this with @berkaysynnada and it seems to have some intricacies. Let's leave this to another PR