opt: enforce consistent input ordering for streaming set ops - #174764
Open
Alignyx wants to merge 1 commit into
Open
opt: enforce consistent input ordering for streaming set ops#174764Alignyx wants to merge 1 commit into
Alignyx wants to merge 1 commit into
Conversation
An INTERSECT or EXCEPT output can inherit column equivalences from only one input. Passing those ordering-choice groups to both inputs can accept an ordering on the other input that does not satisfy the concrete merge ordering used by execution. This causes missing or extra results in the vectorized engine and badly ordered input errors in the row engine. Choose a concrete common streaming ordering before padding all output columns and mapping it to the inputs. Each input can still simplify the requirement using its own functional dependencies. Apply the same rule to explicit streaming variants without an external ordering, preserving unordered hash operations and the UNION ALL ordered synchronizer. Add ordering-invariant tests for both inputs and result regressions for equivalence, intersection/difference, duplicates, NULLs, and descending order. Preserve existing streaming exploration and valid output FDs. Resolves: cockroachdb#144925 Epic: none Release note (bug fix): Fixed incorrect results or badly ordered input errors from INTERSECT and EXCEPT when column equivalences allowed their inputs to use inconsistent orderings for a streaming set operation.
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary and scope
Fix the optimizer/executor ordering contract for streaming set operations when an
output functional dependency (FD) holds on only one input. In #144925, this lets
the two inputs use inconsistent sort keys: the vectorized engine returns an
incorrect intersection, while the row engine detects badly ordered input.
This is a localized correctness repair in
pkg/sql/opt/ordering/set.go, withordering-invariant and SQL regressions. It preserves valid output FDs, existing
streaming exploration, unordered hash operations, and the special
UNION ALLordered-synchronizer behavior. It does not implement the broader bottom-up
planner redesign proposed in the issue discussion.
Problem and expected result
The following reduced setup preserves the report's
INT2input and explicitINT8output casts:The left relation contains
(32,32)and(-129,-129); the right contains(32,NULL)and(-129,-129). Their distinct intersection must therefore beexactly
(-129,-129). The baseline vectorized query instead returns no rows.This expected result follows directly from the input tuples, not from assuming
that a particular execution plan is a trustworthy oracle.
The same ordering-contract failure can affect
EXCEPTandALLvariants: amerge algorithm cannot reliably compute membership or multiplicity when an input
does not satisfy the comparison order it actually uses.
Metamorphic relations used to localize the defect
The investigation kept data, predicates, and output types fixed, transformed the
query into equivalent forms, and compared both results and execution plans. Four
controls independently preserve the expected distinct intersection:
MR1: remove redundant input DISTINCT
For ordinary, duplicate-eliminating
INTERSECT,R INTERSECT DISTINCT(S)is equivalent toR INTERSECT S:Removing the right-side
DISTINCTchanges planning opportunities but cannotchange set membership. This relation must not be applied unconditionally to
INTERSECT ALL, where removingDISTINCTcan change multiplicities.MR2: change only the access path
In W, replace the right-side
FROM t5withFROM t5@primary, leaving itsDISTINCT, predicates, and casts unchanged. The index hint changes how the samerelation is read, not its logical contents. This control is particularly useful
for separating an ordering-dependent failure from incorrect set semantics.
MR3: express intersection as null-safe membership
Left-side
DISTINCTpreserves the output's set semantics. Each output componentuses
IS NOT DISTINCT FROM, rather than ordinary equality, to preserve theNULL-equality semantics of set operations. This is a structurally different
membership computation, not another set-op plan used as the sole oracle.
MR4: introduce a materialization boundary
For this pure query, materialization changes the optimization boundary while
preserving the relation and output types. This claim does not generalize without
qualification to volatile expressions or evaluation-order-dependent errors.
Observed results
Three complete baseline runs and three complete candidate runs agreed:
(-129,-129)(-129,-129)(-129,-129)vectorize = off(-129,-129), no errorvectorize = on,distsql = always(-129,-129)Only W was repeated under the last two settings; this is not a claim that every
control was run under every engine/distribution combination.
distsql = alwaysalso does not by itself establish remote multi-node execution.
Root cause and how it was identified
The left projection emits the same expression twice, so its two output columns
are equivalent. The intersection output is a subset of that input, so the
equivalence remains a valid output FD. The right input's two columns are not
equivalent:
(32,NULL)is a counterexample.The first invalid transition is passing a parent/output ordering choice to both
inputs as though its freedoms were valid on each input. In the captured optimizer
plan, output columns
(17,18)are equivalent and the required ordering becomes+(17|18): either output column is acceptable. The left input sorts its duplicatedexpression, but the right input, with output columns
(16,10), satisfies thechoice using
+10, its second output column, via the(a4,a2)index.Execution does not independently choose a different merge key for each side.
StreamingSetOpOrderingproduces one concrete ordering; execbuilder passes itto
ConstructStreamingSetOp, and the physical planner assigns the samemergeOrderingto bothMergeJoinerSpec.LeftOrderingandRightOrdering.For W, that concrete comparison starts with the first output column.
Consequently, the right input can arrive as:
That is valid for the chosen second-column index order, but invalid for the
executor's first-column ascending comparison. The row engine exposes the mismatch:
EXPLAIN (OPT, VERBOSE)exposed the FD/order-choice mismatch;EXPLAIN (VERBOSE)andEXPLAIN ANALYZEconnected it to the physical path:emits zero rows.
final sort, reads the same two rows on each input, and emits one row.
emits the expected row.
The equal scan cardinalities, concrete tuple counterexample, row-engine diagnostic,
and source-level ordering contract point to an invalid ordering guarantee, rather
than omitted scan data or an incorrect FD derivation rule. The candidate plan for
this particular query is not evidence of an executed repaired streaming plan;
the ordering-invariant tests and unchanged positive streaming fixtures provide
additional coverage of that path.
How the repair works
The shared construction in
setOpBuildRequirednow does the following:ordering, as before.
UNION ALLstreaming paths, executeresult.FromOrdering(result.ToOrdering()). This selects a concrete permittedordering, preserving directions while replacing equivalence groups with
singleton choices and removing output-only optional-column freedom.
streaming comparison has a full-column ordering.
setOpBuildChildReqOrderingmay simplify it using only that child's FDs.An equivalence or constant on one side cannot weaken the other side's contract.
StreamingSetOpOrdering, keeping childrequirements aligned with the concrete order delivered to execution.
The early return for
required.Any()is also narrowed. An explicitly orderedstreaming variant must pass through this construction even when its parent has
no external ordering requirement. Truly unordered paths, where both orderings
are
Any, remain unchanged, as does theUNION ALLspecial case.The key invariant is: each child's required ordering, interpreted using that
child's own FDs, must imply the corresponding mapping of the executor's common
merge ordering. Choosing one representative is sufficient for correctness:
it satisfies the permitted output ordering, and both children must now actually
support it. Recovering the parent's original, unsimplified
ORDER BYis notnecessary for this invariant, although preserving more ordering alternatives
may matter for plan quality.
No FD derivation, type coercion, set-operation multiplicity rule, or execution
operator is changed. Existing set-op execution retains
NullEquality: true.Relationship to the discussion and previous fixes
This addresses the discussion's output-FD/child-ordering diagnosis,
including the fact that the output FDs themselves are correct. The SQL regression
also includes the equality-filter-derived, no-right-side-
DISTINCTshape from theadditional reproduction;
the fix is not conditional on the original
DISTINCTsyntax.Relevant history guided both the implementation boundary and regression coverage:
assigned distinct output column IDs when a left-input column is projected more
than once, while preserving valid output equivalences. This patch does not
revert those properties.
stopped simplifying the common streaming order with output FDs that need not
hold on both inputs. This repair closes the related gap where equivalence
groups and optional columns already embedded in an ordering choice still
transmit that unsafe freedom.
restricted interesting orderings before remapping to avoid invalid streaming
alternatives. That behavior and genuinely unordered hash paths are preserved.
The latest design discussion
proposes a merge-join-style bottom-up approach using interesting orderings.
GenerateStreamingSetOpalready consults bottom-up interesting orderings; thispatch retains it, but also retains implicit streaming driven by parent ordering
requirements. It restores a concrete common-key contract at the existing boundary,
not the proposed planner representation/enumeration redesign. There is a real
scope difference to review, not a claim that the architectural discussion is
fully resolved.
Completed local validation
All results below refer to the existing repair commit
1f04f4c498931d10074acd00b15f706b42798124, based on8812064a015d2faf99d3fc7e15880f94042954b0. They are completed local validation,not a claim that upstream PR CI has passed.
TestSetOpOrdering144925covers 12 scenariosand 38 operator variants, including one-sided equivalences/constants,
nontrivial output/input column mapping, internal-only ordering, descending
prefixes, hash paths,
UNION,UNION ALL, and non-mutation. Beyond expectedstrings, it asserts
childReq.Implies(mergeReq)after mapping the actualStreamingSetOpOrderingand simplifying with the relevant child's FDs.The unchanged tests fail on baseline production code with actual implication
failures and pass on the candidate.
unionlogic test cover theoriginal
DISTINCTshape, equality-derived equivalence without thatDISTINCT,EXCEPT,INTERSECT ALL,EXCEPT ALL, NULL-containing inputs, and orderingthrough the other equivalent output column in descending direction. The frozen
regression fails on baseline and passes on the candidate. Baseline execution
stops at the first failing original query, so later queries are not claimed
to have separate observed native-red runs. These queries use
rowsort;their result checks do not establish the returned row order.
agree with the observations above. The replay verifier checks results, the
exact baseline-only row error, plan changes, and actual scan cardinalities;
it also rejects four deliberately corrupted in-memory evidence variants.
with expected multisets computed independently from those rows: minimum counts
for
INTERSECT ALL, nonnegative count differences forEXCEPT ALL, and countsums for
UNION ALL. Candidate results match all 18 cases with zero errors;the four baseline wrong-result bags and one row-engine ordering error are
repaired. These checks also validate actual ASC/DESC/NULL output ordering
where specified, unlike the native
rowsortchecks.execbuilder unit targets; the separate complete
TestExecBuild_unionplanfixture; complete
union/distsql_unionlogic suites underlocal,local-vec-off,fakedist,fakedist-vec-off, andfakedist-disk; and thefull SQL test target across 16 shards all passed locally. Existing opt: support streaming set ops when ordering not required #64062
streaming-positive, opt: invalid stream ordering results in incorrect results for set ops #69497 single-side-key, and opt: internal error in set operations due to stream ordering not including all columns #68702 duplicate-mapping
fixtures passed without changing their expected plans.
The committed changes are limited to
set.go, the newset_test.go, its Bazelregistration, and the SQL
unionregression: four files, +239/-1. Supplementalmetamorphic and held-out logs/verifiers are retained in the local investigation
record; they are not additional files in this PR.
Current limitations and follow-up questions
Implicit top-down streaming remains available, subject to the strengthened
common-order contract.
strengthen child requirements, require a sort, or change hash-versus-streaming
selection. No performance benchmark or search-space completeness proof is
supplied. The observed hash plan is disclosed above; the unchanged streaming
fixtures do not establish an absence of performance regressions.
and ordering propagation in every relational operator, nor an exhaustive
proof for every future FD/ordering combination. A different operator with a
similar unsafe boundary would need its own analysis.
and candidate, not a backport certification for every affected release or a
production multi-node workload/performance qualification.
Would maintainers prefer this localized ordering-contract fix as an initial
correctness repair, with the bottom-up redesign tracked separately, or want that
redesign included before landing? If follow-up is desired, a possible direction
is to bind explicit concrete common comparison keys to streaming alternatives
derived from child interesting orderings, then derive each input requirement
using only its own FDs, as merge joins do. Retaining the new implication tests
and benchmarking representative sort/hash/streaming choices would help evaluate
that work. This is a proposed follow-up direction, not an implemented or
performance-validated part of this PR.
Resolves: #144925
Epic: none
Release note (bug fix): Fixed incorrect results or badly ordered input
errors from INTERSECT and EXCEPT when column equivalences allowed their
inputs to use inconsistent orderings for a streaming set operation.