Skip to content

Commit

Permalink
props: normalize empty OrderingChoice instances in all cases
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Mark Sirek committed Apr 24, 2022
1 parent 621a046 commit 9e61561
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 6 deletions.
35 changes: 30 additions & 5 deletions pkg/sql/opt/props/ordering_choice.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
}
}
Expand Down Expand Up @@ -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,
}
}
Expand All @@ -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,
}
}
Expand Down Expand Up @@ -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:
Expand Down
14 changes: 13 additions & 1 deletion pkg/sql/opt/props/ordering_choice_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
{
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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 {
Expand Down
91 changes: 91 additions & 0 deletions pkg/sql/opt/xform/testdata/physprops/ordering
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
# --------------------------------------------------
Expand Down

0 comments on commit 9e61561

Please sign in to comment.