Move PruningPredicate to physical-optimizer crate #12037
Closed
Conversation
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
alamb
commented
Aug 16, 2024
| //! Physical expressions. They are kept in their own crate to avoid physical | ||
| //! expressions depending on logical expressions. | ||
| //! | ||
| //! Note this crate is not intended to have substantial logic itself, but rather |
There was a problem hiding this comment.
I added some additional explanation about this crate (partly as a justification about why it was ok to depend on in the physical optimizer)
alamb
commented
Aug 16, 2024
| @@ -23,3 +23,5 @@ mod optimizer; | |||
| pub mod output_requirements; | |||
|
|
|||
| pub use optimizer::PhysicalOptimizerRule; | |||
|
|
|||
| pub mod pruning; | |||
There was a problem hiding this comment.
this was just to make the use statements less ugly
| @@ -615,7 +611,8 @@ impl PruningPredicate { | |||
| is_always_true(&self.predicate_expr) && self.literal_guarantees.is_empty() | |||
| } | |||
|
|
|||
| pub(crate) fn required_columns(&self) -> &RequiredColumns { | |||
| /// Return the columns that are required to evaluate the pruning predicate | |||
| pub fn required_columns(&self) -> &RequiredColumns { | |||
There was a problem hiding this comment.
as it is in a different crate now this function needs to become pub, which I think is reasonable as it is part of the API
Likewise the change to make RequiredColumns pub
| @@ -29,7 +29,6 @@ pub mod join_selection; | |||
| pub mod limited_distinct_aggregation; | |||
| pub mod optimizer; | |||
| pub mod projection_pushdown; | |||
| pub mod pruning; | |||
There was a problem hiding this comment.
there is a
pub use datafusion_physical_optimizer::*;a few lines down which adds an alias to the pruning module in its new home
alamb
force-pushed
the
alamb/move_pruning_predicate
branch
from
519f0f3 to
5a5fa91
Compare
|
I need to update the tests |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Which issue does this PR close?
Part of #11502
Rationale for this change
We are trying to break up pieces the
datafusioncrate into smaller pieces for faster compile time and better modularity. Part of this is the physical optimizerWhat changes are included in this PR?
Move
PruningPredicateinto the physical optimizer crateAre these changes tested?
Yes, by existing CI
Are there any user-facing changes?
No, there are backwards compatible
pub usethat mean no user change are required