sql: refactor the handling of qualified names

[knz]

This patch introduces separate types for qualified names used in
different contexts.

This fixes various panics when invalid expressions were used;
as well as enabling foreign key contraints across databases.

Fixes #8023.
Fixes #8024.
Fixes #8044.
Fixes #8045.

---

This change is

[knz]

@petermattis who do you think is best qualified (ha!) to review this?

[tamird]

Reviewed 22 of 58 files at r1.
Review status: 22 of 58 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

sql/parser/qnames.go, line 132 [r1] (raw file):

// QualifiedName is the AST node that holds a SQL name. Initially
// its Target member is an instance of UnresolvedName. Upon the first

instead of replacing the Target when normalizing, what if you made the targets carry the string representation? the contract would then require you to use the return value of normalize.

That would also allow you to remove the NameTarget interface (I think) and all the pointer use that results from it.

---

sql/parser/qnames.go, line 148 [r1] (raw file):

// to by QualifiedName.
type NameTarget interface {
  NodeFormatter

fmt.Stringer?

---

sql/testdata/insert, line 405 [r1] (raw file):

INSERT INTO return VALUES (1, 2) RETURNING x.*[1]

statement error syntax not supported: x\[1\]

throughout: what does "not supported" mean?

---

sql/testdata/join, line 279 [r1] (raw file):

0  limit  count: 1
1  join   INNER ON (a.b = c.d) AND (c.d = test.onecolumn.x)
2  join   INNER ON a.b = test.twocolumn.x

why does the database name rencer on one side but not the other?

---

sql/testdata/update, line 150 [r1] (raw file):

a   b

statement error column name "nonexistent" not found or not selected

"or not selected"?

---

sql/testdata/upsert, line 127 [r1] (raw file):

CREATE TABLE excluded (a INT PRIMARY KEY, b INT)

statement error ambiguous source name: "test.excluded"

this error seems worse since the database name is far away =/

---

Comments from Reviewable

[tamird]

Reviewed 27 of 58 files at r1.
Review status: 49 of 58 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

sql/insert.go, line 350 [r1] (raw file):

      if len(c.Selector) > 0 {
          return nil, fmt.Errorf("syntax not supported here: \"%s\"", n)

yeah this needs more words, or something

---

sql/select_qvalue.go, line 183 [r1] (raw file):

          switch arg := qname.Target.(type) {
          case parser.UnqualifiedStar:
              break

i dont think we do this, just omit?

---

sql/sort.go, line 93 [r1] (raw file):

              c = t
          default:
              return nil, fmt.Errorf("invalid syntax for ORDER BY: %s", qname.String())

no need for String()

---

sql/table.go, line 82 [r1] (raw file):

  // `expr` is assumed to be of one of several forms:
  //      database.table
  //    database.*

indentation is off

---

sql/sqlbase/keys.go, line 83 [r1] (raw file):

// NormalizeTableName normalizes the TableName using NormalizeName().
func NormalizeTableName(tn *parser.TableName) parser.TableName {

why isn't this in parser?

---

sql/testdata/subquery, line 318 [r1] (raw file):

0 select                                                   (x)        +x,unique
1 render/filter  from (test.xyz.x, test.xyz.y, test.xyz.z) (x)        +x,unique
2 scan           xyz@primary  -                            (x, y, z)  +x,unique

this is also weird in that the database name does not appear consistently.

---

Comments from Reviewable

[knz]

@tamird TYFR!

---

Review status: 49 of 58 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

sql/insert.go, line 350 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

yeah this needs more words, or something

Done.

---

sql/select_qvalue.go, line 183 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

i dont think we do this, just omit?

Done.

---

sql/sort.go, line 93 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

no need for String()

Done.

---

sql/table.go, line 82 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

indentation is off

Done.

---

sql/parser/qnames.go, line 132 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

instead of replacing the Target when normalizing, what if you made the targets carry the string representation? the contract would then require you to use the return value of normalize.

That would also allow you to remove the NameTarget interface (I think) and all the pointer use that results from it.

It's a bit more complicated than that since there are several places that accept multiple different types of qnames, but I think I get what you're saying. I'll take a shot at it.

---

sql/parser/qnames.go, line 148 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

fmt.Stringer?

Done.

---

sql/sqlbase/keys.go, line 83 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why isn't this in parser?

Circular dependency: NormalizeName lives in sqlbase which depends on parser. Unless you propose to move NormalizeName to parser too?

---

sql/testdata/join, line 279 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why does the database name rencer on one side but not the other?

"onecolumn" and "twocolumns" are table names and thus live in the database. "a" and "c" are aliases (`AS`) that only exist in the context of the query, so they are db-less.

---

sql/testdata/subquery, line 318 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this is also weird in that the database name does not appear consistently.

The explain syntax for render/filter reveal the names kept internally in `dataSourceInfo` to resolve qnames. It can contain both db-qualified (real table names) and non-db-qualified (query-local aliased sources) names and it is important to have them displayed differently to troubleshoot name resolution issues.

---

sql/testdata/update, line 150 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

"or not selected"?

Done.

---

sql/testdata/upsert, line 127 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

this error seems worse since the database name is far away =/

I made a slight improvement. Let me know what you think. Also this will be alleviated with #2037.

---

Comments from Reviewable

[tamird]

Reviewed 2 of 58 files at r1, 7 of 11 files at r2, 6 of 7 files at r3.
Review status: 51 of 58 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

sql/insert.go, line 350 [r1] (raw file):

Previously, knz (kena) wrote…

Done.

util.UnimplementedWithIssueErrorf

---

sql/parser/qnames.go, line 132 [r1] (raw file):

Previously, knz (kena) wrote…

It's a bit more complicated than that since there are several places that accept multiple different types of qnames, but I think I get what you're saying. I'll take a shot at it.

Can you give an example?

---

sql/parser/qnames.go, line 162 [r3] (raw file):

var _ NameTarget = &ColumnItem{}

func (u UnresolvedName) String() string      { return AsString(u) }

why bother making any of these receivers values? unless they are empty structs, it doesn't matter (and is less type safe).

looks like only UnqualifiedStar is the empty struct

---

sql/parser/qnames.go, line 208 [r3] (raw file):

// DatabaseName corresponds to a database identifier in a
// CREATE/DROP/RENAME DATABASE statement.
type DatabaseName struct{ Name }

o.O

---

sql/sqlbase/keys.go, line 83 [r1] (raw file):

Previously, knz (kena) wrote…

Circular dependency: NormalizeName lives in sqlbase which depends on parser. Unless you propose to move NormalizeName to parser too?

Yeah, why not?

---

Comments from Reviewable

[maddyblue]

Review status: 30 of 73 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

---

sql/parser/sql.y, line 26 [r1] (raw file):

  "go/token"

    "github.com/pkg/errors"

This entire file has lots of cases where you converted spaces to tabs. Although I agree that tabs are better, they are not standard here and should be changed back to spaces.

---

sql/parser/sql.y, line 939 [r1] (raw file):

  name
  {
    $$.val = &QualifiedName{Base: Name($1)}

tabs -> spaces example. Not commenting on the rest.

---

sql/parser/sql.y, line 1414 [r1] (raw file):

| SHOW TABLES FROM name
  {
    $$.val = &ShowTables{Database: &DatabaseName{Name:Name($4)}}

space after "Name:"

---

Comments from Reviewable

[maddyblue]

FYI I ran this against the random syntax generator for a while and it didn't find anything.

---

Review status: 30 of 73 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[RaduBerinde]

Great stuff! Some minor comments. I still have to take a closer look at the qnames and table_pattern code.

---

Review status: 30 of 73 files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

---

sql/alter_table.go, line 79 [r4] (raw file):

              return err
          }
          status, i, err := n.tableDesc.FindColumnByNormalizedName(sqlbase.ReNormalizeName(col.Name))

[nit] long line, can just do a normColName := .. on a separate line

---

sql/create.go, line 256 [r4] (raw file):

      case *parser.ColumnTableDef:
          if t.References.Table != nil {
              if _, err := t.References.Table.NormalizeTableNameWithDatabaseName(p.session.Database); err != nil {

[nit] long line (here and below), could move outside the if since it's already in its own block

---

sql/create.go, line 342 [r4] (raw file):

                  targetCol = append(targetCol, d.References.Col)
              }
              modified, err := n.resolveFK(&desc, parser.NameList{d.Name}, d.References.Table.TableName(), targetCol, d.References.ConstraintName)

[nit] long line

---

sql/data_source.go, line 141 [r4] (raw file):

// sourceAliases associates a table name (alias) to a set of columns
// in the result row of a data source.
type sourceAliases map[parser.TableName]columnRange

I like the new types! Nothing like self-documenting code :)

---

sql/data_source.go, line 155 [r4] (raw file):

// newSourceInfoForSingleTable creates a simple dataSourceInfo
// which maps the same tableAlias to all columns.
func newSourceInfoForSingleTable(tn *parser.TableName, columns []ResultColumn) *dataSourceInfo {

Why are we using *TableName instead of just TableName? A TableName is only 4 words, it doesn't warrant the cost of indirections and (potentially) heap allocations..

---

sql/data_source.go, line 432 [r4] (raw file):

      if _, ok := src.sourceAliases[*tn]; ok {
          if found {
              return fmt.Errorf("ambiguous source name: %q (relative to database %q)", tn.TableName, tn.DatabaseName)

[nit] long line

---

sql/data_source.go, line 467 [r4] (raw file):

  if _, ok := src.sourceAliases[*tn]; ok {
      if found {
          return fmt.Errorf("ambiguous source name: %q (relative to database %q)", tn.TableName, tn.DatabaseName)

[nit] long line
If you use vim, you can use these to highlight long lines:

autocmd FileType c,cpp,java,sh,python,make,go highlight OverLength ctermbg=darkred ctermfg=white
autocmd FileType c,cpp,java,sh,python,make,go match OverLength /\%101v.\+/

---

sql/rename.go, line 42 [r4] (raw file):

//          mysql >= 5.1.23 does not allow database renames.
func (p *planner) RenameDatabase(n *parser.RenameDatabase) (planNode, error) {
  // FIXME(knz) Use DatabaseName here.

we usually use TODO (it matters when we grep for them)

---

sql/sort.go, line 93 [r4] (raw file):

              c = t
          default:
              return nil, fmt.Errorf("invalid syntax for ORDER BY: %s", qname)

errors.Errorf

---

sql/table.go, line 342 [r4] (raw file):

// The pattern must be already normalized using NormalizeTablePattern().
func (p *planner) expandTableGlob(pattern parser.TablePattern) (
  parser.TableNames, error) {

if it doesn't all fit on one line, put the args on a separate line (see the code style)

---

sql/parser/qnames.go, line 205 [r4] (raw file):

/* UNUSED FOR NOW
// NormalizeDatabaseName identifies a simple database name.
func (node *QualifiedName) NormalizeDatabaseName() (DatabaseName, error) {

you can do a var _ = NormalizeDatabaseName (with a TODO) below and uncomment the code (so it doesn't bitrot)

---

sql/parser/qnames.go, line 278 [r4] (raw file):

  if len(n) == 0 || len(n) > 2 {
      return nil, fmt.Errorf("invalid table name: %q", n)

I think all these should be errors.Errorf

---

sql/parser/sql.y, line 112 [r4] (raw file):

}
func (u *sqlSymUnion) functionName() FunctionName {
      return u.val.(FunctionName)

these should be spaces (and below)

---

sql/parser/table_pattern.go, line 26 [r4] (raw file):

// TablePattern is the common interface to UnresolvedName, TableName
// and AllTablesSelector.
type TablePattern interface {

Why "pattern"?

---

Comments from Reviewable

[maddyblue]

Review status: 30 of 73 files reviewed at latest revision, 26 unresolved discussions, some commit checks failed.

---

sql/parser/qnames.go, line 208 [r3] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

o.O

This embed makes DatabaseName pick up the namePart() method and thus NamePart interface of Name. Is that intended?

---

sql/parser/qnames.go, line 274 [r4] (raw file):

  n, ok := node.Target.(UnresolvedName)
  if !ok {
      panic(fmt.Sprintf("table name already resolved: %+v (%T)", node.Target, node.Target))

Is the panic here because we should never reach this point? Why not return an error?

---

sql/parser/qnames.go, line 315 [r4] (raw file):

func (t *TableName) QualifyWithDatabase(database string) error {
  if t.DatabaseName != "" {
      return nil

Why isn't this an error?

---

sql/parser/qnames.go, line 395 [r4] (raw file):

  case UnresolvedName:
      n = t
      break

Not needed.

---

sql/parser/qnames.go, line 526 [r4] (raw file):

// ClearString causes String to return the current (possibly normalized) name instead of the
// original name (used for testing).
func (node *QualifiedName) ClearString() {

If this is only used for testing, can this method or functionality be moved to a _test.go file?

---

Comments from Reviewable

[maddyblue]

Review status: 30 of 73 files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

sql/show.go, line 226 [r4] (raw file):

// quoteName quotes based on Traditional syntax and adds commas between names.
func quoteNames(names ...string) string {
  var buf bytes.Buffer

Any reason converting to a NameList and running String on it won't work here? Looks the same to me and will prevent duplicating this code.

---

Comments from Reviewable

[maddyblue]

Review status: 30 of 73 files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

sql/parser/sql.y, line 26 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

This entire file has lots of cases where you converted spaces to tabs. Although I agree that tabs are better, they are not standard here and should be changed back to spaces.

Err, however in this case it should be tabs and the spaces were an error. Blergh.

---

Comments from Reviewable

[knz]

Some more serious refactorings in there. Also rebased over master. PTAL.

@nvanbenschoten you may want to look at the small updates to virtual_schema.go. I think there were some normalizations missing in there. LMK.

---

Review status: 14 of 85 files reviewed at latest revision, 27 unresolved discussions.

---

sql/alter_table.go, line 79 [r4] (raw file):

Previously, RaduBerinde wrote…

[nit] long line, can just do a normColName := .. on a separate line

Done.

---

sql/create.go, line 256 [r4] (raw file):

Previously, RaduBerinde wrote…

[nit] long line (here and below), could move outside the if since it's already in its own block

Not sure what you mean here?

---

sql/create.go, line 342 [r4] (raw file):

Previously, RaduBerinde wrote…

[nit] long line

Done.

---

sql/data_source.go, line 155 [r4] (raw file):

Previously, RaduBerinde wrote…

Why are we using *TableName instead of just TableName? A TableName is only 4 words, it doesn't warrant the cost of indirections and (potentially) heap allocations..

Done.

---

sql/data_source.go, line 432 [r4] (raw file):

Previously, RaduBerinde wrote…

[nit] long line

Done.

---

sql/data_source.go, line 467 [r4] (raw file):

Previously, RaduBerinde wrote…

[nit] long line
If you use vim, you can use these to highlight long lines:

autocmd FileType c,cpp,java,sh,python,make,go highlight OverLength ctermbg=darkred ctermfg=white
autocmd FileType c,cpp,java,sh,python,make,go match OverLength /\%101v.\+/

Thanks for the tip. Found the same for emacs (global-whitespace-mode). Done.

---

sql/insert.go, line 350 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

util.UnimplementedWithIssueErrorf

Done.

---

sql/rename.go, line 42 [r4] (raw file):

Previously, RaduBerinde wrote…

we usually use TODO (it matters when we grep for them)

Done.

---

sql/show.go, line 226 [r4] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Any reason converting to a NameList and running String on it won't work here? Looks the same to me and will prevent duplicating this code.

This save an array allocation (the []Name array).

---

sql/sort.go, line 93 [r4] (raw file):

Previously, RaduBerinde wrote…

errors.Errorf

Why? This is a user error, the stack of callers will not be useful.

---

sql/table.go, line 342 [r4] (raw file):

Previously, RaduBerinde wrote…

if it doesn't all fit on one line, put the args on a separate line (see the code style)

Done.

---

sql/parser/qnames.go, line 132 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Can you give an example?

PTAL

---

sql/parser/qnames.go, line 162 [r3] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why bother making any of these receivers values? unless they are empty structs, it doesn't matter (and is less type safe).

looks like only UnqualifiedStar is the empty struct

All these are array types. Pointer to pointer receiver seems ... under-efficient, isn't it?

---

sql/parser/qnames.go, line 208 [r3] (raw file):

Previously, mjibson (Matt Jibson) wrote…

This embed makes DatabaseName pick up the namePart() method and thus NamePart interface of Name. Is that intended?

Nope, wasn't. This type disappears.

---

sql/parser/qnames.go, line 205 [r4] (raw file):

Previously, RaduBerinde wrote…

you can do a var _ = NormalizeDatabaseName (with a TODO) below and uncomment the code (so it doesn't bitrot)

It's gone now.

---

sql/parser/qnames.go, line 274 [r4] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Is the panic here because we should never reach this point? Why not return an error?

Code is gone now.

---

sql/parser/qnames.go, line 278 [r4] (raw file):

Previously, RaduBerinde wrote…

I think all these should be errors.Errorf

Again, not sure why since these are user errors. Perhaps I need someone to explain me when we use one or the other.

---

sql/parser/qnames.go, line 315 [r4] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Why isn't this an error?

If the table name is not qualified, this method should be a no-op (it's always called, regardless of whether the TableName is already qualified or not).

---

sql/parser/qnames.go, line 395 [r4] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Not needed.

Code is gone.

---

sql/parser/qnames.go, line 526 [r4] (raw file):

Previously, mjibson (Matt Jibson) wrote…

If this is only used for testing, can this method or functionality be moved to a _test.go file?

Done.

---

sql/parser/sql.y, line 26 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Err, however in this case it should be tabs and the spaces were an error. Blergh.

I think I fixed it. PTAL

---

sql/parser/sql.y, line 939 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

tabs -> spaces example. Not commenting on the rest.

Done.

---

sql/parser/sql.y, line 1414 [r1] (raw file):

Previously, mjibson (Matt Jibson) wrote…

space after "Name:"

Done.

---

sql/parser/sql.y, line 112 [r4] (raw file):

Previously, RaduBerinde wrote…

these should be spaces (and below)

Done.

---

sql/parser/table_pattern.go, line 26 [r4] (raw file):

Previously, RaduBerinde wrote…

Why "pattern"?

Because it can match zero or more things.

---

sql/sqlbase/keys.go, line 83 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Yeah, why not?

Filed as https://github.com//issues/8320 .

---

Comments from Reviewable

[knz]

@RaduBerinde I'd like your feedback on this one. I think the entire function can be removed.

[RaduBerinde]

If we never change them, yeah that can be removed (and "change" I mean both the interface references inside the slices, and the objects they refer).

Note that as is this function doesn't truly make a copy - the new UnresolvedNames "points" to the same []NamePart.

We should call out the immutability explicitly where we define UnresolvedNames. It is a slice of slices of interface pointers so it's pretty much impossible to see or verify the immutability just from looking at the code.

[danhhz]

Review status: 14 of 85 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

sql/upsert.go, line 98 [r6] (raw file):

Previously, knz (kena) wrote…

@paperstreet @dt I'd like your feedback here. The code prior to this point was not checking the database name prefix to these expressions. Was this on purpose, or do we need to check the db name is not set?

This should do whatever UPDATE does, modulo the synthetic "excluded" table

---

Comments from Reviewable

[RaduBerinde]

How many NameParts can be in a slice here? If it's at most two or three, consider making this a fixed-size array (with nil entries for what's unneeded) to avoid the indirection and extra allocations.

[RaduBerinde]

I'm having a lot of trouble reviewing this (too many files)..

Do you think you could break it up into three commits by files (everything in parser/ in one commit, everything in testdata/ in another, and the rest in another)? You can squash them back before merging.

---

Review status: 14 of 85 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

sql/parser/qnames.go, line 278 [r4] (raw file):

Previously, knz (kena) wrote…

Again, not sure why since these are user errors. Perhaps I need someone to explain me when we use one or the other.

Ah I was under the impresssion we have switched to always using `errors` but I think you're right, never mind.

---

Comments from Reviewable

[RaduBerinde]

So QualifiedName is gone now? There are a few references left in comments and error messages if you grep for it.

---

Review status: 14 of 85 files reviewed at latest revision, 29 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[CLAassistant]

All committers have signed the CLA.

[knz]

@RaduBerinde I went through the remaining mentions of "qualified". Thanks for highlighting that I didn't pass -i to git grep.

@nvanbenschoten: this is a different normalization! the Normalize methods in sql.parser are to transform an UnresolvedName into a TableName, ColumnItem etc. The NormalizeName function in sqlbase normalizes the unicode encoding and changes the name to lowercase. I extended #8320 to clarify this.

---

Review status: 13 of 89 files reviewed at latest revision, 28 unresolved discussions, some commit checks failed.

---

cli/zone.go, line 183 [r16] (raw file):

Previously, RaduBerinde wrote…

This is indeed hacky as we are using TableName for something that may not be a table.. At the end of the day, isn't this function simply splitting at '.' and returning the tokens? Can't it just do that directly and not go through TableName?

The issue is that you want to go through the lexer to support quoted names containing special characters.

---

sql/data_source.go, line 466 [r18] (raw file):

Previously, RaduBerinde wrote…

s/relative to/within/

Done.

---

sql/upsert.go, line 98 [r6] (raw file):

Previously, knz (kena) wrote…

Ok thanks I'll have a look. Probably accidentally OK due to a restriction on the grammar.

Hmm so actually my confusion was due to a problem in this code which I didn't cause :) Filed as #8330.

---

sql/parser/function_name.go, line 36 [r9] (raw file):

Previously, RaduBerinde wrote…

We never use this one-line syntax, FunctionName should be on its own line. Also, does it have to be embedded (as opposed to type NormalizableFunctionName FunctionName)?

It must be a mutable field in a struct (as opposed to a type alias) so that the `Normalize` method can change it in-place. Perhaps this API can be tweaked and have the context (the FunExpr) do `f.Name = f.Name.Normalize()` instead, in which case this struct can go. What do you think?

---

sql/parser/function_name_test.go, line 1 [r9] (raw file):

Previously, RaduBerinde wrote…

2016

Done.

---

sql/parser/table_name.go, line 189 [r9] (raw file):

Previously, RaduBerinde wrote…

Why not []TableName

Sure, why not.

---

sql/parser/table_name.go, line 231 [r9] (raw file):

Previously, RaduBerinde wrote…

Can't it be []TableNameWithIndex?

This one doesn't work well, because the TableNameWithIndex objects are allocated on the heap anyways by the parser (they have their own non-terminal), so we're not saving any allocation by doing this and instead adding copy overhead.

---

sql/parser/table_pattern.go, line 68 [r9] (raw file):

Previously, RaduBerinde wrote…

can just return directly (same below)

Done.

---

sql/parser/table_pattern.go, line 76 [r9] (raw file):

Previously, RaduBerinde wrote…

Is an empty TablePattern and nil error really what we want in the "default" case?

Nice catch. This was indeed a bug.

---

sql/parser/var_name.go, line 144 [r9] (raw file):

Previously, RaduBerinde wrote…

is embedding the table necessary? it would be strange for any TableName methods to work on a ColumnItem but to only refer to the table..

Done.

---

Comments from Reviewable

[RaduBerinde]

Review status: 12 of 93 files reviewed at latest revision, 28 unresolved discussions, all commit checks successful.

---

sql/parser/function_name.go, line 36 [r9] (raw file):

Previously, knz (kena) wrote…

It must be a mutable field in a struct (as opposed to a type alias) so that the Normalize method can change it in-place. Perhaps this API can be tweaked and have the context (the FunExpr) do f.Name = f.Name.Normalize() instead, in which case this struct can go. What do you think?

There is no functional difference between the two. You can have a method that mutates a type alias as long as it has a pointer receiver (just like with a struct). E.g. https://play.golang.org/p/JDbFKJEWbE

The only reason to prefer an embedding is if we want to automatically inherit the methods defined on FunctionName, but it doesn't look like that is the case here.

---

Comments from Reviewable

[knz]

Review status: 12 of 93 files reviewed at latest revision, 17 unresolved discussions, all commit checks successful.

---

sql/parser/function_name.go, line 36 [r9] (raw file):

Previously, RaduBerinde wrote…

There is no functional difference between the two. You can have a method that mutates a type alias as long as it has a pointer receiver (just like with a struct). E.g. https://play.golang.org/p/JDbFKJEWbE

The only reason to prefer an embedding is if we want to automatically inherit the methods defined on FunctionName, but it doesn't look like that is the case here.

What you propose doesn't work if the type being aliased is an interface type. (This is case here) Look at what your playpen example says: https://play.golang.org/p/Ujke_njXQw

---

Comments from Reviewable

[RaduBerinde]

Review status: 12 of 92 files reviewed at latest revision, 17 unresolved discussions, all commit checks successful.

---

sql/parser/function_name.go, line 36 [r9] (raw file):

Previously, knz (kena) wrote…

What you propose doesn't work if the type being aliased is an interface type. (This is case here)
Look at what your playpen example says: https://play.golang.org/p/Ujke_njXQw

Oh! So you can't alias a type to an interface ('type x someint' is an interface not a type). Interesting!

If we don't need the embedding we can make FunctionName a regular (named) struct member. LGTM either way.

---

Comments from Reviewable

[knz]

Review status: 12 of 92 files reviewed at latest revision, 17 unresolved discussions, all commit checks successful.

---

sql/parser/function_name.go, line 36 [r9] (raw file):

Previously, RaduBerinde wrote…

Oh! So you can't alias a type to an interface ('type x someint' is an interface not a type). Interesting!

If we don't need the embedding we can make FunctionName a regular (named) struct member. LGTM either way.

Yeah I made it a named member as per your previous suggestion.

---

Comments from Reviewable

[knz]

Hi guys so since I am leaving on vacation tomorrow I am passing stewardship of this PR to @RaduBerinde. Ideally there should be a completed review from someone else before the PR is merged (it got just one thumb up for now).

[RaduBerinde]

Rebased. Let me know if there are any major outstanding issues. Note that since I'm not the original author of the change I may be missing subtle details and original motivation for various decisions so I will try to minimize further changes.

---

Review status: 12 of 92 files reviewed at latest revision, 17 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[maddyblue]

Review status: 12 of 92 files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

---

sql/show.go, line 226 [r4] (raw file):

Previously, knz (kena) wrote…

This save an array allocation (the []Name array).

My opinion is this happens so infrequently it's not worth the extra code to save an allocation. Up to you.

---

sql/parser/parse.go, line 166 [r19] (raw file):

// ParseTableNameTraditional parses a table name.
func ParseTableNameTraditional(sql string) (*TableName, error) {
  stmt, err := ParseOneTraditional(fmt.Sprintf("ALTER TABLE %s RENAME TO x", sql))

Is this really the best way we have to do this? :( Do we need to use encodeSQLIdent to make sure sql is properly escaped?

---

sql/parser/parse.go, line 172 [r19] (raw file):

  rename, ok := stmt.(*RenameTable)
  if !ok {
      return nil, errors.Errorf("expected a SHOW COLUMN statement, but found %T", stmt)

"expected an ALTER TABLE statement"

---

sql/parser/sql.y, line 1473 [r19] (raw file):

| /* EMPTY */
  {
      $$.val = NameList(nil)

tabs -> spaces

---

sql/parser/sql.y, line 1521 [r19] (raw file):

    /* SKIP DOC */
    $$.val = &InterleaveDef{
          Parent: NormalizableTableName{UnresolvedName{Name($4)}},

tabs -> spaces

---

sql/parser/sql.y, line 1716 [r19] (raw file):

| /* EMPTY */
  {
    $$.val = NameList(nil)

tabs -> spaces

---

sql/parser/sql.y, line 1726 [r19] (raw file):

| /* EMPTY */
  {
    $$.val = NameList(nil)

tabs -> spaces

---

sql/parser/sql.y, line 2099 [r19] (raw file):

| /* EMPTY */
  {
      $$.val = NameList(nil)

tabs -> spaces

---

sql/parser/sql.y, line 3620 [r19] (raw file):

| CURRENT_DATE
  {
      $$.val = &FuncExpr{Name: WrapQualifiedFunctionName($1)}

fix spacing

---

sql/parser/sql.y, line 4191 [r19] (raw file):

  table_pattern
  {
      $$.val = TablePatterns{$1.unresolvedName()}

spacing

---

Comments from Reviewable

[maddyblue]

Just some minor comments, but overall

---

Review status: 12 of 92 files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[RaduBerinde]

Thanks Matt!

---

Review status: 12 of 92 files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

---

sql/show.go, line 226 [r4] (raw file):

Previously, mjibson (Matt Jibson) wrote…

My opinion is this happens so infrequently it's not worth the extra code to save an allocation. Up to you.

Agreed, done.

---

sql/parser/parse.go, line 166 [r19] (raw file):

Previously, mjibson (Matt Jibson) wrote…

Is this really the best way we have to do this? :( Do we need to use encodeSQLIdent to make sure sql is properly escaped?

AFAICT this is only used for testing. Since we are parsing a name I would think we expect it to be already escaped (honestly I don't think we ever use it with names that need escaping).

---

sql/parser/parse.go, line 172 [r19] (raw file):

Previously, mjibson (Matt Jibson) wrote…

"expected an ALTER TABLE statement"

Fixed.

---

sql/parser/sql.y, line 1473 [r19] (raw file):

Previously, mjibson (Matt Jibson) wrote…

tabs -> spaces

Fixed all tabs in the file

---

sql/parser/sql.y, line 3620 [r19] (raw file):

Previously, mjibson (Matt Jibson) wrote…

fix spacing

Fixed

---

sql/parser/sql.y, line 4191 [r19] (raw file):

Previously, mjibson (Matt Jibson) wrote…

spacing

Fixed

---

Comments from Reviewable

[maddyblue]

Review status: 12 of 92 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

---

sql/parser/parse.go, line 166 [r19] (raw file):

Previously, RaduBerinde wrote…

AFAICT this is only used for testing. Since we are parsing a name I would think we expect it to be already escaped (honestly I don't think we ever use it with names that need escaping).

It's also used in cli/zone.go. That just passes in a string from the command line, so it's possible it could be a table with a space or other weird chars in it.

---

Comments from Reviewable

[RaduBerinde]

Review status: 12 of 92 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

---

sql/parser/parse.go, line 166 [r19] (raw file):

Previously, mjibson (Matt Jibson) wrote…

It's also used in cli/zone.go. That just passes in a string from the command line, so it's possible it could be a table with a space or other weird chars in it.

Ah, good catch! But we still have to support being provided with a correctly escaped table name, and if we encode it again it would be doubly-escaped and that wouldn't work. I don't think there's a safety concern here as we are not executing any statement with the table name, just parsing it. But I do agree it is sketchy so I am filing an issue to clean this up and leaving a TODO in this change.

---

Comments from Reviewable

[RaduBerinde]

@dt after merging with your changes I am hitting a timeout during the logic test, on CREATE TABLE crossdb (y INT PRIMARY KEY, FOREIGN KEY (y) REFERENCES test2.other(x));. Could you take a quick look over the changes create.go and see if you spot a mistake?

[RaduBerinde]

@dt never mind, the problem is due to some new FK tests (crossdb) and is reproducible on master, I will file an issue.