Skip to content

Commit

Permalink
opt: do not reuse columns for Unions in SplitScanIntoUnionScans
Browse files Browse the repository at this point in the history 
Unions generated in SplitScanIntoUnionScans no longer reuse column IDs
from their left children as output column IDs. Reusing column IDs in
this way has shown to be dangerous (see cockroachdb#58434).

Release note: None
  • Loading branch information
@mgartner
mgartner committed Jan 6, 2021
1 parent 5955601 commit fc7418e
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 94 deletions.
38 changes: 23 additions & 15 deletions pkg/sql/opt/norm/general_funcs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,26 @@ func (c *CustomFuncs) RedundantCols(input memo.RelExpr, cols opt.ColSet) opt.Col
return cols.Difference(reducedCols)
}

// DuplicateColumnIDs duplicates a table and set of columns IDs in the metadata.
// It returns the new table's ID and the new set of columns IDs.
func (c *CustomFuncs) DuplicateColumnIDs(
table opt.TableID, cols opt.ColSet,
) (opt.TableID, opt.ColSet) {
md := c.mem.Metadata()
tabMeta := md.TableMeta(table)
newTableID := md.DuplicateTable(table, c.RemapCols)

// Build a new set of column IDs from the new TableMeta.
var newColIDs opt.ColSet
for col, ok := cols.Next(0); ok; col, ok = cols.Next(col + 1) {
ord := tabMeta.MetaID.ColumnOrdinal(col)
newColID := newTableID.ColumnID(ord)
newColIDs.Add(newColID)
}

return newTableID, newColIDs
}

// RemapCols remaps columns IDs in the input ScalarExpr by replacing occurrences
// of the keys of colMap with the corresponding values. If column IDs are
// encountered in the input ScalarExpr that are not keys in colMap, they are not
Expand Down Expand Up @@ -1054,23 +1074,11 @@ func (c *CustomFuncs) DatumsEqual(first, second tree.Datum) bool {
// ScanPrivate, so the new ScanPrivate will not have constraints even if the old
// one did.
func (c *CustomFuncs) DuplicateScanPrivate(sp *memo.ScanPrivate) *memo.ScanPrivate {
md := c.mem.Metadata()
tabMeta := md.TableMeta(sp.Table)
newTableID := md.DuplicateTable(sp.Table, c.RemapCols)

// Build a new set of column IDs from the new TableMeta.
var newColIDs opt.ColSet
cols := sp.Cols
for col, ok := cols.Next(0); ok; col, ok = cols.Next(col + 1) {
ord := tabMeta.MetaID.ColumnOrdinal(col)
newColID := newTableID.ColumnID(ord)
newColIDs.Add(newColID)
}

table, cols := c.DuplicateColumnIDs(sp.Table, sp.Cols)
return &memo.ScanPrivate{
Table: newTableID,
Table: table,
Index: sp.Index,
Cols: newColIDs,
Cols: cols,
Flags: sp.Flags,
Locking: sp.Locking,
}
Expand Down
27 changes: 19 additions & 8 deletions pkg/sql/opt/xform/limit_funcs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,12 +249,11 @@ func (c *CustomFuncs) SplitScanIntoUnionScans(

// makeNewUnion extends the Union tree rooted at 'last' to include 'newScan'.
// The ColumnIDs of the original Scan are used by the resulting expression.
oldColList := scan.Relational().OutputCols.ToList()
makeNewUnion := func(last, newScan memo.RelExpr) memo.RelExpr {
makeNewUnion := func(last, newScan memo.RelExpr, outCols opt.ColList) memo.RelExpr {
return c.e.f.ConstructUnion(last, newScan, &memo.SetPrivate{
LeftCols: last.Relational().OutputCols.ToList(),
RightCols: newScan.Relational().OutputCols.ToList(),
OutCols: oldColList,
OutCols: outCols,
})
}

Expand All @@ -263,7 +262,7 @@ func (c *CustomFuncs) SplitScanIntoUnionScans(
// construct a single unlimited Scan, which will also be added to the Unions.
var noLimitSpans constraint.Spans
var last memo.RelExpr
for i := 0; i < spans.Count(); i++ {
for i, n := 0, spans.Count(); i < n; i++ {
if i >= budgetExceededIndex {
// The Scan budget has been reached; no additional Scans can be created.
noLimitSpans.Append(spans.Get(i))
Expand All @@ -276,15 +275,27 @@ func (c *CustomFuncs) SplitScanIntoUnionScans(
noLimitSpans.Append(spans.Get(i))
continue
}
for j := 0; j < singleKeySpans.Count(); j++ {
for j, m := 0, singleKeySpans.Count(); j < m; j++ {
if last == nil {
// This is the first limited Scan, so no Union necessary.
last = c.makeNewScan(sp, cons.Columns, newHardLimit, singleKeySpans.Get(j))
continue
}
// Construct a new Scan for each span and add it to the Union tree.
// Construct a new Scan for each span.
newScan := c.makeNewScan(sp, cons.Columns, newHardLimit, singleKeySpans.Get(j))
last = makeNewUnion(last, newScan)

// Add the scan to the union tree. If it is the final union in the
// tree, use the original scan's columns as the union's out columns.
// Otherwise, create new output column IDs for the union.
var outCols opt.ColList
finalUnion := i == n-1 && j == m-1 && noLimitSpans.Count() == 0
if finalUnion {
outCols = sp.Cols.ToList()
} else {
_, cols := c.DuplicateColumnIDs(sp.Table, sp.Cols)
outCols = cols.ToList()
}
last = makeNewUnion(last, newScan, outCols)
}
}
if noLimitSpans.Count() == spans.Count() {
Expand All @@ -306,7 +317,7 @@ func (c *CustomFuncs) SplitScanIntoUnionScans(
Spans: noLimitSpans,
}
newScan := c.e.f.ConstructScan(newScanPrivate)
return makeNewUnion(last, newScan)
return makeNewUnion(last, newScan, sp.Cols.ToList())
}

// indexHasOrderingSequence returns whether the Scan can provide a given
Expand Down
36 changes: 18 additions & 18 deletions pkg/sql/opt/xform/testdata/external/trading
View file
Original file line number Diff line number Diff line change
Expand Up @@ -612,25 +612,25 @@ project
│ │ │ ├── limit hint: 1.00
│ │ │ └── union
│ │ │ ├── columns: dealerid:8!null version:16!null
│ │ │ ├── left columns: dealerid:8!null version:16!null
│ │ │ ├── right columns: dealerid:65 version:73
│ │ │ ├── left columns: dealerid:75 version:83
│ │ │ ├── right columns: dealerid:85 version:93
│ │ │ ├── cardinality: [0 - 4]
│ │ │ ├── stats: [rows=4, distinct(8,16)=4, null(8,16)=0]
│ │ │ ├── key: (8,16)
│ │ │ ├── union
│ │ │ │ ├── columns: dealerid:8!null version:16!null
│ │ │ │ ├── left columns: dealerid:8!null version:16!null
│ │ │ │ ├── right columns: dealerid:55 version:63
│ │ │ │ ├── columns: dealerid:75!null version:83!null
│ │ │ │ ├── left columns: dealerid:55 version:63
│ │ │ │ ├── right columns: dealerid:65 version:73
│ │ │ │ ├── cardinality: [0 - 3]
│ │ │ │ ├── stats: [rows=3, distinct(8,16)=3, null(8,16)=0]
│ │ │ │ ├── key: (8,16)
│ │ │ │ ├── stats: [rows=3, distinct(75,83)=3, null(75,83)=0]
│ │ │ │ ├── key: (75,83)
│ │ │ │ ├── union
│ │ │ │ │ ├── columns: dealerid:8!null version:16!null
│ │ │ │ │ ├── columns: dealerid:55!null version:63!null
│ │ │ │ │ ├── left columns: dealerid:35 version:43
│ │ │ │ │ ├── right columns: dealerid:45 version:53
│ │ │ │ │ ├── cardinality: [0 - 2]
│ │ │ │ │ ├── stats: [rows=2, distinct(8,16)=2, null(8,16)=0]
│ │ │ │ │ ├── key: (8,16)
│ │ │ │ │ ├── stats: [rows=2, distinct(55,63)=2, null(55,63)=0]
│ │ │ │ │ ├── key: (55,63)
│ │ │ │ │ ├── scan cardsinfo@cardsinfoversionindex,rev
│ │ │ │ │ │ ├── columns: dealerid:35!null version:43!null
│ │ │ │ │ │ ├── constraint: /35/43: [/1 - /1]
Expand All @@ -646,19 +646,19 @@ project
│ │ │ │ │ ├── key: ()
│ │ │ │ │ └── fd: ()-->(45,53)
│ │ │ │ └── scan cardsinfo@cardsinfoversionindex,rev
│ │ │ │ ├── columns: dealerid:55!null version:63!null
│ │ │ │ ├── constraint: /55/63: [/3 - /3]
│ │ │ │ ├── columns: dealerid:65!null version:73!null
│ │ │ │ ├── constraint: /65/73: [/3 - /3]
│ │ │ │ ├── limit: 1(rev)
│ │ │ │ ├── stats: [rows=1, distinct(55)=1, null(55)=0, distinct(55,63)=1, null(55,63)=0]
│ │ │ │ ├── stats: [rows=1, distinct(65)=1, null(65)=0, distinct(65,73)=1, null(65,73)=0]
│ │ │ │ ├── key: ()
│ │ │ │ └── fd: ()-->(55,63)
│ │ │ │ └── fd: ()-->(65,73)
│ │ │ └── scan cardsinfo@cardsinfoversionindex,rev
│ │ │ ├── columns: dealerid:65!null version:73!null
│ │ │ ├── constraint: /65/73: [/4 - /4]
│ │ │ ├── columns: dealerid:85!null version:93!null
│ │ │ ├── constraint: /85/93: [/4 - /4]
│ │ │ ├── limit: 1(rev)
│ │ │ ├── stats: [rows=1, distinct(65)=1, null(65)=0, distinct(65,73)=1, null(65,73)=0]
│ │ │ ├── stats: [rows=1, distinct(85)=1, null(85)=0, distinct(85,93)=1, null(85,93)=0]
│ │ │ ├── key: ()
│ │ │ └── fd: ()-->(65,73)
│ │ │ └── fd: ()-->(85,93)
│ │ └── 1
│ └── aggregations
│ └── const-agg [as=max:33, outer=(16)]
Expand Down
36 changes: 18 additions & 18 deletions pkg/sql/opt/xform/testdata/external/trading-mutation
View file
Original file line number Diff line number Diff line change
Expand Up @@ -618,25 +618,25 @@ project
│ │ │ ├── limit hint: 1.00
│ │ │ └── union
│ │ │ ├── columns: dealerid:8!null version:16!null
│ │ │ ├── left columns: dealerid:8!null version:16!null
│ │ │ ├── right columns: dealerid:81 version:89
│ │ │ ├── left columns: dealerid:95 version:103
│ │ │ ├── right columns: dealerid:109 version:117
│ │ │ ├── cardinality: [0 - 4]
│ │ │ ├── stats: [rows=4, distinct(8,16)=4, null(8,16)=0]
│ │ │ ├── key: (8,16)
│ │ │ ├── union
│ │ │ │ ├── columns: dealerid:8!null version:16!null
│ │ │ │ ├── left columns: dealerid:8!null version:16!null
│ │ │ │ ├── right columns: dealerid:67 version:75
│ │ │ │ ├── columns: dealerid:95!null version:103!null
│ │ │ │ ├── left columns: dealerid:67 version:75
│ │ │ │ ├── right columns: dealerid:81 version:89
│ │ │ │ ├── cardinality: [0 - 3]
│ │ │ │ ├── stats: [rows=3, distinct(8,16)=3, null(8,16)=0]
│ │ │ │ ├── key: (8,16)
│ │ │ │ ├── stats: [rows=3, distinct(95,103)=3, null(95,103)=0]
│ │ │ │ ├── key: (95,103)
│ │ │ │ ├── union
│ │ │ │ │ ├── columns: dealerid:8!null version:16!null
│ │ │ │ │ ├── columns: dealerid:67!null version:75!null
│ │ │ │ │ ├── left columns: dealerid:39 version:47
│ │ │ │ │ ├── right columns: dealerid:53 version:61
│ │ │ │ │ ├── cardinality: [0 - 2]
│ │ │ │ │ ├── stats: [rows=2, distinct(8,16)=2, null(8,16)=0]
│ │ │ │ │ ├── key: (8,16)
│ │ │ │ │ ├── stats: [rows=2, distinct(67,75)=2, null(67,75)=0]
│ │ │ │ │ ├── key: (67,75)
│ │ │ │ │ ├── scan cardsinfo@cardsinfoversionindex,rev
│ │ │ │ │ │ ├── columns: dealerid:39!null version:47!null
│ │ │ │ │ │ ├── constraint: /39/47: [/1 - /1]
Expand All @@ -652,19 +652,19 @@ project
│ │ │ │ │ ├── key: ()
│ │ │ │ │ └── fd: ()-->(53,61)
│ │ │ │ └── scan cardsinfo@cardsinfoversionindex,rev
│ │ │ │ ├── columns: dealerid:67!null version:75!null
│ │ │ │ ├── constraint: /67/75: [/3 - /3]
│ │ │ │ ├── columns: dealerid:81!null version:89!null
│ │ │ │ ├── constraint: /81/89: [/3 - /3]
│ │ │ │ ├── limit: 1(rev)
│ │ │ │ ├── stats: [rows=1, distinct(67)=1, null(67)=0, distinct(67,75)=1, null(67,75)=0]
│ │ │ │ ├── stats: [rows=1, distinct(81)=1, null(81)=0, distinct(81,89)=1, null(81,89)=0]
│ │ │ │ ├── key: ()
│ │ │ │ └── fd: ()-->(67,75)
│ │ │ │ └── fd: ()-->(81,89)
│ │ │ └── scan cardsinfo@cardsinfoversionindex,rev
│ │ │ ├── columns: dealerid:81!null version:89!null
│ │ │ ├── constraint: /81/89: [/4 - /4]
│ │ │ ├── columns: dealerid:109!null version:117!null
│ │ │ ├── constraint: /109/117: [/4 - /4]
│ │ │ ├── limit: 1(rev)
│ │ │ ├── stats: [rows=1, distinct(81)=1, null(81)=0, distinct(81,89)=1, null(81,89)=0]
│ │ │ ├── stats: [rows=1, distinct(109)=1, null(109)=0, distinct(109,117)=1, null(109,117)=0]
│ │ │ ├── key: ()
│ │ │ └── fd: ()-->(81,89)
│ │ │ └── fd: ()-->(109,117)
│ │ └── 1
│ └── aggregations
│ └── const-agg [as=max:37, outer=(16)]
Expand Down
Loading

0 comments on commit fc7418e

Please sign in to comment.