-
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
SanityCheckPlan should compare UnionExec inputs to requirements for output (parent). #12414
Closed
wiedld
wants to merge
11
commits into
apache:main
from
influxdata:iox-11945/sanity-check-plan-sort-pushdown
+150
−27
Closed
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
c2e652e
test: demonstrate bug where the SanityCheckPlan does not know to igno…
wiedld a50edc3
fix: for the UnionExec, the sanity check should enforce restrictions …
wiedld 782e18d
test: update test once bug fix is complete
wiedld 6e7356b
Revert "fix: for the UnionExec, the sanity check should enforce restr…
wiedld 0f317a6
chore: update across_partitions docs to match actual usage
wiedld 4bd4db0
fix: add constants from either side to the UnionExec constants
wiedld 6dafd1a
Revert "chore: update across_partitions docs to match actual usage"
wiedld 08ac804
Revert "fix: add constants from either side to the UnionExec constants"
wiedld 1304bdf
Merge branch 'main' into iox-11945/sanity-check-plan-sort-pushdown
wiedld ad4a321
fix: create a new sort order representing the UNIONed output
wiedld 53e0c56
test: update tests to reflect a unioned sort order added to UNION node
wiedld 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1524,6 +1524,102 @@ impl Hash for ExprWrapper { | |
} | ||
} | ||
|
||
/// Take sort orderings for unioned sides of equal length, and return the unioned ordering. | ||
/// | ||
/// Example: | ||
/// child1 = order by a0, b, c | ||
/// child2 = order by a, b, c | ||
/// => union's joint order is a0, a, b, c. | ||
fn calculate_joint_ordering( | ||
lhs: &EquivalenceProperties, | ||
rhs: &EquivalenceProperties, | ||
) -> LexOrdering { | ||
let mut union_ordering = vec![]; | ||
for ordering in lhs | ||
.normalized_oeq_class() | ||
.orderings | ||
.iter() | ||
.chain(rhs.normalized_oeq_class().orderings.iter()) | ||
{ | ||
if union_ordering.is_empty() { | ||
union_ordering = ordering.clone(); | ||
continue; | ||
} | ||
|
||
if !union_ordering.len().eq(&ordering.len()) { | ||
break; | ||
} | ||
|
||
let mut unioned = union_ordering.into_iter().peekable(); | ||
let mut curr = ordering.iter().peekable(); | ||
let mut new_union = vec![]; | ||
loop { | ||
match (curr.next(), unioned.next()) { | ||
(None, None) => break, | ||
(None, Some(u)) => { | ||
new_union.push(u.clone()); | ||
continue; | ||
} | ||
(Some(c), None) => { | ||
new_union.push(c.clone()); | ||
continue; | ||
} | ||
(Some(c), Some(u)) => { | ||
if c.eq(&u) { | ||
new_union.push(c.clone()); | ||
continue; | ||
} else if c.expr.eq(&u.expr) { | ||
// options are different => negates each other | ||
continue; | ||
} else { | ||
new_union.push(u.clone()); | ||
new_union.push(c.clone()); | ||
continue; | ||
} | ||
} | ||
} | ||
} | ||
union_ordering = new_union; | ||
} | ||
collapse_lex_ordering(union_ordering) | ||
} | ||
|
||
/// Take sort orderings for unioned sides return the shorten, novel sort order. | ||
/// | ||
/// Example: | ||
/// child1 = order by a, b | ||
/// child2 = order by a1, b1, c1 | ||
/// => union's prefixed order is a, b. | ||
fn calculate_prefix_ordering( | ||
lhs: &EquivalenceProperties, | ||
rhs: &EquivalenceProperties, | ||
) -> Vec<LexOrdering> { | ||
// Calculate valid orderings for the union by searching for prefixes | ||
// in both sides. | ||
let mut orderings = vec![]; | ||
for mut ordering in lhs.normalized_oeq_class().orderings { | ||
// Progressively shorten the ordering to search for a satisfied prefix: | ||
while !rhs.ordering_satisfy(&ordering) { | ||
ordering.pop(); | ||
} | ||
// There is a non-trivial satisfied prefix, add it as a valid ordering: | ||
if !ordering.is_empty() { | ||
orderings.push(ordering); | ||
} | ||
} | ||
for mut ordering in rhs.normalized_oeq_class().orderings { | ||
// Progressively shorten the ordering to search for a satisfied prefix: | ||
while !lhs.ordering_satisfy(&ordering) { | ||
ordering.pop(); | ||
} | ||
// There is a non-trivial satisfied prefix, add it as a valid ordering: | ||
if !ordering.is_empty() { | ||
orderings.push(ordering); | ||
} | ||
} | ||
orderings | ||
} | ||
|
||
/// Calculates the union (in the sense of `UnionExec`) `EquivalenceProperties` | ||
/// of `lhs` and `rhs` according to the schema of `lhs`. | ||
fn calculate_union_binary( | ||
|
@@ -1553,32 +1649,15 @@ fn calculate_union_binary( | |
}) | ||
.collect(); | ||
|
||
// Next, calculate valid orderings for the union by searching for prefixes | ||
// in both sides. | ||
let mut orderings = vec![]; | ||
for mut ordering in lhs.normalized_oeq_class().orderings { | ||
// Progressively shorten the ordering to search for a satisfied prefix: | ||
while !rhs.ordering_satisfy(&ordering) { | ||
ordering.pop(); | ||
} | ||
// There is a non-trivial satisfied prefix, add it as a valid ordering: | ||
if !ordering.is_empty() { | ||
orderings.push(ordering); | ||
} | ||
} | ||
for mut ordering in rhs.normalized_oeq_class().orderings { | ||
// Progressively shorten the ordering to search for a satisfied prefix: | ||
while !lhs.ordering_satisfy(&ordering) { | ||
ordering.pop(); | ||
} | ||
// There is a non-trivial satisfied prefix, add it as a valid ordering: | ||
if !ordering.is_empty() { | ||
orderings.push(ordering); | ||
} | ||
} | ||
// Create a unioned ordering. | ||
let mut orderings = calculate_prefix_ordering(&lhs, &rhs); | ||
let union_ordering = calculate_joint_ordering(&lhs, &rhs); | ||
orderings.push(union_ordering); | ||
|
||
let mut eq_properties = EquivalenceProperties::new(lhs.schema); | ||
eq_properties.constants = constants; | ||
eq_properties.add_new_orderings(orderings); | ||
|
||
Ok(eq_properties) | ||
} | ||
|
||
|
@@ -2645,8 +2724,8 @@ mod tests { | |
Arc::clone(&schema3), | ||
), | ||
], | ||
// Expected | ||
vec![vec!["a", "b"]], | ||
// Expected: union sort orders | ||
vec![vec!["a", "b", "c"]], | ||
), | ||
// --------- TEST CASE 2 ---------- | ||
( | ||
|
@@ -2720,8 +2799,8 @@ mod tests { | |
Arc::clone(&schema3), | ||
), | ||
], | ||
// Expected | ||
vec![], | ||
// Expected: union sort orders | ||
vec![vec!["a", "b", "c"]], | ||
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 these test changes are not correct. 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 agree -- these changes make the expected output incorrect |
||
), | ||
// --------- TEST CASE 5 ---------- | ||
( | ||
|
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.
I didn't actually fully understand why do you need such a function. I think its logic is not correct.
SCHEMA: a-b-c-a0
CHILD1: (5,15,25,35),(4,15,25,36),(3,15,25,37)
CHILD2: (5,15,25,35),(6,15,25,34),(7,15,25,33)
How could we deduce unioned output is (a0, a, b, c)?