Merge #57354

57354: sql: fix some type / string conversion brokenness r=otan a=knz

Fixes #57347
Fixes #57187

This ensures that e.g. `drop type "a+b"` works if there is a type
indeed named `a+b`.

Release note (bug fix): DROP TYPE and certain other statements that
work over SQL scalar types now properly support type names containing
special characters.

Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/internal/sqlsmith/schema.go

@@ -314,7 +314,7 @@ ORDER BY
 			currentCols = nil
 		}
 
-		coltyp, err := s.typeFromName(typ)
+		coltyp, err := s.typeFromSQLTypeSyntax(typ)
 		if err != nil {
 			return nil, err
 		}

pkg/internal/sqlsmith/type.go

@@ -21,10 +21,10 @@ import (
 	"github.com/lib/pq/oid"
 )
 
-func (s *Smither) typeFromName(name string) (*types.T, error) {
-	typRef, err := parser.ParseType(name)
+func (s *Smither) typeFromSQLTypeSyntax(typeStr string) (*types.T, error) {
+	typRef, err := parser.GetTypeFromValidSQLSyntax(typeStr)
 	if err != nil {
-		return nil, errors.AssertionFailedf("failed to parse type: %v", name)
+		return nil, errors.AssertionFailedf("failed to parse type: %v", typeStr)
 	}
 	typ, err := tree.ResolveType(context.Background(), typRef, s)
 	if err != nil {

pkg/sql/faketreeeval/evalctx.go

@@ -137,8 +137,8 @@ func (ep *DummyEvalPlanner) ResolveTableName(
 	return 0, errors.WithStack(errEvalPlanner)
 }
 
-// ParseType is part of the tree.EvalPlanner interface.
-func (ep *DummyEvalPlanner) ParseType(sql string) (*types.T, error) {
+// GetTypeFromValidSQLSyntax is part of the tree.EvalPlanner interface.
+func (ep *DummyEvalPlanner) GetTypeFromValidSQLSyntax(sql string) (*types.T, error) {
 	return nil, errors.WithStack(errEvalPlanner)
 }
 

pkg/sql/logictest/testdata/logic_test/builtin_function

@@ -1962,6 +1962,7 @@ VALUES (format_type('anyelement'::regtype, -1)),
        (format_type('bool'::regtype, -1)),
        (format_type('bytea'::regtype, -1)),
        (format_type('char'::regtype, -1)),
+       (format_type('"char"'::regtype, -1)),
        (format_type('date'::regtype, -1)),
        (format_type('decimal'::regtype, -1)),
        (format_type('float'::regtype, -1)),
@@ -1995,13 +1996,16 @@ VALUES (format_type('anyelement'::regtype, -1)),
        (format_type('int[]'::regtype, -1)),
        (format_type('int2[]'::regtype, -1)),
        (format_type('string[]'::regtype, -1)),
-       (format_type('varchar[]'::regtype, -1))
+       (format_type('varchar[]'::regtype, -1)),
+       (format_type('pg_catalog.int4'::regtype, -1)),
+       (format_type('pg_catalog.int2'::regtype, -1))
 ----
 anyelement
 bit
 boolean
 bytea
 bpchar
+"char"
 date
 numeric
 double precision
@@ -2036,6 +2040,8 @@ bigint[]
 smallint[]
 text[]
 character varying[]
+integer
+smallint
 
 query T
 VALUES (format_type('anyelement'::regtype, NULL)),

pkg/sql/logictest/testdata/logic_test/drop_type

@@ -338,3 +338,16 @@ DROP DATABASE d
 
 query I
 SELECT id FROM system.namespace WHERE name LIKE 'd_t%'
+
+subtest regression_57187
+
+statement ok
+CREATE DATABASE d;
+CREATE TYPE d."a<b" AS ENUM('hello');
+CREATE TYPE d."b+c" AS ENUM('hello')
+
+statement ok
+DROP TYPE d."b+c"
+
+statement ok
+DROP DATABASE d

pkg/sql/logictest/testdata/logic_test/grant_type

@@ -10,27 +10,35 @@ statement ok
 CREATE TYPE public.enum_a AS ENUM ('hello', 'goodbye');
 GRANT USAGE ON TYPE public.enum_a TO user1
 
+statement ok
+CREATE TYPE public."enum_a+b" AS ENUM ('hello', 'goodbye');
+GRANT USAGE ON TYPE public."enum_a+b" TO user1
+
 statement ok
 CREATE TYPE schema_a.enum_b AS ENUM ('hi', 'bye');
 GRANT ALL ON TYPE schema_a.enum_b TO user1
 
 query TTTTT colnames
-SHOW GRANTS ON TYPE schema_a.enum_b, enum_a, int
+SHOW GRANTS ON TYPE schema_a.enum_b, "enum_a+b", enum_a, int4
 ----
 database_name  schema_name  type_name  grantee  privilege_type
-test           pg_catalog   int        admin    ALL
-test           pg_catalog   int        public   USAGE
-test           pg_catalog   int        root     ALL
+test           pg_catalog   int4       admin    ALL
+test           pg_catalog   int4       public   USAGE
+test           pg_catalog   int4       root     ALL
 test           public       enum_a     admin    ALL
 test           public       enum_a     public   USAGE
 test           public       enum_a     root     ALL
 test           public       enum_a     user1    USAGE
+test           public       enum_a+b   admin    ALL
+test           public       enum_a+b   public   USAGE
+test           public       enum_a+b   root     ALL
+test           public       enum_a+b   user1    USAGE
 test           schema_a     enum_b     admin    ALL
 test           schema_a     enum_b     root     ALL
 test           schema_a     enum_b     user1    ALL
 
 query TTTTT colnames
-SHOW GRANTS ON TYPE schema_a.enum_b, enum_a, int FOR user1
+SHOW GRANTS ON TYPE schema_a.enum_b, enum_a, int4 FOR user1
 ----
 database_name  schema_name  type_name  grantee  privilege_type
 test           public       enum_a     user1    USAGE
@@ -41,6 +49,7 @@ SHOW GRANTS FOR user1
 ----
 database_name  schema_name  relation_name  grantee  privilege_type
 test           public       enum_a         user1    USAGE
+test           public       enum_a+b       user1    USAGE
 test           schema_a     enum_b         user1    ALL
 
 statement error type "non_existent" does not exist

pkg/sql/logictest/testdata/logic_test/pgoidtype

@@ -131,6 +131,11 @@ SELECT 'numeric(10,3)'::REGTYPE, 'numeric( 10, 3 )'::REGTYPE
 ----
 numeric  numeric
 
+query OO
+SELECT '"char"'::REGTYPE, 'pg_catalog.int4'::REGTYPE
+----
+"char"  integer
+
 query error type 'foo.' does not exist
 SELECT 'foo.'::REGTYPE
 
@@ -149,6 +154,9 @@ SELECT 'blah'::REGNAMESPACE
 query error type 'blah' does not exist
 SELECT 'blah'::REGTYPE
 
+query error type 'pg_catalog.int' does not exist
+SELECT 'pg_catalog.int'::REGTYPE
+
 ## Test other cast syntaxes
 
 query O

pkg/sql/opt/optgen/exprgen/parse_type.go

@@ -42,7 +42,7 @@ func ParseType(typeStr string) (*types.T, error) {
 		}
 		return types.MakeTuple(colTypes), nil
 	}
-	typ, err := parser.ParseType(typeStr)
+	typ, err := parser.GetTypeFromValidSQLSyntax(typeStr)
 	if err != nil {
 		return nil, err
 	}

pkg/sql/opt/testutils/testcat/test_catalog.go

@@ -1022,7 +1022,7 @@ func (ts *TableStat) Histogram() []cat.HistogramBucket {
 	if ts.js.HistogramColumnType == "" || ts.js.HistogramBuckets == nil {
 		return nil
 	}
-	colTypeRef, err := parser.ParseType(ts.js.HistogramColumnType)
+	colTypeRef, err := parser.GetTypeFromValidSQLSyntax(ts.js.HistogramColumnType)
 	if err != nil {
 		panic(err)
 	}

pkg/sql/parser/BUILD.bazel

@@ -62,7 +62,6 @@ go_test(
         "//vendor/github.com/cockroachdb/datadriven",
         "//vendor/github.com/cockroachdb/errors",
         "//vendor/github.com/stretchr/testify/assert",
-        "//vendor/github.com/stretchr/testify/require",
     ],
 )
 

pkg/sql/parser/parse.go

@@ -274,8 +274,13 @@ func HasMultipleStatements(sql string) bool {
 	return false
 }
 
-// ParseQualifiedTableName parses a SQL string of the form
-// `[ database_name . ] [ schema_name . ] table_name`.
+// ParseQualifiedTableName parses a possibly qualified table name. The
+// table name must contain one or more name parts, using the full
+// input SQL syntax: each name part containing special characters, or
+// non-lowercase characters, must be enclosed in double quote. The
+// name may not be an invalid table name (the caller is responsible
+// for guaranteeing that only valid table names are provided as
+// input).
 func ParseQualifiedTableName(sql string) (*tree.TableName, error) {
 	name, err := ParseTableName(sql)
 	if err != nil {
@@ -285,7 +290,12 @@ func ParseQualifiedTableName(sql string) (*tree.TableName, error) {
 	return &tn, nil
 }
 
-// ParseTableName parses a table name.
+// ParseTableName parses a table name. The table name must contain one
+// or more name parts, using the full input SQL syntax: each name
+// part containing special characters, or non-lowercase characters,
+// must be enclosed in double quote. The name may not be an invalid
+// table name (the caller is responsible for guaranteeing that only
+// valid table names are provided as input).
 func ParseTableName(sql string) (*tree.UnresolvedObjectName, error) {
 	// We wrap the name we want to parse into a dummy statement since our parser
 	// can only parse full statements.
@@ -300,30 +310,6 @@ func ParseTableName(sql string) (*tree.UnresolvedObjectName, error) {
 	return rename.Name, nil
 }
 
-// ParseTableNameWithQualifiedNames can be used to parse an input table name that
-// might be prefixed with an unquoted qualified name. The standard ParseTableName
-// cannot do this due to limitations with our parser. In particular, the parser
-// can't parse different productions individually -- it must parse them as part
-// of a top level statement. This causes qualified names that contain keywords
-// to require quotes, which are not required in some cases due to Postgres
-// compatibility (in particular, as arguments to pg_dump). This function gets
-// around this limitation by parsing the input table name as a column name
-// with a fake non-keyword prefix, and then shifting the result down into an
-// UnresolvedObjectName.
-func ParseTableNameWithQualifiedNames(sql string) (*tree.UnresolvedObjectName, error) {
-	stmt, err := ParseOne(fmt.Sprintf("SELECT fakeprefix.%s", sql))
-	if err != nil {
-		return nil, err
-	}
-	un := stmt.AST.(*tree.Select).Select.(*tree.SelectClause).Exprs[0].Expr.(*tree.UnresolvedName)
-	var nameParts [3]string
-	numParts := un.NumParts - 1
-	for i := 0; i < numParts; i++ {
-		nameParts[i] = un.Parts[i]
-	}
-	return tree.NewUnresolvedObjectName(numParts, nameParts, 0 /* annotationIdx */)
-}
-
 // parseExprsWithInt parses one or more sql expressions.
 func parseExprsWithInt(exprs []string, nakedIntType *types.T) (tree.Exprs, error) {
 	stmt, err := ParseOneWithInt(fmt.Sprintf("SET ROW (%s)", strings.Join(exprs, ",")), nakedIntType)
@@ -337,22 +323,31 @@ func parseExprsWithInt(exprs []string, nakedIntType *types.T) (tree.Exprs, error
 	return set.Values, nil
 }
 
-// ParseExprs is a short-hand for parseExprsWithInt(sql, defaultNakedIntType).
+// ParseExprs parses a comma-delimited sequence of SQL scalar
+// expressions. The caller is responsible for ensuring that the input
+// is, in fact, a comma-delimited sequence of SQL scalar expressions —
+// the results are undefined if the string contains invalid SQL
+// syntax.
 func ParseExprs(sql []string) (tree.Exprs, error) {
 	if len(sql) == 0 {
 		return tree.Exprs{}, nil
 	}
 	return parseExprsWithInt(sql, defaultNakedIntType)
 }
 
-// ParseExpr is a short-hand for parseExprsWithInt([]string{sql},
-// defaultNakedIntType).
+// ParseExpr parses a SQL scalar expression. The caller is responsible
+// for ensuring that the input is, in fact, a valid SQL scalar
+// expression — the results are undefined if the string contains
+// invalid SQL syntax.
 func ParseExpr(sql string) (tree.Expr, error) {
 	return ParseExprWithInt(sql, defaultNakedIntType)
 }
 
-// ParseExprWithInt is a short-hand for parseExprsWithInt([]string{sql},
-// nakedIntType).'
+// ParseExprWithInt parses a SQL scalar expression, using the given
+// type when INT is used as type name in the SQL syntax. The caller is
+// responsible for ensuring that the input is, in fact, a valid SQL
+// scalar expression — the results are undefined if the string
+// contains invalid SQL syntax.
 func ParseExprWithInt(sql string, nakedIntType *types.T) (tree.Expr, error) {
 	exprs, err := parseExprsWithInt([]string{sql}, nakedIntType)
 	if err != nil {
@@ -364,8 +359,29 @@ func ParseExprWithInt(sql string, nakedIntType *types.T) (tree.Expr, error) {
 	return exprs[0], nil
 }
 
-// ParseType parses a column type.
-func ParseType(sql string) (tree.ResolvableTypeReference, error) {
+// GetTypeReferenceFromName turns a type name into a type
+// reference. This supports only “simple” (single-identifier)
+// references to built-in types, when the identifer has already been
+// parsed away from the input SQL syntax.
+func GetTypeReferenceFromName(typeName tree.Name) (tree.ResolvableTypeReference, error) {
+	expr, err := ParseExpr(fmt.Sprintf("1::%s", typeName.String()))
+	if err != nil {
+		return nil, err
+	}
+
+	cast, ok := expr.(*tree.CastExpr)
+	if !ok {
+		return nil, errors.AssertionFailedf("expected a tree.CastExpr, but found %T", expr)
+	}
+
+	return cast.Type, nil
+}
+
+// GetTypeFromValidSQLSyntax retrieves a type from its SQL syntax. The caller is
+// responsible for guaranteeing that the type expression is valid
+// SQL. This includes verifying that complex identifiers are enclosed
+// in double quotes, etc.
+func GetTypeFromValidSQLSyntax(sql string) (tree.ResolvableTypeReference, error) {
 	expr, err := ParseExpr(fmt.Sprintf("1::%s", sql))
 	if err != nil {
 		return nil, err

pkg/sql/parser/parse_test.go

@@ -28,7 +28,6 @@ import (
 	"github.com/cockroachdb/datadriven"
 	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 // TestParse verifies that we can parse the supplied SQL and regenerate the SQL
@@ -2901,23 +2900,6 @@ func TestParseDatadriven(t *testing.T) {
 	})
 }
 
-func TestParseTableNameWithQualifiedNames(t *testing.T) {
-	testdata := []struct {
-		name     string
-		expected string
-	}{
-		{"unique", `"unique"`},
-		{"unique.index", `"unique".index`},
-		{"table.index.primary", `"table".index.primary`},
-	}
-
-	for _, tc := range testdata {
-		name, err := parser.ParseTableNameWithQualifiedNames(tc.name)
-		require.NoError(t, err)
-		require.Equal(t, tc.expected, name.String())
-	}
-}
-
 func TestParsePanic(t *testing.T) {
 	// Replicates #1801.
 	defer func() {

pkg/sql/planner.go

@@ -466,10 +466,10 @@ func (p *planner) DistSQLPlanner() *DistSQLPlanner {
 	return p.extendedEvalCtx.DistSQLPlanner
 }
 
-// ParseType implements the tree.EvalPlanner interface.
+// GetTypeFromValidSQLSyntax implements the tree.EvalPlanner interface.
 // We define this here to break the dependency from eval.go to the parser.
-func (p *planner) ParseType(sql string) (*types.T, error) {
-	ref, err := parser.ParseType(sql)
+func (p *planner) GetTypeFromValidSQLSyntax(sql string) (*types.T, error) {
+	ref, err := parser.GetTypeFromValidSQLSyntax(sql)
 	if err != nil {
 		return nil, err
 	}

pkg/sql/schemachange/alter_column_type_test.go

@@ -38,7 +38,7 @@ func TestColumnConversions(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
 	columnType := func(typStr string) *types.T {
-		t, err := parser.ParseType(typStr)
+		t, err := parser.GetTypeFromValidSQLSyntax(typStr)
 		if err != nil {
 			panic(err)
 		}

pkg/sql/sem/tree/datum.go

@@ -4258,7 +4258,7 @@ func ParseDOid(ctx *EvalContext, s string, t *types.T) (*DOid, error) {
 		}
 		return queryOid(ctx, t, NewDString(funcDef.Name))
 	case oid.T_regtype:
-		parsedTyp, err := ctx.Planner.ParseType(s)
+		parsedTyp, err := ctx.Planner.GetTypeFromValidSQLSyntax(s)
 		if err == nil {
 			return &DOid{
 				semanticType: t,

pkg/sql/sem/tree/eval.go

@@ -3026,8 +3026,11 @@ type EvalDatabase interface {
 type EvalPlanner interface {
 	EvalDatabase
 	TypeReferenceResolver
-	// ParseType parses a column type.
-	ParseType(sql string) (*types.T, error)
+
+	// GetTypeFromValidSQLSyntax parses a column type when the input
+	// string uses the parseable SQL representation of a type name, e.g.
+	// `INT(13)`, `mytype`, `"mytype"`, `pg_catalog.int4` or `"public".mytype`.
+	GetTypeFromValidSQLSyntax(sql string) (*types.T, error)
 
 	// EvalSubquery returns the Datum for the given subquery node.
 	EvalSubquery(expr *Subquery) (Datum, error)