From ecfff8ca6d9b453949583c8db093443bc440ed52 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Wed, 28 Apr 2021 10:21:08 -0700 Subject: [PATCH] opt: remove non-equivalent group columns in OrderingChoice.Simplify Previously, `OrderingChoice.Simplify` would add but never remove columns from ordering column groups based on equivalency in an FD. In rare cases, this could cause the optimizer to generate expressions which violated an invariant that all columns in an ordering column group are equivalent according to the expression's FD. Violation of this invariant only panics in test builds, and in the test cases found that trigger this panic, there is likely no correctness issues with the expression. Therefore, there was probably no impact in any release builds. This commit updates `OrderingChoice.Simplify` so that non-equivalent columns in an ordering column group are removed from the group, satisfying the invariant. Fixes #63794 Release note: None --- pkg/sql/opt/memo/expr_format.go | 4 +- pkg/sql/opt/norm/testdata/rules/inline | 57 +++++++++++++++++++ pkg/sql/opt/props/physical/ordering_choice.go | 23 +++++--- .../props/physical/ordering_choice_test.go | 56 ++++++++++++------ 4 files changed, 111 insertions(+), 29 deletions(-) diff --git a/pkg/sql/opt/memo/expr_format.go b/pkg/sql/opt/memo/expr_format.go index da5b17028fa1..58c07de5e85a 100644 --- a/pkg/sql/opt/memo/expr_format.go +++ b/pkg/sql/opt/memo/expr_format.go @@ -745,9 +745,9 @@ func (f *ExprFmtCtx) formatRelational(e RelExpr, tp treeprinter.Node) { reqStr := required.Ordering.String() provStr := provided.String() if provStr == reqStr { - tp.Childf("ordering: %s", required.Ordering.String()) + tp.Childf("ordering: %s", reqStr) } else { - tp.Childf("ordering: %s [actual: %s]", required.Ordering.String(), provided.String()) + tp.Childf("ordering: %s [actual: %s]", reqStr, provStr) } } } diff --git a/pkg/sql/opt/norm/testdata/rules/inline b/pkg/sql/opt/norm/testdata/rules/inline index 100be311846e..26f967dd9360 100644 --- a/pkg/sql/opt/norm/testdata/rules/inline +++ b/pkg/sql/opt/norm/testdata/rules/inline @@ -848,6 +848,63 @@ semi-join (hash) └── filters └── v:5 = s:11 [outer=(5,11), constraints=(/5: (/NULL - ]; /11: (/NULL - ]), fd=(5)==(11), (11)==(5)] +# Regression test for #63794. Inlining filters on virtual columns during +# exploration should not violate the ordering choice that all columns in an +# ordering column group should be equal according to the expression's FD. +exec-ddl +CREATE TABLE t63794_a (a INT) +---- + +exec-ddl +CREATE TABLE t63794_bc (b INT, c INT AS (b % 2) VIRTUAL, INDEX (b)) +---- + +opt format=hide-all +SELECT a.a +FROM t63794_a AS a + JOIN t63794_bc AS bc1 + JOIN t63794_bc AS bc2 ON true + ON a.a = bc1.b AND + bc1.b = bc2.b AND + bc1.c = bc2.c + JOIN t63794_bc AS bc3 ON + bc1.b = bc3.b AND bc2.b = bc3.c +---- +project + └── inner-join (hash) + ├── scan t63794_a [as=a] + ├── inner-join (hash) + │ ├── project + │ │ ├── scan t63794_bc + │ │ │ └── computed column expressions + │ │ │ └── c + │ │ │ └── b % 2 + │ │ └── projections + │ │ └── b % 2 + │ ├── inner-join (hash) + │ │ ├── project + │ │ │ ├── scan t63794_bc + │ │ │ │ └── computed column expressions + │ │ │ │ └── c + │ │ │ │ └── b % 2 + │ │ │ └── projections + │ │ │ └── b % 2 + │ │ ├── project + │ │ │ ├── scan t63794_bc + │ │ │ │ └── computed column expressions + │ │ │ │ └── c + │ │ │ │ └── b % 2 + │ │ │ └── projections + │ │ │ └── b % 2 + │ │ └── filters + │ │ ├── b = b + │ │ └── c = c + │ └── filters + │ ├── b = b + │ └── b = c + └── filters + └── a = b + # -------------------------------------------------- # InlineProjectInProject # -------------------------------------------------- diff --git a/pkg/sql/opt/props/physical/ordering_choice.go b/pkg/sql/opt/props/physical/ordering_choice.go index 6836193a922e..7257f3505aea 100644 --- a/pkg/sql/opt/props/physical/ordering_choice.go +++ b/pkg/sql/opt/props/physical/ordering_choice.go @@ -488,9 +488,10 @@ func (oc *OrderingChoice) Copy() OrderingChoice { // CanSimplify returns true if a call to Simplify would result in any changes to // the OrderingChoice. Changes include additional constant columns, removed -// groups, and additional equivalent columns. This is used to quickly check -// whether Simplify needs to be called without requiring allocations in the -// common case. This logic should be changed in concert with the Simplify logic. +// groups, additional equivalent columns, or removed non-equivalent columns. +// This is used to quickly check whether Simplify needs to be called without +// requiring allocations in the common case. This logic should be changed in +// concert with the Simplify logic. func (oc *OrderingChoice) CanSimplify(fdset *props.FuncDepSet) bool { if oc.Any() { // Any ordering allowed, so can't simplify further. @@ -519,8 +520,8 @@ func (oc *OrderingChoice) CanSimplify(fdset *props.FuncDepSet) bool { return true } - // Check whether new equivalent columns can be added by the FD set. - equiv := fdset.ComputeEquivClosure(group.Group) + // Check whether the equivalency group needs to change based on the FD. + equiv := fdset.ComputeEquivGroup(group.AnyID()) if !equiv.Equals(group.Group) { return true } @@ -534,14 +535,16 @@ func (oc *OrderingChoice) CanSimplify(fdset *props.FuncDepSet) bool { } // Simplify uses the given FD set to streamline the orderings allowed by this -// instance, and to potentially increase the number of allowed orderings: +// instance. It can both increase and decrease the number of allowed orderings: // // 1. Constant columns add additional optional column choices. // // 2. Equivalent columns allow additional choices within an ordering column // group. // -// 3. If the columns in a group are functionally determined by columns from +// 3. Non-equivalent columns in an ordering column group are removed. +// +// 4. If the columns in a group are functionally determined by columns from // previous groups, the group can be dropped. This technique is described // in the "Reduce Order" section of this paper: // @@ -574,8 +577,10 @@ func (oc *OrderingChoice) Simplify(fdset *props.FuncDepSet) { continue } - // Expand group with equivalent columns from FD set. - group.Group = fdset.ComputeEquivClosure(group.Group) + // Set group to columns equivalent to an arbitrary column in the group + // based on the FD set. This can both add and remove columns from the + // group. + group.Group = fdset.ComputeEquivGroup(group.AnyID()) // Add this group's columns and find closure with the new columns. closure = closure.Union(group.Group) diff --git a/pkg/sql/opt/props/physical/ordering_choice_test.go b/pkg/sql/opt/props/physical/ordering_choice_test.go index 435cf0bcea70..8cda7f657c63 100644 --- a/pkg/sql/opt/props/physical/ordering_choice_test.go +++ b/pkg/sql/opt/props/physical/ordering_choice_test.go @@ -344,40 +344,54 @@ func TestOrderingChoice_Copy(t *testing.T) { // ()-->(8) // (3)==(9) // (9)==(3) + // (6)==(7) + // (7)==(6) var fd props.FuncDepSet fd.AddConstants(opt.MakeColSet(8)) fd.AddEquivalency(3, 9) + fd.AddEquivalency(6, 7) copied.Simplify(&fd) - if ordering.String() != "+1,-(2|3) opt(4,5,100)" { - t.Errorf("original was modified: %s", ordering.String()) + original := "+1,-(2|3) opt(4,5,100)" + if ordering.String() != original { + t.Errorf("original %s was modified to %s", original, ordering.String()) } - if copied.String() != "+1,-(2|3|9),-(6|7) opt(4,5,8)" { - t.Errorf("copy is not correct: %s", copied.String()) + expectedCopy := "+1,-2,-(6|7) opt(4,5,8)" + if copied.String() != expectedCopy { + t.Errorf("copy: expected %s, actual %s", expectedCopy, copied.String()) } } func TestOrderingChoice_Simplify(t *testing.T) { // ()-->(4,5) - // (1)==(1,3) - // (2)==(1) - // (3)==(1) + // (1)==(2,3) + // (2)==(1,3) + // (3)==(1,2) var fd1 props.FuncDepSet fd1.AddConstants(opt.MakeColSet(4, 5)) fd1.AddEquivalency(1, 2) fd1.AddEquivalency(1, 3) + // ()-->(2) + // (3)==(4,5) + // (4)==(3,5) + // (5)==(3,4) + var fd2 props.FuncDepSet + fd2.AddConstants(opt.MakeColSet(2)) + fd2.AddEquivalency(3, 4) + fd2.AddEquivalency(3, 5) + // (1)-->(1,2,3,4,5) // (2)-->(4) // (4)-->(5) // (2)==(3) // (3)==(2) - var fd2 props.FuncDepSet - fd2.AddStrictKey(opt.MakeColSet(1), opt.MakeColSet(1, 2, 3, 4, 5)) - fd2.AddSynthesizedCol(opt.MakeColSet(2), 4) - fd2.AddSynthesizedCol(opt.MakeColSet(4), 5) - fd2.AddEquivalency(2, 3) + var fd3 props.FuncDepSet + fd3.AddStrictKey(opt.MakeColSet(1), opt.MakeColSet(1, 2, 3, 4, 5)) + fd3.AddSynthesizedCol(opt.MakeColSet(2), 4) + fd3.AddSynthesizedCol(opt.MakeColSet(4), 5) + fd3.AddEquivalency(2, 3) testcases := []struct { fdset *props.FuncDepSet @@ -397,13 +411,19 @@ func TestOrderingChoice_Simplify(t *testing.T) { {fdset: &fd1, s: "+(4|5|6)", expected: "+6 opt(4,5)"}, {fdset: &fd1, s: "+(4|6)", expected: "+6 opt(4,5)"}, + // Columns removed from ordering groups because they are not equivalent + // in the FD. + {fdset: &fd1, s: "+(2|4|5)", expected: "+(1|2|3) opt(4,5)"}, + {fdset: &fd2, s: "+(1|3)", expected: "+1 opt(2)"}, + {fdset: &fd2, s: "+(1|3|4|5)", expected: "+1 opt(2)"}, + // Columns functionally determine one another. - {fdset: &fd2, s: "", expected: ""}, - {fdset: &fd2, s: "+1,+2,+4", expected: "+1"}, - {fdset: &fd2, s: "+2,+4,+5", expected: "+(2|3)"}, - {fdset: &fd2, s: "+3,+5", expected: "+(2|3)"}, - {fdset: &fd2, s: "-(2|3),+1,+5", expected: "-(2|3),+1"}, - {fdset: &fd2, s: "-(2|4),+5,+1", expected: "-(2|3|4),+1"}, + {fdset: &fd3, s: "", expected: ""}, + {fdset: &fd3, s: "+1,+2,+4", expected: "+1"}, + {fdset: &fd3, s: "+2,+4,+5", expected: "+(2|3)"}, + {fdset: &fd3, s: "+3,+5", expected: "+(2|3)"}, + {fdset: &fd3, s: "-(2|3),+1,+5", expected: "-(2|3),+1"}, + {fdset: &fd3, s: "-(2|4),+5,+1", expected: "-(2|3),+1"}, } for _, tc := range testcases {