SanityCheckPlan should compare UnionExec inputs to requirements for output (parent).

[wiedld]

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.

[berkaysynnada]

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

[berkaysynnada]

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]

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

[wiedld]

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:

• the EquivalenceProperties::output_ordering remove the constants
• the EquivalenceProperties::normalized_oeq_class remove the constants

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]

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

datafusion/datafusion/physical-expr/src/equivalence/properties.rs

Lines 1559 to 1578 in 3ece7a7

	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]

I filed #12446 to track this issue

[wiedld]

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.

[berkaysynnada]

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

[berkaysynnada]

I think these test changes are not correct.

[alamb]

I agree -- these changes make the expected output incorrect

[berkaysynnada]

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]

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]

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]

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