Skip to content

Commit

Permalink
opt: fix ordering-related optimizer panics
Browse files Browse the repository at this point in the history
It is possible for some functional-dependency information to be visible
to a child operator but invisible to its parent. This could previously
cause panics when a child provided an ordering that could be proven to
satisfy the required ordering with the child FDs, but not with the
parent's FDs.

This patch adds a step to the logic that builds provided orderings that
ensures a provided ordering can be proven to respect the required ordering
without needing additional FD information. This ensures that a parent never
needs to know its child's FDs in order to prove that the provided ordering
is correct. The extra step is a no-op in the common case when the provided
ordering can already be proven to respect the required ordering.

Informs #85393
Informs #87806
Fixes #96288

Release note (bug fix): Fixed a rare internal error in the optimizer that has
existed since before version 22.1, which could occur while enforcing orderings
between SQL operators.
  • Loading branch information
DrewKimball committed Apr 6, 2023
1 parent 80831fb commit 0e5f0a8
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 12 deletions.
56 changes: 48 additions & 8 deletions pkg/sql/opt/ordering/ordering.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func BuildProvided(expr memo.RelExpr, required *props.OrderingChoice) opt.Orderi
return nil
}
provided := funcMap[expr.Op()].buildProvidedOrdering(expr, required)
provided = finalizeProvided(provided, required, expr.Relational().OutputCols)

if buildutil.CrdbTestBuild {
checkProvided(expr, required, provided)
Expand Down Expand Up @@ -423,6 +424,53 @@ func trimProvided(
return provided[:provIdx]
}

// finalizeProvided ensures that the provided ordering satisfies the following
// properties:
// 1. The provided ordering can be proven to satisfy the required ordering
// without the use of additional (e.g. functional dependency) information.
// 2. The provided ordering is simplified, such that it does not contain any
// columns from the required ordering optional set.
// 3. The provided ordering only refers to output columns for the operator.
//
// This step is necessary because it is possible for child operators to have
// different functional dependency information than their parents as well as
// different output columns. We have to protect against the case where a parent
// operator cannot prove that its child's provided ordering satisfies its
// required ordering.
func finalizeProvided(
provided opt.Ordering, required *props.OrderingChoice, outCols opt.ColSet,
) (newProvided opt.Ordering) {
// First check if the given provided is already suitable.
providedCols := provided.ColSet()
if len(provided) == len(required.Columns) && providedCols.SubsetOf(outCols) {
needsRemap := false
for i := range provided {
choice, ordCol := required.Columns[i], provided[i]
if !choice.Group.Contains(ordCol.ID()) || choice.Descending != ordCol.Descending() {
needsRemap = true
break
}
}
if !needsRemap {
return provided
}
}
newProvided = make(opt.Ordering, len(required.Columns))
for i, choice := range required.Columns {
group := choice.Group.Intersection(outCols)
if group.Intersects(providedCols) {
// Prefer using columns from the provided ordering if possible.
group.IntersectionWith(providedCols)
}
col, ok := group.Next(0)
if !ok {
panic(errors.AssertionFailedf("no output column equivalent to %d", redact.Safe(col)))
}
newProvided[i] = opt.MakeOrderingColumn(col, choice.Descending)
}
return newProvided
}

// checkRequired runs sanity checks on the ordering required of an operator.
func checkRequired(expr memo.RelExpr, required *props.OrderingChoice) {
rel := expr.Relational()
Expand Down Expand Up @@ -472,12 +520,4 @@ func checkProvided(expr memo.RelExpr, required *props.OrderingChoice, provided o
))
}
}

// The provided ordering should not have unnecessary columns.
fds := &expr.Relational().FuncDeps
if trimmed := trimProvided(provided, required, fds); len(trimmed) != len(provided) {
panic(errors.AssertionFailedf(
"provided %s can be trimmed to %s (FDs: %s)", redact.Safe(provided), redact.Safe(trimmed), redact.Safe(fds),
))
}
}
41 changes: 38 additions & 3 deletions pkg/sql/opt/xform/testdata/physprops/ordering
Original file line number Diff line number Diff line change
Expand Up @@ -3072,7 +3072,7 @@ sort
├── cardinality: [0 - 0]
├── key: ()
├── fd: ()-->(6,12,23)
├── ordering: +(12|23),-6 [actual: ]
├── ordering: +(12|23),-6 [actual: +12,-6]
└── values
├── columns: tab_922.crdb_internal_mvcc_timestamp:6!null col1_4:12!null col_2150:23!null
├── cardinality: [0 - 0]
Expand All @@ -3097,16 +3097,51 @@ distinct-on
│ ├── columns: c1:1!null c2:2
│ ├── key: (1)
│ ├── fd: (1)-->(2)
│ ├── ordering: +1 [actual: +2,+1]
│ ├── ordering: +1
│ ├── scan v0@i3
│ │ ├── columns: c1:5!null c2:6
│ │ ├── constraint: /6/5: [/'2023-01-31 00:00:00' - /'2023-01-31 00:00:00']
│ │ ├── key: (5)
│ │ ├── fd: (5)-->(6)
│ │ └── ordering: +5 [actual: +6,+5]
│ │ └── ordering: +5
│ └── projections
│ ├── c1:5 [as=c1:1, outer=(5)]
│ └── c2:6 [as=c2:2, outer=(6)]
└── aggregations
└── const-agg [as=c2:2, outer=(2)]
└── c2:2

opt
SELECT c1 FROM v0
WHERE (c1 = c1 AND c2 = '01-31-2023 00:00:00'::TIMESTAMP)
OR (c1 = b'00' AND c1 = b'0')
OR (c1 IS NULL AND c2 IS NULL)
ORDER BY c1;
----
project
├── columns: c1:1!null
├── key: (1)
├── ordering: +1
└── distinct-on
├── columns: c1:1!null c2:2
├── grouping columns: c1:1!null
├── key: (1)
├── fd: (1)-->(2)
├── ordering: +1
├── project
│ ├── columns: c1:1!null c2:2
│ ├── key: (1)
│ ├── fd: (1)-->(2)
│ ├── ordering: +1
│ ├── scan v0@i3
│ │ ├── columns: c1:5!null c2:6
│ │ ├── constraint: /6/5: [/'2023-01-31 00:00:00' - /'2023-01-31 00:00:00']
│ │ ├── key: (5)
│ │ ├── fd: (5)-->(6)
│ │ └── ordering: +5
│ └── projections
│ ├── c1:5 [as=c1:1, outer=(5)]
│ └── c2:6 [as=c2:2, outer=(6)]
└── aggregations
└── const-agg [as=c2:2, outer=(2)]
└── c2:2
2 changes: 1 addition & 1 deletion pkg/sql/opt/xform/testdata/rules/join
Original file line number Diff line number Diff line change
Expand Up @@ -9823,7 +9823,7 @@ project
│ ├── cardinality: [0 - 1]
│ ├── key: (25)
│ ├── fd: (25)-->(26,27), (27)~~>(25,26)
│ ├── ordering: +25 [actual: ]
│ ├── ordering: +25
│ └── project
│ ├── columns: a:25!null b:26!null c:27!null q:31!null r:32!null
│ ├── cardinality: [0 - 1]
Expand Down

0 comments on commit 0e5f0a8

Please sign in to comment.