Skip to content

Commit

Permalink
sql: propagate opt types through renders
Browse files Browse the repository at this point in the history
Previously, folded expressions would sometimes get improperly typed,
because the optimizer type would be lost and replaced with the
ResolvedType of the folded expression. Datums can sometimes have
imprecise ResolvedTypes if they were created from expressions with
non-canonical types of a family.

Fixes cockroachdb#48563.

Release note (bug fix): improve accuracy of column types returned
from queries to improve PostgreSQL compatibility.
  • Loading branch information
jordanlewis committed May 16, 2020
1 parent f66ff29 commit b16dcae
Show file tree
Hide file tree
Showing 8 changed files with 78 additions and 48 deletions.
1 change: 0 additions & 1 deletion pkg/sql/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ func newCopyMachine(
Typ: cols[i].Type,
TableID: tableDesc.GetID(),
PGAttributeNum: cols[i].GetLogicalColumnID(),
TypeModifier: cols[i].Type.TypeModifier(),
}
}
c.rowsMemAcc = c.p.extendedEvalCtx.Mon.MakeBoundAccount()
Expand Down
5 changes: 4 additions & 1 deletion pkg/sql/opt/bench/stub_factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ func (f *stubFactory) ConstructSimpleProject(
}

func (f *stubFactory) ConstructRender(
n exec.Node, exprs tree.TypedExprs, colNames []string, reqOrdering exec.OutputOrdering,
n exec.Node,
columns sqlbase.ResultColumns,
exprs tree.TypedExprs,
reqOrdering exec.OutputOrdering,
) (exec.Node, error) {
return struct{}{}, nil
}
Expand Down
18 changes: 13 additions & 5 deletions pkg/sql/opt/exec/execbuilder/relational.go
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,7 @@ func (b *Builder) buildProject(prj *memo.ProjectExpr) (execPlan, error) {

var res execPlan
exprs := make(tree.TypedExprs, 0, len(projections)+prj.Passthrough.Len())
colNames := make([]string, 0, len(exprs))
cols := make(sqlbase.ResultColumns, 0, len(exprs))
ctx := input.makeBuildScalarCtx()
for i := range projections {
item := &projections[i]
Expand All @@ -575,15 +575,23 @@ func (b *Builder) buildProject(prj *memo.ProjectExpr) (execPlan, error) {
}
res.outputCols.Set(int(item.Col), i)
exprs = append(exprs, expr)
colNames = append(colNames, md.ColumnMeta(item.Col).Alias)
cols = append(cols, sqlbase.ResultColumn{
Name: md.ColumnMeta(item.Col).Alias,
Typ: item.Typ,
})
}
prj.Passthrough.ForEach(func(colID opt.ColumnID) {
res.outputCols.Set(int(colID), len(exprs))
exprs = append(exprs, b.indexedVar(&ctx, md, colID))
colNames = append(colNames, md.ColumnMeta(colID).Alias)
indexedVar := b.indexedVar(&ctx, md, colID)
exprs = append(exprs, indexedVar)
meta := md.ColumnMeta(colID)
cols = append(cols, sqlbase.ResultColumn{
Name: meta.Alias,
Typ: meta.Type,
})
})
reqOrdering := res.reqOrdering(prj)
res.root, err = b.factory.ConstructRender(input.root, exprs, colNames, reqOrdering)
res.root, err = b.factory.ConstructRender(input.root, cols, exprs, reqOrdering)
if err != nil {
return execPlan{}, err
}
Expand Down
5 changes: 3 additions & 2 deletions pkg/sql/opt/exec/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,10 @@ type Factory interface {
) (Node, error)

// ConstructRender returns a node that applies a projection on the results of
// the given input node. The projection can contain new expressions.
// the given input node. The projection can contain new expressions. The input
// expression slice will be modified.
ConstructRender(
n Node, exprs tree.TypedExprs, colNames []string, reqOrdering OutputOrdering,
n Node, columns sqlbase.ResultColumns, exprs tree.TypedExprs, reqOrdering OutputOrdering,
) (Node, error)

// ConstructApplyJoin returns a node that runs an apply join between an input
Expand Down
54 changes: 23 additions & 31 deletions pkg/sql/opt_exec_factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,15 +281,18 @@ func (ef *execFactory) ConstructSimpleProject(
}

var rb renderBuilder
rb.init(n, reqOrdering, len(cols))
rb.init(n, reqOrdering)
exprs := make(tree.TypedExprs, len(cols))
resultCols := make(sqlbase.ResultColumns, len(cols))
for i, col := range cols {
v := rb.r.ivarHelper.IndexedVar(int(col))
if colNames == nil {
rb.addExpr(v, inputCols[col].Name, inputCols[col].TableID, inputCols[col].PGAttributeNum, inputCols[col].GetTypeModifier())
} else {
rb.addExpr(v, colNames[i], 0 /* tableID */, 0 /* pgAttributeNum */, v.ResolvedType().TypeModifier())
exprs[i] = v
resultCols[i] = inputCols[col]
if colNames != nil {
resultCols[i].Name = colNames[i]
}
}
rb.setOutput(exprs, resultCols)
return rb.res, nil
}

Expand All @@ -306,14 +309,18 @@ func hasDuplicates(cols []exec.NodeColumnOrdinal) bool {

// ConstructRender is part of the exec.Factory interface.
func (ef *execFactory) ConstructRender(
n exec.Node, exprs tree.TypedExprs, colNames []string, reqOrdering exec.OutputOrdering,
n exec.Node,
columns sqlbase.ResultColumns,
exprs tree.TypedExprs,
reqOrdering exec.OutputOrdering,
) (exec.Node, error) {
var rb renderBuilder
rb.init(n, reqOrdering, len(exprs))
rb.init(n, reqOrdering)
for i, expr := range exprs {
expr = rb.r.ivarHelper.Rebind(expr, false /* alsoReset */, true /* normalizeToNonNil */)
rb.addExpr(expr, colNames[i], 0 /* tableID */, 0 /* pgAttributeNum */, -1 /* typeModifier */)
exprs[i] = expr
}
rb.setOutput(exprs, columns)
return rb.res, nil
}

Expand Down Expand Up @@ -2024,12 +2031,10 @@ type renderBuilder struct {
}

// init initializes the renderNode with render expressions.
func (rb *renderBuilder) init(n exec.Node, reqOrdering exec.OutputOrdering, cap int) {
func (rb *renderBuilder) init(n exec.Node, reqOrdering exec.OutputOrdering) {
src := asDataSource(n)
rb.r = &renderNode{
source: src,
render: make([]tree.TypedExpr, 0, cap),
columns: make([]sqlbase.ResultColumn, 0, cap),
source: src,
}
rb.r.ivarHelper = tree.MakeIndexedVarHelper(rb.r, len(src.columns))
rb.r.reqOrdering = ReqOrdering(reqOrdering)
Expand All @@ -2044,25 +2049,12 @@ func (rb *renderBuilder) init(n exec.Node, reqOrdering exec.OutputOrdering, cap
}
}

// addExpr adds a new render expression with the given name.
func (rb *renderBuilder) addExpr(
expr tree.TypedExpr,
colName string,
tableID sqlbase.ID,
pgAttributeNum sqlbase.ColumnID,
typeModifier int32,
) {
rb.r.render = append(rb.r.render, expr)
rb.r.columns = append(
rb.r.columns,
sqlbase.ResultColumn{
Name: colName,
Typ: expr.ResolvedType(),
TableID: tableID,
PGAttributeNum: pgAttributeNum,
TypeModifier: typeModifier,
},
)
// setOutput sets the output of the renderNode. exprs is the list of render
// expressions, and columns is the list of information about the expressions,
// including their names, types, and so on. They must be the same length.
func (rb *renderBuilder) setOutput(exprs tree.TypedExprs, columns sqlbase.ResultColumns) {
rb.r.render = exprs
rb.r.columns = columns
}

// makeColDescList returns a list of table column descriptors. Columns are
Expand Down
34 changes: 34 additions & 0 deletions pkg/sql/pgwire/testdata/pgtest/char
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
send
Query {"String": "CREATE TABLE a (a INT PRIMARY KEY)"}
----

until
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"CREATE TABLE"}
{"Type":"ReadyForQuery","TxStatus":"I"}

send
Query {"String": "INSERT INTO a VALUES(1)"}
----

until
ReadyForQuery
----
{"Type":"CommandComplete","CommandTag":"INSERT 0 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

# Make sure that values casted to "char" get their type oid and type size
# reported correctly via pgwire.

send
Query {"String": "SELECT 'a'::\"char\" FROM a"}
----

until
ReadyForQuery
----
{"Type":"RowDescription","Fields":[{"Name":"char","TableOID":0,"TableAttributeNumber":0,"DataTypeOID":18,"DataTypeSize":1,"TypeModifier":-1,"Format":0}]}
{"Type":"DataRow","Values":[{"text":"a"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}
8 changes: 1 addition & 7 deletions pkg/sql/sqlbase/result_columns.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ type ResultColumn struct {
// reference, these fields are zeroes.
TableID ID // OID of column's source table (pg_attribute.attrelid).
PGAttributeNum ColumnID // Column's number in source table (pg_attribute.attnum).
TypeModifier int32 // Type-specific data size (pg_attribute.atttypmod).
}

// ResultColumns is the type used throughout the sql module to
Expand All @@ -57,7 +56,6 @@ func ResultColumnsFromColDescs(tableID ID, colDescs []ColumnDescriptor) ResultCo
Hidden: hidden,
TableID: tableID,
PGAttributeNum: colDesc.GetLogicalColumnID(),
TypeModifier: typ.TypeModifier(),
},
)
}
Expand All @@ -67,11 +65,7 @@ func ResultColumnsFromColDescs(tableID ID, colDescs []ColumnDescriptor) ResultCo
// GetTypeModifier returns the type modifier for this column. If it is not set,
// it defaults to returning -1.
func (r ResultColumn) GetTypeModifier() int32 {
if r.TypeModifier != 0 {
return r.TypeModifier
}

return -1
return r.Typ.TypeModifier()
}

// TypesEqual returns whether the length and types of r matches other. If
Expand Down
1 change: 0 additions & 1 deletion pkg/sql/virtual_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,6 @@ func (e virtualDefEntry) getPlanInfo(
Typ: col.Type,
TableID: table.GetID(),
PGAttributeNum: col.GetLogicalColumnID(),
TypeModifier: col.Type.TypeModifier(),
})
}

Expand Down

0 comments on commit b16dcae

Please sign in to comment.