From f4ed83ef55b9deaf56dc73970a48945aa49f3ab0 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Wed, 19 Apr 2023 17:28:42 -0400 Subject: [PATCH 1/2] opt: do not add derived FK equalities to lookup join ON filters This commit fixes a minor bug introduced in #90599 which can unnecessarily add derived FK equality filters to a lookup join's ON condition. The bug is minor because it does not cause incorrect results. It only adds a bit of extra work to evaluate the equality. This commit ensures that the derived FK filters are only used as equality columns in the lookup join. Fixes #101844 Release note: None --- pkg/sql/opt/lookupjoin/constraint_builder.go | 19 ++++---- pkg/sql/opt/xform/testdata/rules/join | 48 ++++++++++++++++++++ pkg/sql/opt/xform/testdata/rules/limit | 8 +--- 3 files changed, 60 insertions(+), 15 deletions(-) diff --git a/pkg/sql/opt/lookupjoin/constraint_builder.go b/pkg/sql/opt/lookupjoin/constraint_builder.go index 96d8df11406f..af7630849e3d 100644 --- a/pkg/sql/opt/lookupjoin/constraint_builder.go +++ b/pkg/sql/opt/lookupjoin/constraint_builder.go @@ -145,18 +145,18 @@ func (b *ConstraintBuilder) Init( func (b *ConstraintBuilder) Build( index cat.Index, onFilters, optionalFilters, derivedFkOnFilters memo.FiltersExpr, ) (_ Constraint, foundEqualityCols bool) { - onFilters = append(onFilters, derivedFkOnFilters...) + // Combine the ON and derived FK filters which can contain equality + // conditions. + allFilters := make(memo.FiltersExpr, 0, len(onFilters)+len(derivedFkOnFilters)+len(optionalFilters)) + allFilters = append(allFilters, onFilters...) + allFilters = append(allFilters, derivedFkOnFilters...) - // Extract the equality columns from onFilters. We cannot use the results of - // the extraction in Init because onFilters may be reduced by the caller - // after Init due to partial index implication. If the filters are reduced, - // eqFilterOrds calculated during Init would no longer be valid because the - // ordinals of the filters will have changed. + // Extract the equality columns from the ON and derived FK filters. leftEq, rightEq, eqFilterOrds := - memo.ExtractJoinEqualityColumnsWithFilterOrds(b.leftCols, b.rightCols, onFilters) + memo.ExtractJoinEqualityColumnsWithFilterOrds(b.leftCols, b.rightCols, allFilters) rightEqSet := rightEq.ToSet() - // Retrieve the inequality columns from onFilters. + // Retrieve the inequality columns from the ON and derived FK filters. var rightCmp opt.ColList var inequalityFilterOrds []int if b.evalCtx.SessionData().VariableInequalityLookupJoinEnabled { @@ -164,7 +164,8 @@ func (b *ConstraintBuilder) Build( memo.ExtractJoinInequalityRightColumnsWithFilterOrds(b.leftCols, b.rightCols, onFilters) } - allFilters := append(onFilters, optionalFilters...) + // Add the optional filters. + allFilters = append(allFilters, optionalFilters...) // Check if the first column in the index either: // diff --git a/pkg/sql/opt/xform/testdata/rules/join b/pkg/sql/opt/xform/testdata/rules/join index 105fdc74bedd..ac7bfd90030e 100644 --- a/pkg/sql/opt/xform/testdata/rules/join +++ b/pkg/sql/opt/xform/testdata/rules/join @@ -4618,6 +4618,54 @@ inner-join (lookup t63735) └── filters └── y:7 = 15 [outer=(7), constraints=(/7: [/15 - /15]; tight), fd=()-->(7)] +# Regression test for #101844. Derived FK equalities should not be added to the +# ON filters of a lookup join. +exec-ddl +CREATE TABLE p101844 ( + r INT, + id INT, + PRIMARY KEY (r, id), + UNIQUE WITHOUT INDEX (id) +) +---- + +exec-ddl +CREATE TABLE c101844 ( + r INT, + p_id INT, + id INT, + PRIMARY KEY (r, p_id, id), + UNIQUE INDEX c_p_id_id_key (p_id, id), + FOREIGN KEY (r, p_id) REFERENCES p101844 (r, id) +) +---- + +# The derived c.r = p.r filters should not be added to the lookup join ON +# condition if they aren't used as equality columns. +opt +SELECT * +FROM p101844 p LEFT LOOKUP JOIN c101844@c_p_id_id_key c +ON c.p_id = p.id AND c.id = 1234 +---- +left-join (lookup c101844@c_p_id_id_key [as=c]) + ├── columns: r:1!null id:2!null r:5 p_id:6 id:7 + ├── flags: force lookup join (into right side) + ├── key columns: [2 10] = [6 7] + ├── lookup columns are key + ├── key: (2) + ├── fd: (2)-->(1,5-7), (6)-->(5) + ├── project + │ ├── columns: "lookup_join_const_col_@7":10!null p.r:1!null p.id:2!null + │ ├── key: (2) + │ ├── fd: ()-->(10), (2)-->(1) + │ ├── scan p101844 [as=p] + │ │ ├── columns: p.r:1!null p.id:2!null + │ │ ├── key: (2) + │ │ └── fd: (2)-->(1) + │ └── projections + │ └── 1234 [as="lookup_join_const_col_@7":10] + └── filters (true) + # -------------------------------------------------- # GenerateLookupJoinsWithVirtualCols diff --git a/pkg/sql/opt/xform/testdata/rules/limit b/pkg/sql/opt/xform/testdata/rules/limit index 7086c207ce10..0b1a50018cc4 100644 --- a/pkg/sql/opt/xform/testdata/rules/limit +++ b/pkg/sql/opt/xform/testdata/rules/limit @@ -2796,9 +2796,7 @@ project │ │ │ │ └── fd: ()-->(1,2), (5)-->(3,4,9) │ │ │ └── filters │ │ │ ├── t2.col2:20 = 1 [outer=(20), constraints=(/20: [/1 - /1]; tight), fd=()-->(20)] - │ │ │ ├── t2.col1:19 = 1 [outer=(19), constraints=(/19: [/1 - /1]; tight), fd=()-->(19)] - │ │ │ ├── t2.col3:21 = t1.col3:3 [outer=(3,21), constraints=(/3: (/NULL - ]; /21: (/NULL - ]), fd=(3)==(21), (21)==(3)] - │ │ │ └── t2.col4:22 = t1.col4:4 [outer=(4,22), constraints=(/4: (/NULL - ]; /22: (/NULL - ]), fd=(4)==(22), (22)==(4)] + │ │ │ └── t2.col1:19 = 1 [outer=(19), constraints=(/19: [/1 - /1]; tight), fd=()-->(19)] │ │ └── aggregations │ │ ├── array-agg [as=array_agg:27, outer=(24)] │ │ │ └── t2.col6:24 @@ -2953,9 +2951,7 @@ limit │ │ │ └── fd: ()-->(1,2), (5)-->(3,4,9) │ │ └── filters │ │ ├── t2.col2:20 = 1 [outer=(20), constraints=(/20: [/1 - /1]; tight), fd=()-->(20)] - │ │ ├── t2.col1:19 = 1 [outer=(19), constraints=(/19: [/1 - /1]; tight), fd=()-->(19)] - │ │ ├── t2.col3:21 = t1.col3:3 [outer=(3,21), constraints=(/3: (/NULL - ]; /21: (/NULL - ]), fd=(3)==(21), (21)==(3)] - │ │ └── t2.col4:22 = t1.col4:4 [outer=(4,22), constraints=(/4: (/NULL - ]; /22: (/NULL - ]), fd=(4)==(22), (22)==(4)] + │ │ └── t2.col1:19 = 1 [outer=(19), constraints=(/19: [/1 - /1]; tight), fd=()-->(19)] │ └── aggregations │ └── first-agg [as=t2.col2:20, outer=(20)] │ └── t2.col2:20 From 216c77b468d8787885c6fe2c88bb3365063049ae Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 20 Apr 2023 09:24:45 -0400 Subject: [PATCH 2/2] util/intsets: fix test-only Fast In #90009, `x/tools/Sparse` was replaced by a new `Sparse` integer set library. In that commit we forgot to update the test-only implementation of `Fast` to use the new library correctly. This was not caught earlier because CI and roachtests are not run with the `fast_int_set_small` and `fast_int_set_large` build tags. Release note: None --- pkg/util/intsets/fast_testonly.go | 36 +++++++++++++++---------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/pkg/util/intsets/fast_testonly.go b/pkg/util/intsets/fast_testonly.go index 0ba78c3ef040..ee47f456dd32 100644 --- a/pkg/util/intsets/fast_testonly.go +++ b/pkg/util/intsets/fast_testonly.go @@ -56,7 +56,7 @@ func (s *Fast) prepareForMutation() { // Add adds a value to the set. No-op if the value is already in the set. func (s *Fast) Add(i int) { s.prepareForMutation() - s.s.Insert(i) + s.s.Add(i) } // AddRange adds values 'from' up to 'to' (inclusively) to the set. @@ -66,7 +66,7 @@ func (s *Fast) Add(i int) { func (s *Fast) AddRange(from, to int) { s.prepareForMutation() for i := from; i <= to; i++ { - s.s.Insert(i) + s.s.Add(i) } } @@ -78,12 +78,12 @@ func (s *Fast) Remove(i int) { // Contains returns true if the set contains the value. func (s Fast) Contains(i int) bool { - return s.s != nil && s.s.Has(i) + return s.s != nil && s.s.Contains(i) } // Empty returns true if the set is empty. func (s Fast) Empty() bool { - return s.s == nil || s.s.IsEmpty() + return s.s == nil || s.s.Empty() } // Len returns the number of the elements in the set. @@ -119,7 +119,11 @@ func (s Fast) Ordered() []int { if s.Empty() { return nil } - return s.s.AppendTo([]int(nil)) + result := make([]int, 0, s.Len()) + s.ForEach(func(i int) { + result = append(result, i) + }) + return result } // Copy returns a copy of s which can be modified independently. @@ -213,16 +217,6 @@ func (s Fast) SubsetOf(rhs Fast) bool { return s.s.SubsetOf(rhs.s) } -// Shift generates a new set which contains elements i+delta for elements i in -// the original set. -func (s *Fast) Shift(delta int) Fast { - n := &Sparse{} - s.ForEach(func(i int) { - n.Insert(i + delta) - }) - return Fast{s: n} -} - // Encode the set and write it to a bytes.Buffer using binary.varint byte // encoding. // @@ -243,7 +237,14 @@ func (s *Fast) Encode(buf *bytes.Buffer) error { //gcassert:noescape tmp := make([]byte, binary.MaxVarintLen64+1) - if s.s == nil || s.s.Max() < 64 { + max := MinInt + s.ForEach(func(i int) { + if i > max { + max = i + } + }) + + if s.s == nil || max < 64 { n := binary.PutUvarint(tmp, 0) var bitmap uint64 for i, ok := s.Next(0); ok; i, ok = s.Next(i + 1) { @@ -296,6 +297,3 @@ func (s *Fast) Decode(br io.ByteReader) error { } return nil } - -// This is defined to allow the test to build. -const smallCutoff = 10