-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Deprecate PhysicalSortRequirement::from_sort_exprs
and PhysicalSortRequirement::to_sort_exprs
#13222
Merged
Merged
Deprecate PhysicalSortRequirement::from_sort_exprs
and PhysicalSortRequirement::to_sort_exprs
#13222
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f3168cf
Deprecate `PhysicalSortRequirement::from_sort_exprs` and `PhysicalSor…
alamb 79278f8
Update for API changes
alamb 6999626
fix clippy
alamb e5aebb9
Merge remote-tracking branch 'apache/main' into alamb/deprecate_conve…
alamb f5ff912
fmt
alamb 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,7 +61,7 @@ use crate::physical_plan::{Distribution, ExecutionPlan, InputOrderMode}; | |
|
||
use datafusion_common::plan_err; | ||
use datafusion_common::tree_node::{Transformed, TransformedResult, TreeNode}; | ||
use datafusion_physical_expr::{Partitioning, PhysicalSortRequirement}; | ||
use datafusion_physical_expr::Partitioning; | ||
use datafusion_physical_expr_common::sort_expr::{LexOrdering, LexRequirement}; | ||
use datafusion_physical_optimizer::PhysicalOptimizerRule; | ||
use datafusion_physical_plan::limit::{GlobalLimitExec, LocalLimitExec}; | ||
|
@@ -221,7 +221,7 @@ fn replace_with_partial_sort( | |
// here we're trying to find the common prefix for sorted columns that is required for the | ||
// sort and already satisfied by the given ordering | ||
let child_eq_properties = child.equivalence_properties(); | ||
let sort_req = PhysicalSortRequirement::from_sort_exprs(sort_plan.expr()); | ||
let sort_req = LexRequirement::from(sort_plan.expr().clone()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, there is no more cloning going on than previously, just now the clone is explicit |
||
|
||
let mut common_prefix_length = 0; | ||
while child_eq_properties.ordering_satisfy_requirement(&LexRequirement { | ||
|
@@ -275,8 +275,8 @@ fn parallelize_sorts( | |
{ | ||
// Take the initial sort expressions and requirements | ||
let (sort_exprs, fetch) = get_sort_exprs(&requirements.plan)?; | ||
let sort_reqs = PhysicalSortRequirement::from_sort_exprs(sort_exprs); | ||
let sort_exprs = LexOrdering::new(sort_exprs.to_vec()); | ||
let sort_reqs = LexRequirement::from(sort_exprs.clone()); | ||
let sort_exprs = sort_exprs.clone(); | ||
|
||
// If there is a connection between a `CoalescePartitionsExec` and a | ||
// global sort that satisfy the requirements (i.e. intermediate | ||
|
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,7 +94,8 @@ fn pushdown_sorts_helper( | |
if is_sort(plan) { | ||
let required_ordering = plan | ||
.output_ordering() | ||
.map(PhysicalSortRequirement::from_sort_exprs) | ||
.cloned() | ||
.map(LexRequirement::from) | ||
.unwrap_or_default(); | ||
if !satisfy_parent { | ||
// Make sure this `SortExec` satisfies parent requirements: | ||
|
@@ -180,11 +181,12 @@ fn pushdown_requirement_to_children( | |
RequirementsCompatibility::NonCompatible => Ok(None), | ||
} | ||
} else if let Some(sort_exec) = plan.as_any().downcast_ref::<SortExec>() { | ||
let sort_req = PhysicalSortRequirement::from_sort_exprs( | ||
let sort_req = LexRequirement::from( | ||
sort_exec | ||
.properties() | ||
.output_ordering() | ||
.unwrap_or(&LexOrdering::default()), | ||
.cloned() | ||
.unwrap_or(LexOrdering::default()), | ||
); | ||
if sort_exec | ||
.properties() | ||
|
@@ -205,10 +207,11 @@ fn pushdown_requirement_to_children( | |
.iter() | ||
.all(|maintain| *maintain) | ||
{ | ||
let output_req = PhysicalSortRequirement::from_sort_exprs( | ||
let output_req = LexRequirement::from( | ||
plan.properties() | ||
.output_ordering() | ||
.unwrap_or(&LexOrdering::default()), | ||
.cloned() | ||
.unwrap_or(LexOrdering::default()), | ||
); | ||
// Push down through operator with fetch when: | ||
// - requirement is aligned with output ordering | ||
|
@@ -227,14 +230,12 @@ fn pushdown_requirement_to_children( | |
} else if is_union(plan) { | ||
// UnionExec does not have real sort requirements for its input. Here we change the adjusted_request_ordering to UnionExec's output ordering and | ||
// propagate the sort requirements down to correct the unnecessary descendant SortExec under the UnionExec | ||
let req = (!parent_required.is_empty()) | ||
.then(|| LexRequirement::new(parent_required.to_vec())); | ||
let req = (!parent_required.is_empty()).then(|| parent_required.clone()); | ||
Ok(Some(vec![req; plan.children().len()])) | ||
} else if let Some(smj) = plan.as_any().downcast_ref::<SortMergeJoinExec>() { | ||
// If the current plan is SortMergeJoinExec | ||
let left_columns_len = smj.left().schema().fields().len(); | ||
let parent_required_expr = | ||
PhysicalSortRequirement::to_sort_exprs(parent_required.iter().cloned()); | ||
let parent_required_expr = LexOrdering::from(parent_required.clone()); | ||
match expr_source_side( | ||
parent_required_expr.as_ref(), | ||
smj.join_type(), | ||
|
@@ -251,8 +252,7 @@ fn pushdown_requirement_to_children( | |
smj.schema().fields.len() - smj.right().schema().fields.len(); | ||
let new_right_required = | ||
shift_right_required(parent_required, right_offset)?; | ||
let new_right_required_expr = | ||
PhysicalSortRequirement::to_sort_exprs(new_right_required); | ||
let new_right_required_expr = LexOrdering::from(new_right_required); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this structure makes it much clearer what is going on |
||
try_pushdown_requirements_to_join( | ||
smj, | ||
parent_required, | ||
|
@@ -278,8 +278,7 @@ fn pushdown_requirement_to_children( | |
// Pushing down is not beneficial | ||
Ok(None) | ||
} else if is_sort_preserving_merge(plan) { | ||
let new_ordering = | ||
PhysicalSortRequirement::to_sort_exprs(parent_required.to_vec()); | ||
let new_ordering = LexOrdering::from(parent_required.clone()); | ||
let mut spm_eqs = plan.equivalence_properties().clone(); | ||
// Sort preserving merge will have new ordering, one requirement above is pushed down to its below. | ||
spm_eqs = spm_eqs.with_reorder(new_ordering); | ||
|
@@ -412,7 +411,7 @@ fn try_pushdown_requirements_to_join( | |
let should_pushdown = smj_eqs.ordering_satisfy_requirement(parent_required); | ||
Ok(should_pushdown.then(|| { | ||
let mut required_input_ordering = smj.required_input_ordering(); | ||
let new_req = Some(PhysicalSortRequirement::from_sort_exprs(sort_expr)); | ||
let new_req = Some(LexRequirement::from(sort_expr.clone())); | ||
match push_side { | ||
JoinSide::Left => { | ||
required_input_ordering[0] = new_req; | ||
|
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
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.
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.
This is a pretty good example of the potential improvement -- it is now clear this function is returning a
LexRequirement
and that it clones the underlyingLexOrdering
structureThis was still happening on main but the conversion was hidden