From 9e615612efadc56006a775e7c4e6416921e86997 Mon Sep 17 00:00:00 2001 From: Mark Sirek Date: Sat, 23 Apr 2022 12:47:45 -0700 Subject: [PATCH] props: normalize empty OrderingChoice instances in all cases Fixes #79644 Previously, a query with an ORDER BY clause a DISTINCT ON clause and a GROUP BY clause involving columns in different join tables may error out if there is an index on one of the GROUP BY's Optional OrderingChoice columns and that column is not in the ordering required by the ORDER BY clause. `FromOrderingWithOptCols` is called during construction of the group by expression, and builds a `GroupingPrivate` with `Ordering` of `Any` (no columns), but still includes the `Optional` grouping columns. This violates the rule stated in the comments for `Optional`: ``` ...if Columns is empty, then Optional must be as well. ``` Because of this unnormalized `OrderingChoice`, this makes it appear that the projected grouping column can provide the required ordering even though this column does not actually intersect with the ordering columns. A later call to `OrderingChoice.Intersection` is made in `groupByBuildChildReqOrdering` to find the intersection between required and provided orderings, which panics when it cannot build the intersection. `Optional` columns may only be added to an ordering when there is at least one ordering column, and this assumption is also made in the code that builds `OrderingChoice`s. To address this, this patch fixes all locations where an empty `OrderingChoice` which matches `Any` ordering is built by normalizing it so that the `Optional` column set is empty. Release note (bug fix): This patch fixes queries which involve an ORDER BY clause, a DISTINCT ON clause and a GROUP BY clause, which may sometimes error out depending on the columns referenced in those clauses. --- pkg/sql/opt/props/ordering_choice.go | 35 ++++++- pkg/sql/opt/props/ordering_choice_test.go | 14 ++- pkg/sql/opt/xform/testdata/physprops/ordering | 91 +++++++++++++++++++ 3 files changed, 134 insertions(+), 6 deletions(-) diff --git a/pkg/sql/opt/props/ordering_choice.go b/pkg/sql/opt/props/ordering_choice.go index f199f6c08d7d..256747f4fd79 100644 --- a/pkg/sql/opt/props/ordering_choice.go +++ b/pkg/sql/opt/props/ordering_choice.go @@ -215,16 +215,21 @@ func (oc *OrderingChoice) FromOrdering(ord opt.Ordering) { // and with the given optional columns. Any optional columns in the given // ordering are ignored. func (oc *OrderingChoice) FromOrderingWithOptCols(ord opt.Ordering, optCols opt.ColSet) { - oc.Optional = optCols.Copy() oc.Columns = make([]OrderingColumnChoice, 0, len(ord)) for i := range ord { - if !oc.Optional.Contains(ord[i].ID()) { + if !optCols.Contains(ord[i].ID()) { oc.Columns = append(oc.Columns, OrderingColumnChoice{ Group: opt.MakeColSet(ord[i].ID()), Descending: ord[i].Descending(), }) } } + // If Columns is empty, then Optional must be as well. + if oc.Any() { + oc.Optional = opt.ColSet{} + } else { + oc.Optional = optCols.Copy() + } } // ToOrdering returns an opt.Ordering instance composed of the shortest possible @@ -430,8 +435,14 @@ func (oc *OrderingChoice) Intersection(other *OrderingChoice) OrderingChoice { for ; right < len(other.Columns); right++ { result = append(result, other.Columns[right]) } + var optional opt.ColSet + // If Columns is empty, then Optional must be as well. len(result) should + // never be zero here, but check anyway in case the logic changes. + if len(result) != 0 { + optional = oc.Optional.Intersection(other.Optional) + } return OrderingChoice{ - Optional: oc.Optional.Intersection(other.Optional), + Optional: optional, Columns: result, } } @@ -494,8 +505,13 @@ func (oc *OrderingChoice) CommonPrefix(other *OrderingChoice) OrderingChoice { right++ default: + var optional opt.ColSet + // If Columns is empty, then Optional must be as well. + if len(result) != 0 { + optional = oc.Optional.Intersection(other.Optional) + } return OrderingChoice{ - Optional: oc.Optional.Intersection(other.Optional), + Optional: optional, Columns: result, } } @@ -512,8 +528,13 @@ func (oc *OrderingChoice) CommonPrefix(other *OrderingChoice) OrderingChoice { Descending: rightCol.Descending, }) } + var optional opt.ColSet + // If Columns is empty, then Optional must be as well. + if len(result) != 0 { + optional = oc.Optional.Intersection(other.Optional) + } return OrderingChoice{ - Optional: oc.Optional.Intersection(other.Optional), + Optional: optional, Columns: result, } } @@ -805,6 +826,10 @@ func (oc *OrderingChoice) RestrictToCols(cols opt.ColSet) { } } } + // Normalize when OrderingChoice is Any. + if oc.Any() { + oc.Optional = opt.ColSet{} + } } // PrefixIntersection computes an OrderingChoice which: diff --git a/pkg/sql/opt/props/ordering_choice_test.go b/pkg/sql/opt/props/ordering_choice_test.go index 45bdedc3e17f..44728dea79e1 100644 --- a/pkg/sql/opt/props/ordering_choice_test.go +++ b/pkg/sql/opt/props/ordering_choice_test.go @@ -28,6 +28,11 @@ func TestOrderingChoice_FromOrdering(t *testing.T) { if exp, actual := "-2,+4 opt(1,3,5)", oc.String(); exp != actual { t.Errorf("expected %s, got %s", exp, actual) } + + oc.FromOrderingWithOptCols(opt.Ordering{1, 2, 3}, opt.MakeColSet(1, 2, 3)) + if exp, actual := "", oc.String(); exp != actual { + t.Errorf("expected %s, got %s", exp, actual) + } } func TestOrderingChoice_ToOrdering(t *testing.T) { @@ -155,6 +160,9 @@ func TestOrderingChoice_Intersection(t *testing.T) { {left: "+1", right: "+2 opt(2)", expected: "NO"}, {left: "+1", right: "-1 opt(2)", expected: "NO"}, {left: "+(1|2),+(3|4)", right: "+(2|5),+(6|7)", expected: "NO"}, + {left: "+1 opt(3)", right: "+2", expected: "NO"}, + {left: "+1", right: "+2 opt(3)", expected: "NO"}, + {left: "+1 opt(3)", right: "+2 opt(3)", expected: "NO"}, // Non-commutative cases. { @@ -236,6 +244,9 @@ func TestOrderingChoice_CommonPrefix(t *testing.T) { {left: "+1", right: "", expected: ""}, {left: "+1 opt(2)", right: "", expected: ""}, {left: "+1", right: "+1", expected: "+1"}, + {left: "+1 opt(3)", right: "+2", expected: ""}, + {left: "+1", right: "+2 opt(3)", expected: ""}, + {left: "+1 opt(3)", right: "+2 opt(3)", expected: ""}, {left: "+1,-2", right: "+1", expected: "+1"}, {left: "+1,-2", right: "+1,-2", expected: "+1,-2"}, {left: "+1", right: "+1 opt(2)", expected: "+1"}, @@ -614,7 +625,8 @@ func TestOrderingChoice_RestrictToCols(t *testing.T) { {s: "+1,+(2|3),-4 opt(5,6)", cols: []opt.ColumnID{1, 2, 4}, expected: "+1,+2,-4"}, {s: "+1,+(2|3),-4 opt(5,6)", cols: []opt.ColumnID{1, 4, 5, 6}, expected: "+1 opt(5,6)"}, {s: "+1,+(2|3),-4 opt(5,6)", cols: []opt.ColumnID{1, 3, 5}, expected: "+1,+3 opt(5)"}, - {s: "+1,+(2|3),-4 opt(5,6)", cols: []opt.ColumnID{2, 4, 5}, expected: "opt(5)"}, + {s: "+1,+(2|3),-4 opt(5,6)", cols: []opt.ColumnID{2, 4, 5}, expected: ""}, + {s: "+1,+(2|3),-4 opt(5,6)", cols: []opt.ColumnID{5, 6}, expected: ""}, } for _, tc := range testcases { diff --git a/pkg/sql/opt/xform/testdata/physprops/ordering b/pkg/sql/opt/xform/testdata/physprops/ordering index 88b9e31d8fb0..180e217d1562 100644 --- a/pkg/sql/opt/xform/testdata/physprops/ordering +++ b/pkg/sql/opt/xform/testdata/physprops/ordering @@ -1473,6 +1473,97 @@ distinct-on └── first-agg [as=b:2, outer=(2)] └── b:2 +# Regression test for #79644 +exec-ddl +CREATE TABLE t1 ( + id INT8 PRIMARY KEY, + col2 INT8 NOT NULL, + name STRING(1024), + UNIQUE (col2, name) +) +---- + +exec-ddl +CREATE TABLE t2 ( + col1 INT8 NOT NULL REFERENCES t1, + col2 INT4 NOT NULL, + PRIMARY KEY (col1, col2) +) +---- + +exec-ddl +CREATE TABLE t3 ( + col1 INT8 NOT NULL, + col2 INT4 NOT NULL, + col3 INT8 NOT NULL, + PRIMARY KEY (col1, col2, col3), + CONSTRAINT fk_t2 FOREIGN KEY (col1, col2) REFERENCES t2 (col1, col2) +) +---- + +# DISTINCT ON + GROUP BY + ORDER BY should not use the GROUP BY's empty +# OrderingChoice to try and fulfill the required ordering. +opt format=hide-stats +SELECT +DISTINCT ON (t2.col1) +t1.col2, t2.col1, array_agg(t3.col3) FROM t2 +INNER JOIN t1 ON t1.id = t2.col1 +INNER JOIN t3 ON t2.col2 = t3.col2 +GROUP BY (t2.col1, t1.col2, t2.col2) +ORDER BY (t2.col1, t2.col2) +---- +distinct-on + ├── columns: col2:6!null col1:1!null array_agg:15!null + ├── grouping columns: t2.col1:1!null + ├── internal-ordering: +2 opt(1,6) + ├── key: (1) + ├── fd: (1)-->(6,15) + ├── ordering: +1 + ├── sort + │ ├── columns: t2.col1:1!null t2.col2:2!null t1.col2:6!null array_agg:15!null + │ ├── key: (1,2) + │ ├── fd: (1)-->(6), (1,2)-->(6,15) + │ ├── ordering: +1,+2 + │ └── group-by (hash) + │ ├── columns: t2.col1:1!null t2.col2:2!null t1.col2:6!null array_agg:15!null + │ ├── grouping columns: t2.col1:1!null t2.col2:2!null + │ ├── key: (1,2) + │ ├── fd: (1)-->(6), (1,2)-->(6,15) + │ ├── inner-join (hash) + │ │ ├── columns: t2.col1:1!null t2.col2:2!null id:5!null t1.col2:6!null t3.col2:11!null col3:12!null + │ │ ├── multiplicity: left-rows(zero-or-more), right-rows(one-or-more) + │ │ ├── fd: (5)-->(6), (1)==(5), (5)==(1), (2)==(11), (11)==(2) + │ │ ├── inner-join (merge) + │ │ │ ├── columns: t2.col1:1!null t2.col2:2!null id:5!null t1.col2:6!null + │ │ │ ├── left ordering: +1 + │ │ │ ├── right ordering: +5 + │ │ │ ├── key: (2,5) + │ │ │ ├── fd: (5)-->(6), (1)==(5), (5)==(1) + │ │ │ ├── scan t2 + │ │ │ │ ├── columns: t2.col1:1!null t2.col2:2!null + │ │ │ │ ├── key: (1,2) + │ │ │ │ └── ordering: +1 + │ │ │ ├── scan t1 + │ │ │ │ ├── columns: id:5!null t1.col2:6!null + │ │ │ │ ├── key: (5) + │ │ │ │ ├── fd: (5)-->(6) + │ │ │ │ └── ordering: +5 + │ │ │ └── filters (true) + │ │ ├── scan t3 + │ │ │ └── columns: t3.col2:11!null col3:12!null + │ │ └── filters + │ │ └── t2.col2:2 = t3.col2:11 [outer=(2,11), fd=(2)==(11), (11)==(2)] + │ └── aggregations + │ ├── array-agg [as=array_agg:15, outer=(12)] + │ │ └── col3:12 + │ └── const-agg [as=t1.col2:6, outer=(6)] + │ └── t1.col2:6 + └── aggregations + ├── first-agg [as=t1.col2:6, outer=(6)] + │ └── t1.col2:6 + └── first-agg [as=array_agg:15, outer=(15)] + └── array_agg:15 + # -------------------------------------------------- # Set Operations. # --------------------------------------------------