Merge pull request #2861 from dolthub/zachmu/quote-identifiers

Added configurable options for how to quote identifiers in column defaults

Author: zachmu

memory/table.go

@@ -134,7 +134,7 @@ func stripTblNames(e sql.Expression) (sql.Expression, transform.TreeIdentity, er
 	case *expression.GetField:
 		// strip table names
 		ne := expression.NewGetField(e.Index(), e.Type(), e.Name(), e.IsNullable())
-		ne = ne.WithBackTickNames(e.IsBackTickNames())
+		ne = ne.WithQuotedNames(sql.GlobalParser, e.IsQuotedIdentifier())
 		return ne, transform.NewTree, nil
 	default:
 	}

sql/analyzer/analyzer.go

@@ -266,6 +266,7 @@ func (ab *Builder) Build() *Analyzer {
 		Catalog:      NewCatalog(ab.provider),
 		Coster:       memo.NewDefaultCoster(),
 		ExecBuilder:  rowexec.DefaultBuilder,
+		Parser:       sql.GlobalParser,
 	}
 }
 
@@ -288,6 +289,8 @@ type Analyzer struct {
 	ExecBuilder sql.NodeExecBuilder
 	// Runner represents the engine, which is represented as a separate interface to work around circular dependencies
 	Runner StatementRunner
+	// Parser is the parser used to parse SQL statements.
+	Parser sql.Parser
 }
 
 // NewDefault creates a default Analyzer instance with all default Rules and configuration.

sql/analyzer/resolve_column_defaults.go

@@ -307,15 +307,15 @@ func stripTableNamesFromDefault(e *expression.Wrapper) (sql.Expression, transfor
 	return expression.WrapExpression(&nd), transform.NewTree, nil
 }
 
-func backtickDefaultColumnValueNames(ctx *sql.Context, _ *Analyzer, n sql.Node, _ *plan.Scope, _ RuleSelector, qFlags *sql.QueryFlags) (sql.Node, transform.TreeIdentity, error) {
-	span, ctx := ctx.Span("backtickDefaultColumnValueNames")
+func quoteDefaultColumnValueNames(ctx *sql.Context, a *Analyzer, n sql.Node, _ *plan.Scope, _ RuleSelector, qFlags *sql.QueryFlags) (sql.Node, transform.TreeIdentity, error) {
+	span, ctx := ctx.Span("quoteDefaultColumnValueNames")
 	defer span.End()
 
 	return transform.Node(n, func(n sql.Node) (sql.Node, transform.TreeIdentity, error) {
 		switch node := n.(type) {
 		case *plan.AlterDefaultSet:
 			eWrapper := expression.WrapExpression(node.Default)
-			newExpr, same, err := backtickDefault(eWrapper)
+			newExpr, same, err := quoteIdentifiers(a.Parser, eWrapper)
 			if err != nil {
 				return node, transform.SameTree, err
 			}
@@ -335,7 +335,7 @@ func backtickDefaultColumnValueNames(ctx *sql.Context, _ *Analyzer, n sql.Node,
 					return e, transform.SameTree, nil
 				}
 
-				return backtickDefault(eWrapper)
+				return quoteIdentifiers(a.Parser, eWrapper)
 			})
 		case *plan.ResolvedTable:
 			ct, ok := node.Table.(*information_schema.ColumnsTable)
@@ -354,7 +354,7 @@ func backtickDefaultColumnValueNames(ctx *sql.Context, _ *Analyzer, n sql.Node,
 					return e, transform.SameTree, nil
 				}
 
-				return backtickDefault(eWrapper)
+				return quoteIdentifiers(a.Parser, eWrapper)
 			})
 
 			if err != nil {
@@ -376,7 +376,7 @@ func backtickDefaultColumnValueNames(ctx *sql.Context, _ *Analyzer, n sql.Node,
 	})
 }
 
-func backtickDefault(wrap *expression.Wrapper) (sql.Expression, transform.TreeIdentity, error) {
+func quoteIdentifiers(parser sql.Parser, wrap *expression.Wrapper) (sql.Expression, transform.TreeIdentity, error) {
 	newDefault, ok := wrap.Unwrap().(*sql.ColumnDefaultValue)
 	if !ok {
 		return wrap, transform.SameTree, nil
@@ -388,7 +388,7 @@ func backtickDefault(wrap *expression.Wrapper) (sql.Expression, transform.TreeId
 
 	newExpr, same, err := transform.Expr(newDefault.Expr, func(expr sql.Expression) (sql.Expression, transform.TreeIdentity, error) {
 		if e, isGf := expr.(*expression.GetField); isGf {
-			return e.WithBackTickNames(true), transform.NewTree, nil
+			return e.WithQuotedNames(parser, true), transform.NewTree, nil
 		}
 		return expr, transform.SameTree, nil
 	})

sql/analyzer/rule_ids.go

@@ -81,7 +81,7 @@ const (
 	validateDeleteFromId        // validateDeleteFrom
 
 	// after all
-	cacheSubqueryAliasesInJoinsId    // cacheSubqueryAliasesInJoins
-	BacktickDefaulColumnValueNamesId // backtickDefaultColumnValueNames
-	TrackProcessId                   // trackProcess
+	cacheSubqueryAliasesInJoinsId  // cacheSubqueryAliasesInJoins
+	QuoteDefaultColumnValueNamesId // quoteDefaultColumnValueNames
+	TrackProcessId                 // trackProcess
 )

sql/analyzer/ruleid_string.go

@@ -23,7 +23,7 @@ func init() {
 		{applyProceduresId, applyProcedures},
 		{inlineSubqueryAliasRefsId, inlineSubqueryAliasRefs},
 		{cacheSubqueryAliasesInJoinsId, cacheSubqueryAliasesInJoins},
-		{BacktickDefaulColumnValueNamesId, backtickDefaultColumnValueNames},
+		{QuoteDefaultColumnValueNamesId, quoteDefaultColumnValueNames},
 		{TrackProcessId, trackProcess},
 	}
 }

sql/analyzer/rules.go

@@ -38,7 +38,11 @@ type GetField struct {
 	fieldType2 sql.Type2
 	nullable   bool
 
-	backTickNames bool
+	// parser is the parser used to parse the expression and print it
+	parser sql.Parser
+
+	// quoteName indicates whether the field name should be quoted when printed with String()
+	quoteName bool
 }
 
 var _ sql.Expression = (*GetField)(nil)
@@ -161,12 +165,16 @@ func (p *GetField) WithChildren(children ...sql.Expression) (sql.Expression, err
 }
 
 func (p *GetField) String() string {
+	// We never quote anything if the table identifier is present. Quoting the field name is a very narrow use case
+	// used only for serializing column default values and related fields, in which case the table name will always be
+	// stripped away. The output of this method is load-bearing in many places of analysis and execution.
 	if p.table == "" {
-		if p.backTickNames {
-			return "`" + p.name + "`"
+		if p.quoteName {
+			return p.parser.QuoteIdentifier(p.name)
 		}
 		return p.name
 	}
+
 	return p.table + "." + p.name
 }
 
@@ -188,16 +196,17 @@ func (p *GetField) WithIndex(n int) sql.Expression {
 	return &p2
 }
 
-// WithBackTickNames returns a copy of this expression with the backtick names flag set to the given value.
-func (p *GetField) WithBackTickNames(backtick bool) *GetField {
+// WithQuotedNames returns a copy of this expression with the backtick names flag set to the given value.
+func (p *GetField) WithQuotedNames(parser sql.Parser, quoteNames bool) *GetField {
 	p2 := *p
-	p2.backTickNames = backtick
+	p2.quoteName = quoteNames
+	p2.parser = parser
 	return &p2
 }
 
-// IsBackTickNames returns whether the field name should be quoted with backticks.
-func (p *GetField) IsBackTickNames() bool {
-	return p.backTickNames
+// IsQuotedIdentifier returns whether the field name should be quoted.
+func (p *GetField) IsQuotedIdentifier() bool {
+	return p.quoteName
 }
 
 // CollationCoercibility implements the interface sql.CollationCoercible.

sql/expression/get_field.go

@@ -16,6 +16,7 @@ package sql
 
 import (
 	"context"
+	"fmt"
 	trace2 "runtime/trace"
 	"strings"
 	"unicode"
@@ -44,6 +45,10 @@ type Parser interface {
 	// the index of the start of the next query. If |query| represents a no-op statement, such as ";" or "-- comment",
 	// then implementations must return Vitess' ErrEmpty error.
 	ParseOneWithOptions(context.Context, string, ast.ParserOptions) (ast.Statement, int, error)
+	// QuoteIdentifier returns the identifier given quoted according to this parser's dialect. This is used to
+	// standardize identifiers that cannot be parsed without quoting, because they break the normal identifier naming
+	// rules (such as containing spaces)
+	QuoteIdentifier(identifier string) string
 }
 
 var _ Parser = &MysqlParser{}
@@ -99,3 +104,7 @@ func RemoveSpaceAndDelimiter(query string, d rune) string {
 		return r == d || unicode.IsSpace(r)
 	})
 }
+
+func (m *MysqlParser) QuoteIdentifier(identifier string) string {
+	return fmt.Sprintf("`%s`", strings.ReplaceAll(identifier, "`", "``"))
+}