Rewriter infrastructure revamp

[systay]

Before this change, we have two different AST visitors - one for rewriting expressions, and one for visiting SQLNodes. Both these needed to be manually updated whenever the AST changed.

This change introduces a new visitor pattern, that can do what the older visitors could, but that is generated from the AST instead of being hand coded.

To enable this, I had to restructure the AST a little. Now, the AST structs and interface implementations live in the ast.go file, which is what the visitor generator reads to produce the visitor. The rest of the AST functionality, all methods belonging to structs and not implementing any interfaces, now live in ast_funcs.go.

I'm now using this new visitor to simplify the AST rewriting we do today: auto-parameterization (replacing values with bindvars), last_insert_id() and database() should all happen close to each other and not be done while planning.

[deepthi]

This is a major refactor/rewrite. Can you write a description for the PR that lays out how things are changing and why?

[deepthi]

shouldn't we be calling this whenever there is a change to sql.y (and sql.go)?

[systay]

not really. only when there is a change to ast.go

[deepthi]

will people need to know to call this manually? is there a test that will catch it if someone forgets to do so after changing ast.go?

[systay]

added pre-commit hook

[deepthi]

missing license header

[deepthi]

There should be some explanatory docs here.

[deepthi]

missing license header.

[sougou]

I believe this codegen is not as type-safe as I had expected. It can still panic if you pass the wrong type to cursor.Replace. In this particular case, the codegen should have generated two visitor functions: one for *AliasedExpression, and one for Expr. The cursor.Replace for *AliasedExpression would only accept an AliasedExpression instead of SQLNode.

And the pattern I described for handling the substitutions in the rewriter would allow you to compose the two.

However, this is good enough for now. Maybe we can improve this later. Let's move forward with this. I can merge once the minor nits are taken care of.

PS: Note that I am nitpicking. This is otherwise an awesome piece of work!

[sougou]

It's better to just go run go/visitorgen/main/main.go -input go/vt/sqlparser/ast.go -output=go/vt/sqlparser/rewriter.go. Then you don't have to worry about cleanup of the binary. I just tried, and it works as intended. For some reason, I had to remove the = after input.

[systay]

I can't get this to run on my Mac. All I get is:

> go run -o visitorgen go/visitorgen/main/main.go -input go/vt/sqlparser/ast.go -output=go/vt/sqlparser/rewriter.go
flag provided but not defined: -o
usage: go run [build flags] [-exec xprog] package [arguments...]
Run 'go help run' for details.
 > go run -o visitorgen go/visitorgen/main/main.go -input=go/vt/sqlparser/ast.go -output=go/vt/sqlparser/rewriter.go
flag provided but not defined: -o
usage: go run [build flags] [-exec xprog] package [arguments...]
Run 'go help run' for details.
> go run -o visitorgen go/visitorgen/main/main.go -input=go/vt/sqlparser/ast.go -output go/vt/sqlparser/rewriter.go
flag provided but not defined: -o
usage: go run [build flags] [-exec xprog] package [arguments...]
Run 'go help run' for details.
> go run -o visitorgen go/visitorgen/main/main.go -input go/vt/sqlparser/ast.go -output go/vt/sqlparser/rewriter.go
flag provided but not defined: -o
usage: go run [build flags] [-exec xprog] package [arguments...]
Run 'go help run' for details.

[dweitzman]

I agree with Sugu that go run would be much nicer here. In the error output you saw, it looks like you had go run -o vistorgen and an error about the -o. If you get rid of those arguments it seems like go run should work

[sougou]

better to use case keywordStrings[INT], keywordStrings[INTEGER]:

[sougou]

Better not to panic. This impossible in the new framework, right? Then it's better to not checkat all.

[sougou]

use bytes.Equal

[systay]

Not sure what you mean, and this is not something I changed: https://github.com/vitessio/vitess/blob/master/go/vt/sqlparser/ast.go#L2431

[sougou]

I now see why you were trying to add aliases for every expression :).

If you were already willing to pay the price of generating an alias for every select expression, there is a simpler way: As soon as you encounter an unaliased *AliasedExpression, generate an alias for it locally. Then create a new RewriteASTResult and invole a Rewrite on it. If it returns saying that something has changed, then you substitute the new alias after it returns. And always return false.

This avoids the need to maintain a separate aliases list, and also there is no need for a comingUp function.

[systay]

Nice idea. Much cleaner.

[sougou]

Could you use something other than new? It could confuse people that use it as keyword.

[dweitzman]

I'm wondering if parameterizing the parser could avoid a whole bunch of code and complexity in supporting database() and last_insert_id().

Suppose we could change this part of sql.y:

/*
  Regular function calls without special token or syntax, guaranteed to not
  introduce side effects due to being a simple identifier
*/
function_call_generic:
  sql_id openb select_expression_list_opt closeb
  {
    $$ = &FuncExpr{Name: $1, Exprs: $3}
  }

To something more in the spirit of this:

sql.go:
// vtgate can set up a custom SQL parser that replaces certain
// functions during parse time. Avoids the need to adjust the AST
// later.
func yyNewParser(f FuncCreator) yyParser {
	return &yyParserImpl{funcCreator: f}
}

func (yyrcvr *yyParserImpl) CreateSimpleFunc(name ColIdent, exprs SelectExprs) *FuncExpr {
  if (len(exprs) == 0 && yyrcvr.funcCreator != nil) {
    res := yyrcvr.funcCreator.Create(name)
    if res != nil {
       return res
    }
  }
  return &FuncExpr{Name: name, Exprs: exprs}
}

sql.y:
/*
  Regular function calls without special token or syntax, guaranteed to not
  introduce side effects due to being a simple identifier
*/
function_call_generic:
  sql_id openb select_expression_list_opt closeb
  {
    $$ = yyrcvr.CreateSimpleFunc($1, $3)
  }

If a parameterized parser could be set up, is there any functionality in the rewriter today that a parameterized parser wouldn't be able to handle?

[dweitzman]

I have the larger question of whether the rewriter can be avoided entirely, but since the code is committed now I'll send some of the pending comments I had for the small fraction of the changes I've read so far

[dweitzman]

"go run ./go/visitorgen/main" is a little shorter and avoids a "missing dot in first path element" error

[dweitzman]

What do you think about putting this package at go/ast/visitorgen so it's more clear that it's related to ast?

[systay]

Could do. I was thinking I wanted to use visitorgen for our Primitive plans as well. We don't rewrite them today, but we do visit them using the Find method, and I foresee rewriting of plans in our future as well.

Never mind - it will be easy to move it somewhere else when we do decide to start using it for more than the ast. For now, I'll move it into the sqlparser package

[dweitzman]

panicing with a printed stack trace feels a bit aggressive. Maybe just log error an exit? Same comment elsewhere

exit.Return from vitess.io/vitess/go/exit seems to be popular for most of the vitess commands, although it's not super clear to me what value it provides over directly calling os.Exit()

[dweitzman]

Seems like the fileWriter struct could be avoided here. One way:

var b bytes.Buffer
fmt.Fprintln(b, fileHeader)
fmt.Fprintln(b, ...)
err := ioutil.WriteFile(*outputFile, b.Bytes(), 0644)

[dweitzman]

How do you feel about having the Makefile invoke go generate and the //go:generate annotation could have the go run command?

Being able to use pure go build tools seems nice. Anyone who wants to use make to invoke go would have the option, although I don't think it would do much for them.

[dweitzman]

Seems like this does a serialize / memory allocations / copying even when nothing needs to change

[dweitzman]

I agree with Sugu that go run would be much nicer here. In the error output you saw, it looks like you had go run -o vistorgen and an error about the -o. If you get rid of those arguments it seems like go run should work

[systay]

Thanks for the feedback, @dweitzman. Addressed most of it with #5770

[systay]

I'm wondering if parameterizing the parser could avoid a whole bunch of code and complexity in supporting database() and last_insert_id().

...

If a parameterized parser could be set up, is there any functionality in the rewriter today that a parameterized parser wouldn't be able to handle?

You are probably right, at least for this case. I'm thinking ahead, being guided by experience and gut feeling and not much hard data. Maybe I'm overestimating my pretty singular experience and not seeing aspects that are new in this situation.

I'll try to be clear about the assumptions that lead to thinking this would be a good idea:

• I think we'll soon do more even more rewriting of the AST
• We'll want to use the same generator to produce rewriting support for the vtgate engine Primitive trees
• I value separation of concerns a lot around these things. It makes it easier to test, to understand and to reason about.
• If performance is the issue we're trying to address with the parameterized parser, I think we can use the visitorgen tool to produce pretty fast code.