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.

Sign up for GitHub

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

Jump to bottom

Improve get_meet_of_orderings to check for common prefixes #5111

Merged
merged 2 commits into from
Jan 31, 2023
Merged

Improve get_meet_of_orderings to check for common prefixes #5111

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44114c6
Improve get_meet_of_orderings to check for common prefixes among the …
ozankabak Jan 30, 2023
21d00a1
Incorporate review suggestions
ozankabak Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Incorporate review suggestions
  • Loading branch information
@ozankabak
ozankabak committed Jan 30, 2023
commit 21d00a1c1348a5347d1935640e56e24f08bdd7bc
21 changes: 13 additions & 8 deletions datafusion/core/src/physical_plan/union.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ use crate::{
error::Result,
physical_plan::{expressions, metrics::BaselineMetrics},
};
use datafusion_physical_expr::utils::ordering_satisfy;
use tokio::macros::support::thread_rng_n;

/// `UnionExec`: `UNION ALL` execution plan.
Expand Down Expand Up @@ -240,14 +239,20 @@ impl ExecutionPlan for UnionExec {
// which is the "meet" of all input orderings. In this example, this
// function will return vec![false, true, true], indicating that we
// preserve the orderings for the 2nd and the 3rd children.
self.inputs()
.iter()
.map(|child| {
ordering_satisfy(self.output_ordering(), child.output_ordering(), || {
child.equivalence_properties()
if let Some(output_ordering) = self.output_ordering() {
self.inputs()
.iter()
.map(|child| {
if let Some(child_ordering) = child.output_ordering() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we no longer need to check that the output order of the child is compatible with the output order of the input. Shouldn't this be calling get_meet_of_orderings to check rather than checking the length?

Copy link
Contributor Author

@ozankabak ozankabak Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call self.output_ordering() in turn calls get_meet_of_orderings and computes the common longest prefix. We then check whether any of the inputs are equal to that prefix (equality in length implies equality in this case). Note that we are checking for strict equality instead of an equivalence-aware check following @mingmwang's suggestion.

output_ordering.len() == child_ordering.len()
} else {
false
}
})
})
.collect()
.collect()
} else {
vec![false; self.inputs().len()]
}
}

fn with_new_children(
Expand Down