From 1f04f4c498931d10074acd00b15f706b42798124 Mon Sep 17 00:00:00 2001 From: Alignyx <317810310+Alignyx@users.noreply.github.com> Date: Sat, 5 Sep 2026 15:16:39 +0800 Subject: [PATCH] opt: enforce consistent input ordering for streaming set ops 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: #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. --- pkg/sql/logictest/testdata/logic_test/union | 60 +++++++ pkg/sql/opt/ordering/BUILD.bazel | 1 + pkg/sql/opt/ordering/set.go | 9 +- pkg/sql/opt/ordering/set_test.go | 170 ++++++++++++++++++++ 4 files changed, 239 insertions(+), 1 deletion(-) create mode 100644 pkg/sql/opt/ordering/set_test.go diff --git a/pkg/sql/logictest/testdata/logic_test/union b/pkg/sql/logictest/testdata/logic_test/union index da1185d9a240..cf9166cb4009 100644 --- a/pkg/sql/logictest/testdata/logic_test/union +++ b/pkg/sql/logictest/testdata/logic_test/union @@ -799,3 +799,63 @@ TABLE t130591_1 INTERSECT TABLE t130591_1; statement count 1 TABLE t130591_1 UNION TABLE t130591_1; + +subtest regression_144925 + +statement ok +CREATE TABLE t144925 (a INT2, b INT8, INDEX (b, a)) + WITH (sql_stats_automatic_collection_enabled = false); +INSERT INTO t144925 VALUES (32, NULL), (-129, -129); + +# The two output columns are equivalent only on the left. The right input +# cannot satisfy a first-column merge ordering by ordering its second column. +query II rowsort +SELECT a::INT8, a::INT8 FROM t144925 WHERE a IS NOT NULL +INTERSECT SELECT DISTINCT a::INT8, b FROM t144925 WHERE a IS NOT NULL +ORDER BY 1; +---- +-129 -129 + +query II rowsort +SELECT a::INT8, a::INT8 FROM t144925 WHERE a IS NOT NULL +EXCEPT SELECT DISTINCT a::INT8, b FROM t144925 WHERE a IS NOT NULL +ORDER BY 1; +---- +32 32 + +# An equality filter also establishes output equivalence, without DISTINCT. +query II rowsort +SELECT b, c FROM (SELECT a::INT8 AS c, b FROM t144925) WHERE c = b +INTERSECT SELECT a::INT8, b FROM t144925 +ORDER BY 1; +---- +-129 -129 + +# Preserve multiplicities when the left input has duplicate projected rows. +statement ok +INSERT INTO t144925 VALUES (-129, -129), (NULL, NULL); + +query II rowsort +SELECT a::INT8, a::INT8 FROM t144925 +INTERSECT ALL SELECT DISTINCT a::INT8, b FROM t144925 +ORDER BY 1; +---- +NULL NULL +-129 -129 + +query II rowsort +SELECT a::INT8, a::INT8 FROM t144925 +EXCEPT ALL SELECT DISTINCT a::INT8, b FROM t144925 +ORDER BY 1; +---- +-129 -129 +32 32 + +# An ordering on the other equivalent output column is equally valid. +query II rowsort +SELECT a::INT8, a::INT8 FROM t144925 +INTERSECT SELECT DISTINCT a::INT8, b FROM t144925 +ORDER BY 2 DESC; +---- +NULL NULL +-129 -129 diff --git a/pkg/sql/opt/ordering/BUILD.bazel b/pkg/sql/opt/ordering/BUILD.bazel index 072c3e8d83f8..3e074ca200b2 100644 --- a/pkg/sql/opt/ordering/BUILD.bazel +++ b/pkg/sql/opt/ordering/BUILD.bazel @@ -50,6 +50,7 @@ go_test( "project_test.go", "row_number_test.go", "scan_test.go", + "set_test.go", ], embed = [":ordering"], deps = [ diff --git a/pkg/sql/opt/ordering/set.go b/pkg/sql/opt/ordering/set.go index 6f14dd85d92c..5c7d8cd87571 100644 --- a/pkg/sql/opt/ordering/set.go +++ b/pkg/sql/opt/ordering/set.go @@ -63,7 +63,7 @@ func setOpBuildProvided(expr memo.RelExpr, required *props.OrderingChoice) opt.O // operation if the ordering involves all columns. func setOpBuildRequired(expr memo.RelExpr, required *props.OrderingChoice) *props.OrderingChoice { private := expr.Private().(*memo.SetPrivate) - if required.Any() { + if required.Any() && (private.Ordering.Any() || expr.Op() == opt.UnionAllOp) { return &private.Ordering } @@ -79,6 +79,13 @@ func setOpBuildRequired(expr memo.RelExpr, required *props.OrderingChoice) *prop return &result } + // Equivalences and optional columns in the output ordering need not hold on + // both inputs (for example, INTERSECT can inherit an equivalence from only + // one input). Choose a concrete ordering before adding the remaining columns, + // so both inputs must satisfy the same ordering used by the execution engine. + // setOpBuildChildReqOrdering can then simplify it using each input's own FDs. + result.FromOrdering(result.ToOrdering()) + // If required includes some columns but not all, add the remaining columns in // an arbitrary (but deterministic) order. missing := expr.Relational().OutputCols.Difference(result.ColSet()) diff --git a/pkg/sql/opt/ordering/set_test.go b/pkg/sql/opt/ordering/set_test.go new file mode 100644 index 000000000000..c2744f89caa0 --- /dev/null +++ b/pkg/sql/opt/ordering/set_test.go @@ -0,0 +1,170 @@ +// Copyright 2026 The Cockroach Authors. +// +// Use of this software is governed by the CockroachDB Software License +// included in the /LICENSE file. + +package ordering + +import ( + "context" + "fmt" + "testing" + + "github.com/cockroachdb/cockroach/pkg/settings/cluster" + "github.com/cockroachdb/cockroach/pkg/sql/opt" + "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" + "github.com/cockroachdb/cockroach/pkg/sql/opt/norm" + "github.com/cockroachdb/cockroach/pkg/sql/opt/props" + "github.com/cockroachdb/cockroach/pkg/sql/opt/testutils/testcat" + "github.com/cockroachdb/cockroach/pkg/sql/opt/testutils/testexpr" + "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" + "github.com/cockroachdb/cockroach/pkg/sql/types" +) + +func TestSetOpOrdering144925(t *testing.T) { + intersectOps := []opt.Operator{opt.IntersectOp, opt.IntersectAllOp} + filterOps := []opt.Operator{opt.IntersectOp, opt.IntersectAllOp, opt.ExceptOp, opt.ExceptAllOp} + allOps := []opt.Operator{ + opt.IntersectOp, opt.IntersectAllOp, opt.ExceptOp, opt.ExceptAllOp, + opt.UnionOp, opt.UnionAllOp, + } + testCases := []struct { + name string + ops []opt.Operator + required string + internal string + leftFD string + rightFD string + streaming string + children [2]string + }{ + { + name: "left-equivalence", ops: filterOps, + required: "+(7|8)", leftFD: "equivalence", + streaming: "+7,+8,+9", children: [2]string{"+(1|3),+2", "+6,+4,+5"}, + }, + { + name: "right-equivalence", ops: intersectOps, + required: "+(7|8)", rightFD: "equivalence", + streaming: "+7,+8,+9", children: [2]string{"+3,+1,+2", "+(4|6),+5"}, + }, + { + name: "left-constant", ops: filterOps, + required: "+8 opt(7)", leftFD: "constant", + streaming: "+8,+7,+9", children: [2]string{"+1,+2 opt(3)", "+4,+6,+5"}, + }, + { + name: "right-constant", ops: intersectOps, + required: "+8 opt(7)", rightFD: "constant", + streaming: "+8,+7,+9", children: [2]string{"+1,+3,+2", "+4,+5 opt(6)"}, + }, + { + name: "internal-equivalence", ops: filterOps, + internal: "+(7|8),+9", leftFD: "equivalence", + streaming: "+7,+9,+8", children: [2]string{"+(1|3),+2", "+6,+5,+4"}, + }, + { + name: "internal-constant", ops: filterOps, + internal: "+8,+7,+9", leftFD: "constant", + streaming: "+8,+7,+9", children: [2]string{"+1,+2 opt(3)", "+4,+6,+5"}, + }, + { + name: "required-and-internal", ops: filterOps, + required: "+(7|8)", internal: "+(7|8),-9", leftFD: "equivalence", + streaming: "+7,-9,+8", children: [2]string{"+(1|3),-2", "+6,-5,+4"}, + }, + { + name: "hash", ops: allOps, + streaming: "", children: [2]string{"", ""}, + }, + { + name: "descending-prefix", ops: allOps[:len(allOps)-1], + required: "-8", + streaming: "-8,+7,+9", children: [2]string{"-1,+3,+2", "-4,+6,+5"}, + }, + { + name: "union-shared-equivalence", ops: []opt.Operator{opt.UnionOp}, + required: "+(7|8)", leftFD: "equivalence", rightFD: "equivalence", + streaming: "+7,+8,+9", children: [2]string{"+(1|3),+2", "+(4|6),+5"}, + }, + { + name: "union-all-prefix", ops: []opt.Operator{opt.UnionAllOp}, + required: "-8", + streaming: "-8", children: [2]string{"-1", "-4"}, + }, + { + name: "union-all-shared-equivalence", ops: []opt.Operator{opt.UnionAllOp}, + required: "+(7|8)", leftFD: "equivalence", rightFD: "equivalence", + streaming: "+7", children: [2]string{"+(1|3)", "+(4|6)"}, + }, + } + for _, tc := range testCases { + for _, op := range tc.ops { + t.Run(fmt.Sprintf("%s/%s", tc.name, op), func(t *testing.T) { + st := cluster.MakeTestingClusterSettings() + evalCtx := eval.NewTestingEvalContext(st) + var f norm.Factory + f.Init(context.Background(), evalCtx, testcat.New()) + for i := 1; i <= 9; i++ { + f.Metadata().AddColumn(fmt.Sprintf("c%d", i), types.Int) + } + + // Output 7,8,9 maps to left 3,1,2 and right 6,4,5. Neither + // column IDs nor the lists' positions determine the merge order. + private := memo.SetPrivate{ + OutCols: opt.ColList{8, 7, 9}, LeftCols: opt.ColList{1, 3, 2}, + RightCols: opt.ColList{4, 6, 5}, Ordering: props.ParseOrderingChoice(tc.internal), + } + makeInput := func(cols opt.ColList, fd string) *testexpr.Instance { + input := &testexpr.Instance{Rel: &props.Relational{OutputCols: cols.ToSet()}} + switch fd { + case "equivalence": + input.Rel.FuncDeps.AddEquivalency(cols[0], cols[1]) + case "constant": + input.Rel.FuncDeps.AddConstants(opt.MakeColSet(cols[1])) + } + return input + } + left := makeInput(private.LeftCols, tc.leftFD) + right := makeInput(private.RightCols, tc.rightFD) + constructors := map[opt.Operator]func(memo.RelExpr, memo.RelExpr, *memo.SetPrivate) memo.RelExpr{ + opt.IntersectOp: f.Memo().MemoizeIntersect, + opt.IntersectAllOp: f.Memo().MemoizeIntersectAll, + opt.ExceptOp: f.Memo().MemoizeExcept, + opt.ExceptAllOp: f.Memo().MemoizeExceptAll, + opt.UnionOp: f.Memo().MemoizeUnion, + opt.UnionAllOp: f.Memo().MemoizeUnionAll, + } + expr := constructors[op](left, right, &private) + required := props.ParseOrderingChoice(tc.required) + requiredBefore := required.String() + privateBefore := fmt.Sprintf("%+v", expr.Private()) + + streaming := StreamingSetOpOrdering(expr, &required) + if got := streaming.String(); got != tc.streaming { + t.Errorf("streaming ordering: expected %q, got %q", tc.streaming, got) + } + for childIdx, cols := range []opt.ColList{private.LeftCols, private.RightCols} { + childReq := setOpBuildChildReqOrdering(expr, &required, childIdx) + if got := childReq.String(); got != tc.children[childIdx] { + t.Errorf("child %d: expected %q, got %q", childIdx, tc.children[childIdx], got) + } + + // Each child's requirement must guarantee the executor's same + // concrete merge order. Only that child's FDs can justify + // omitting a column or choosing an equivalent column instead. + var mergeReq props.OrderingChoice + mergeReq.FromOrdering(streaming) + mergeReq = mergeReq.RemapColumns(private.OutCols, cols) + mergeReq.Simplify(&expr.Child(childIdx).(memo.RelExpr).Relational().FuncDeps) + if !childReq.Implies(&mergeReq) { + t.Errorf("child %d: %s does not guarantee merge ordering %s", childIdx, childReq, mergeReq) + } + } + if required.String() != requiredBefore || fmt.Sprintf("%+v", expr.Private()) != privateBefore { + t.Fatal("building a set ordering mutated the required ordering or set private") + } + }) + } + } +}