Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
39052: opt: compute join output col map properly r=justinj a=justinj

Fixes cockroachdb#39010.

This manifested with the advent of WITH, but the problem was actually
that we were computing a join output map somewhat incorrectly. This
never actually caused problems before because we would prune any columns
that could cause there to be a disparity between the two ways of
computing the size of the ColMap, but since WITH doesn't support
pruning, it caused issues.

I also replaced some other uses of Len that were potentially sketchy.

Release note: None

Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
  • Loading branch information
craig[bot] and Justin Jaffray committed Jul 29, 2019
2 parents cee171c + 1feb3b0 commit 39ccedb
Showing 2 changed files with 51 additions and 4 deletions.
16 changes: 12 additions & 4 deletions pkg/sql/opt/exec/execbuilder/relational.go
Original file line number Diff line number Diff line change
@@ -69,7 +69,13 @@ type execPlan struct {
// if the node outputs the same optimizer ColumnID multiple times.
// TODO(justin): we should keep track of this instead of computing it each time.
func (ep *execPlan) numOutputCols() int {
max, ok := ep.outputCols.MaxValue()
return numOutputColsInMap(ep.outputCols)
}

// numOutputColsInMap returns the number of slots required to fill in all of
// the columns referred to by this ColMap.
func numOutputColsInMap(m opt.ColMap) int {
max, ok := m.MaxValue()
if !ok {
return 0
}
@@ -514,6 +520,7 @@ func (b *Builder) buildProject(prj *memo.ProjectExpr) (execPlan, error) {
if err != nil {
return execPlan{}, err
}

projections := prj.Projections
if len(projections) == 0 {
// We have only pass-through columns.
@@ -635,7 +642,7 @@ func (b *Builder) buildApplyJoin(join memo.RelExpr) (execPlan, error) {
allCols := joinOutputMap(left.outputCols, fakeRight.outputCols)

ctx := buildScalarCtx{
ivh: tree.MakeIndexedVarHelper(nil /* container */, allCols.Len()),
ivh: tree.MakeIndexedVarHelper(nil /* container */, numOutputColsInMap(allCols)),
ivarMap: allCols,
}

@@ -773,7 +780,7 @@ func (b *Builder) initJoinBuild(
allCols := joinOutputMap(leftPlan.outputCols, rightPlan.outputCols)

ctx := buildScalarCtx{
ivh: tree.MakeIndexedVarHelper(nil /* container */, allCols.Len()),
ivh: tree.MakeIndexedVarHelper(nil /* container */, numOutputColsInMap(allCols)),
ivarMap: allCols,
}

@@ -794,7 +801,8 @@ func (b *Builder) initJoinBuild(
// joinOutputMap determines the outputCols map for a (non-semi/anti) join, given
// the outputCols maps for its inputs.
func joinOutputMap(left, right opt.ColMap) opt.ColMap {
numLeftCols := left.Len()
numLeftCols := numOutputColsInMap(left)

res := left.Copy()
right.ForEach(func(colIdx, rightIdx int) {
res.Set(colIdx, rightIdx+numLeftCols)
39 changes: 39 additions & 0 deletions pkg/sql/opt/exec/execbuilder/testdata/with
Original file line number Diff line number Diff line change
@@ -68,3 +68,42 @@ root · · (a
· size 2 columns, 1 row · ·
· row 0, expr 0 1 · ·
· row 0, expr 1 unique_rowid() · ·

# Regression test for #39010.

statement ok
CREATE TABLE table39010 (col NAME)

query TTTTT
EXPLAIN (VERBOSE)
WITH
w AS (SELECT NULL, NULL FROM table39010)
SELECT
col
FROM
w, table39010
----
root · · (col) ·
├── render · · (col) ·
│ │ render 0 col · ·
│ └── hash-join · · ("?column?", "?column?", col) ·
│ │ type cross · ·
│ ├── render · · ("?column?", "?column?") ·
│ │ │ render 0 "?column?" · ·
│ │ │ render 1 "?column?" · ·
│ │ └── scan buffer node · · ("?column?") ·
│ │ label buffer 1 (w) · ·
│ └── scan · · (col) ·
│ table table39010@primary · ·
│ spans ALL · ·
└── subquery · · (col) ·
│ id @S1 · ·
│ original sql SELECT NULL, NULL FROM table39010 · ·
│ exec mode all rows · ·
└── buffer node · · ("?column?") ·
│ label buffer 1 (w) · ·
└── render · · ("?column?") ·
│ render 0 NULL · ·
└── scan · · () ·
· table table39010@primary · ·
· spans ALL · ·

0 comments on commit 39ccedb

Please sign in to comment.