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.

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

Conversation

wiedld
Copy link
Contributor

@wiedld wiedld commented Sep 10, 2024

Which issue does this PR close?

Resolves #12446

We have failures caused by SanityCheckPlan not considering the constants in the UnionExec input orderings. We made an exact reproducer in the first commit's test case.

Rationale for this change

Given the following scenario:

ParentWithSort [proj1 sorted, proj2 sorted]
     UnionExec
           InputWithSort [proj1 sorted] proj2 is constant
           InputWithSort [proj2 sorted] proj1 is constant

The SanityCheckPlan was failing, because the UnionExec has the following orderings vs constants:

oeq_class: OrderingEquivalenceClass { 
        orderings: [
            [proj1 sorted], 
            [proj2 sorted]
        ] 
    }, 
    constants: [], 
    ...
}

which means that no single ordering (minus constants) could fulfill the [proj1 sorted, proj2 sorted] sort requirement. Because the UnionExec has it's input orderings but not it's input constants.

What changes are included in this PR?

I elected to perform the comparisons (during the SanityPlanCheck) between the UnionExec's parent and children, skipping the union exec itself.

Alternatively, I could have elected to:

  • modify the UnionExec's equivalence properties output ordering:
    • to make it `orderings: [[proj1 sorted, proj2-constant sorted], [proj1-contant sorted, proj2 sorted]]
    • But that adds constants to ordering. 🤔
  • I wasn't going to add to the UnionExec's constants, since neither projections are constant across the entire union.
  • modify the contract for EquivalenceProperties::ordering_satisfy_requirement

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Sep 10, 2024
@wiedld wiedld force-pushed the iox-11945/sanity-check-plan-sort-pushdown branch from b5e60ab to a26c49e Compare September 10, 2024 04:20
@wiedld wiedld force-pushed the iox-11945/sanity-check-plan-sort-pushdown branch from a26c49e to 782e18d Compare September 10, 2024 04:28
@berkaysynnada
Copy link
Contributor

Thanks @wiedld for the detailed description of the problem. Once it's ready, I will review it thoroughly.

@berkaysynnada
Copy link
Contributor

berkaysynnada commented Sep 10, 2024

When I briefly look at the solution, I actually think we shouldn't patch the sanity checker to relax it. A more accurate solution seems to be your alternate proposal:

modify the UnionExec's equivalence properties output ordering:
to make it `orderings: [[proj1 sorted, proj2-constant sorted], [proj1-contant sorted, proj2 sorted]]
But that adds constants to ordering. 🤔
I wasn't going to add to the UnionExec's constants, since neither projections are constant across the entire union.

You can actually add constants, as there is a flag, "across_partitions," that indicates whether the value is constant across all partitions or only in its corresponding partition. SortPreservingMergeExec and CoalescePartitionsExec refer to this flag later when coalescing partitions.

However, after adding constants, I am not sure if ordering_satisfy_requirement still needs a refactor.

@alamb
Copy link
Contributor

alamb commented Sep 10, 2024

I agree with @berkaysynnada that we should fix the calculation rather than relax the sanity checker in DataFusion

We (already) patch DataFusion to skip the sanity check for certain plan nodes so we aren't blocked downstream. I think we should focus on the "right"fix that allows the sanity checker to run but also correctly recognize the plan is valid

@github-actions github-actions bot added physical-expr Physical Expressions and removed core Core DataFusion crate labels Sep 10, 2024
    caveat: this has an unintended side effect, as the EnforceSorting removes the sort_expr from one input/side of the UnionExec (where it's not constant)
@wiedld wiedld force-pushed the iox-11945/sanity-check-plan-sort-pushdown branch from d928d04 to 4bd4db0 Compare September 10, 2024 23:16
Comment on lines 1223 to 1226
04)------SortExec: TopK(fetch=2), expr=[d@4 ASC NULLS LAST,c@1 ASC NULLS LAST,a@2 ASC NULLS LAST,b@0 ASC NULLS LAST], preserve_partitioning=[false]
04)------SortExec: TopK(fetch=2), expr=[d@4 ASC NULLS LAST,c@1 ASC NULLS LAST,b@0 ASC NULLS LAST], preserve_partitioning=[false]
05)--------ProjectionExec: expr=[b@1 as b, c@2 as c, a@0 as a, NULL as a0, d@3 as d]
06)----------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a, b, c, d], output_ordering=[c@2 ASC NULLS LAST], has_header=true
07)------SortExec: TopK(fetch=2), expr=[d@4 ASC NULLS LAST,c@1 ASC NULLS LAST,a0@3 ASC NULLS LAST,b@0 ASC NULLS LAST], preserve_partitioning=[false]
Copy link
Contributor Author

@wiedld wiedld Sep 10, 2024

Choose a reason for hiding this comment

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

you can actually add constants, as there is a flag, "across_partitions," that indicates whether the value is constant across all partitions or only in its corresponding partition

Made the change per suggestion (demonstration only, not final commits), and I'm not sure this is the proper fix. If I add the constants on the union's equivalence properties, there are other ramifications because:

In the example above, the sort orders [a@2 ASC NULLS LAST] and [a0@3 ASC NULLS LAST] are removed on non-constant projections. The EnforceSorting optimization adds (and pushes down) the SortExecs, but the change itself really occurs based upon the EquivalenceProperties's definition of a (non-constant) sort order. Since the UnionExec listed certain constants -- they are removed from the sort order.

I started hacking around this in the EnforceSorting, but it feels like the suggested change (adding to constants) may be actually changing the definition of what the constants are? 🤔 Am I heading in the right direction here?

@berkaysynnada
Copy link
Contributor

I think I misled you unintentionally. The part we need to focus on is

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);
}
}

When I think about what we need to obtain as the finest ordering from the union of two children:

LEFT: orderings: [[a]], constants: [c]
RIGHT: orderings: [[c]], constants: [a]

Then the union should have: orderings: [[a, c], [c, a]], constants: [].

To get this result, we need to keep track of the left and right constants of the children, which are not allowed to be placed in the union's constants. Going back to my example above, these child-specific constants can be treated as:

LEFT: orderings: [[a]], constants: [c] → orderings: [[a, c], [c, a]], constants: []
RIGHT: orderings: [[c]], constants: [a] → orderings: [[c, a], [a, c]], constants: []

If we reapply the ordering calculation part I shared above on these updated child properties, we should get what we need: orderings: [[a, c], [c, a]], which would pass the sanity check for the [a, c] ordering.

@alamb
Copy link
Contributor

alamb commented Sep 12, 2024

I filed #12446 to track this issue

@wiedld
Copy link
Contributor Author

wiedld commented Sep 19, 2024

I pushed up a possible solution, which has a slight change to the test case output ordering as per this commit.

Please lmk @berkaysynnada if this is the solution we are seeking. 🙏🏼
Then I'll finish up the needed updates to the EnforcedSorting pass.

/// child2 = order by a, b, c
/// => union's joint order is a0, a, b, c.
fn calculate_joint_ordering(
lhs: &EquivalenceProperties,
Copy link
Contributor

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)?

// Expected
vec![],
// Expected: union sort orders
vec![vec!["a", "b", "c"]],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these test changes are not correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree -- these changes make the expected output incorrect

Copy link
Contributor

@berkaysynnada berkaysynnada left a comment

Choose a reason for hiding this comment

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

I believe you don't need to add a new logic for sort expr's. You should only focus on the following conversion, when constant expressions appear as the global order of the other children:

LEFT: orderings: [[a]], constants: [c] → orderings: [[a, c], [c, a]], constants: []
RIGHT: orderings: [[c]], constants: [a] → orderings: [[c, a], [a, c]], constants: []

When such an ordering and constant properties exist in children (LEFT: orderings: [[a]], constants: [c], RIGHT: orderings: [[c]], constants: [a]), we should treat them as if the ordering is permutation of existing orderings and the constant.

@wiedld
Copy link
Contributor Author

wiedld commented Sep 19, 2024

I believe you don't need to add a new logic for sort expr's. You should only focus on the following conversion, when constant expressions appear as the global order of the other children:

LEFT: orderings: [[a]], constants: [c] → orderings: [[a, c], [c, a]], constants: []
RIGHT: orderings: [[c]], constants: [a] → orderings: [[c, a], [a, c]], constants: []

I think this is due to how I explained the issue. The actual children are:

LEFT: orderings: [[a]], constants: [], NULL as c (but is not named as a constant on the child)
RIGHT: orderings: [[c]], constants: [], NULL as a (but is not named as a constant on the child)

That comes out of this query reproducer, where we have two fields a and a0, which are nulls on alternating sides:

select * from (select b, c, a, NULL::int as a0 from ordered_table order by a, c) t1
union all
select * from (select b, c, NULL::int as a, a0 from ordered_table order by a0, c) t2
order by d, c, a, a0, b
limit 2;

I added to the complexity by trying to handle a generalized case, and misusing the constant term (which is very specific for our equivlance props). I think what we want is a special casing for this exact scenario.

@alamb
Copy link
Contributor

alamb commented Sep 19, 2024

I looked at this problem some more (not really the code, but just a smaller reproducer) and left my notes here #12446 (comment) and here #12446 (comment). TLDR is it is not clear to me the issue is with the union properties calculation 🤔

@alamb
Copy link
Contributor

alamb commented Sep 19, 2024

I plan to take over this PR / fix (likely tomorrow) -- I will keep #12446 updated as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SanityChecker rejects certain valid UNION plans
3 participants