From b16dcae612afb34a63c4bd161fc76b62e2821788 Mon Sep 17 00:00:00 2001 From: Jordan Lewis Date: Fri, 8 May 2020 21:08:17 -0400 Subject: [PATCH] sql: propagate opt types through renders 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 #48563. Release note (bug fix): improve accuracy of column types returned from queries to improve PostgreSQL compatibility. --- pkg/sql/copy.go | 1 - pkg/sql/opt/bench/stub_factory.go | 5 +- pkg/sql/opt/exec/execbuilder/relational.go | 18 ++++++-- pkg/sql/opt/exec/factory.go | 5 +- pkg/sql/opt_exec_factory.go | 54 +++++++++------------- pkg/sql/pgwire/testdata/pgtest/char | 34 ++++++++++++++ pkg/sql/sqlbase/result_columns.go | 8 +--- pkg/sql/virtual_schema.go | 1 - 8 files changed, 78 insertions(+), 48 deletions(-) create mode 100644 pkg/sql/pgwire/testdata/pgtest/char diff --git a/pkg/sql/copy.go b/pkg/sql/copy.go index 63886c08aaa6..65ee8354f9e5 100644 --- a/pkg/sql/copy.go +++ b/pkg/sql/copy.go @@ -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() diff --git a/pkg/sql/opt/bench/stub_factory.go b/pkg/sql/opt/bench/stub_factory.go index 60118fd81a12..7676f7caa810 100644 --- a/pkg/sql/opt/bench/stub_factory.go +++ b/pkg/sql/opt/bench/stub_factory.go @@ -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 } diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index 20ff6d25f91a..d29adf4890bb 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -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] @@ -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 } diff --git a/pkg/sql/opt/exec/factory.go b/pkg/sql/opt/exec/factory.go index 2dd844ff2edd..dec7e9ac0238 100644 --- a/pkg/sql/opt/exec/factory.go +++ b/pkg/sql/opt/exec/factory.go @@ -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 diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go index d90d3d98831f..c11356ad8895 100644 --- a/pkg/sql/opt_exec_factory.go +++ b/pkg/sql/opt_exec_factory.go @@ -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 } @@ -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 } @@ -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) @@ -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 diff --git a/pkg/sql/pgwire/testdata/pgtest/char b/pkg/sql/pgwire/testdata/pgtest/char new file mode 100644 index 000000000000..ce115601c68c --- /dev/null +++ b/pkg/sql/pgwire/testdata/pgtest/char @@ -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"} diff --git a/pkg/sql/sqlbase/result_columns.go b/pkg/sql/sqlbase/result_columns.go index 35d61922bb8b..330fe189fa09 100644 --- a/pkg/sql/sqlbase/result_columns.go +++ b/pkg/sql/sqlbase/result_columns.go @@ -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 @@ -57,7 +56,6 @@ func ResultColumnsFromColDescs(tableID ID, colDescs []ColumnDescriptor) ResultCo Hidden: hidden, TableID: tableID, PGAttributeNum: colDesc.GetLogicalColumnID(), - TypeModifier: typ.TypeModifier(), }, ) } @@ -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 diff --git a/pkg/sql/virtual_schema.go b/pkg/sql/virtual_schema.go index cccf42995ee0..4866ddd0d14b 100644 --- a/pkg/sql/virtual_schema.go +++ b/pkg/sql/virtual_schema.go @@ -338,7 +338,6 @@ func (e virtualDefEntry) getPlanInfo( Typ: col.Type, TableID: table.GetID(), PGAttributeNum: col.GetLogicalColumnID(), - TypeModifier: col.Type.TypeModifier(), }) }