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