MySQL support

[jmhodges]

Love this project idea! All of my tools are built on mysql 5.x and it'd be great to have that supported.

[kyleconroy]

There's a high-quality open source MySQL parser included in the vitess project (https://godoc.org/github.com/youtube/vitess/go/vt/sqlparser#BuildParsedQuery). I'll take a look into using that.

[clintberry]

We actually are looking at using this for vitess in particular. Super excited you are at least looking at it.

[mightyguava]

TiDB also has a pretty good standalone MySQL parser worth taking a look at: https://github.com/pingcap/parser

I've been using sqlc in a hobby project and really love it. Would love to see MySQL support so I can use it at work too.

[cmoog]

@kyleconroy
This project is super cool and basically the best solution I've come across for adding type safety and code generation to the data access layer. I'm in the process of adding MySQL support on a fork branch here.
A few things to note:

• much of the generator logic is specific to the pg parser. On my branch, I've attempted to abstract the Go generator logic to be parser-agnostic. To further clean and decouple the components, I think it would be wise to separate gen into its own package.
• how do you envision the user specifying their desired parser? From the CLI or in the config file?

The status of my branch is still very rough and lacking many implementations such as

• settings and overrides
• file reading and writing
• imports
• a few parameter conditions that fail

It's basically only unit tests validating that basic parsing and generation is functional.

[kyleconroy]

@jmhodges @clintberry I just merged experimental MySQL support. It's not production ready, but should work for simple queries and schema. Please try it out and report any bugs.

[jmhodges]

Wilco! Found some. Will make separate tickets (I can bring them back here and close the ones I make if you'd like)

[mightyguava]

So i was chatting with @jmhodges earlier, I'd like to recommend against vitess for a couple reasons, after talking through with systay (Andres) (maintainer of the vitess query/parsing code)

1. vitess's parser intentionally doesn't support all of mysql syntax. The parser only supports what vitess supports, intentionally failing on things that vitess does not, so the user sees an error instead of getting their statement ignored
2. if we want to add features to the parser, we would need to first add a second parsing pass that would throw on unsupported vitess features. Otherwise we would cause problems for Vitess. This is probably simple, but enumerating the features and the actual work might be tedious
3. there is an unknown number of features that the vitess parser doesn't support from the mysql syntax, and the method for discovering incompatibilities will probably be by writing test cases and parsing a ton of code examples. The burden is already being felt by @jmhodges in implementing alter support mysql alter add inside of schema file fails #251
4. while planetscale (the company that builds vitess) is in the process of expanding its mysql compatibility, extending the sql parser isn't quite on their list of priorities yet, so if we want to use their parser, the work is on us

Meanwhile, i was poking around tidb's parser, and according to this they should be fairly complete: pingcap/tidb#11486. This seems like a happier path. I'm pretty noob to compilers and AST so I could be totally wrong too.

I know @jmhodges and @jmhodges have done a ton of work around the current parser that uses Vitess, so I'm curious about your thoughts. No idea how much work would need to be redone to switch parsers. I personally am just interested in getting the best possible MySQL support so whatever gets us there is best! And I'd be happy to contribute if you think this is a good idea, I'm just a compilers noob so it might take me a while.

[cmoog]

Meanwhile, i was poking around tidb's parser, and according to this they should be fairly complete: pingcap/tidb#11486. This seems like a happier path. I'm pretty noob to compilers and AST so I could be totally wrong too.

Upon closer examination, this does look like a viable alternative to vitess. I'll take a closer look at the documentation and try to assess the work and how difficult a conversion would be.

I personally am just interested in getting the best possible MySQL support so whatever gets us there is best!

Me too! I'll definitely defer to @jmhodges on the difficulty of adding features to vitess, so I agree we should seriously consider migrating to tidbif the conversion effort justifies it.

[jmhodges]

After writing my first patch to sqlparser, I can see how we might have more work than doing a conversion to the tidb parser. I think it's someone to trying out the conversion is worth a shot.

(As a helper, do we have any contacts working on that project?)

[mightyguava]

I have contacts! Most of their company is based in China though and they currently are on the Chinese New Year holidays, till sometime next week.

Generally, I’ve found them super receptive and helpful.

[shenli]

Hello, TiDB developer here. This is a cool project! We would love to help on this issue.
One aim of the parser is to provide the best MySQL compatible parser to all the Golang projects.

[gregwebs]

Not sure if people here are familiar with the project, but I use go-queryset which supports MySQL and is similar in that it generates type-safe Go code from definitions. But the schema is defined with go structs. For simple schemas it is probably easier to make definitions with go structs, but for more complex schemas declaring them in SQL could be a lot nicer. From a first look, sqlc appears to generate more polished high-level query code, but it may be much less extensible than what go-queryset generates. I normally write my own higher-level wrappers around go-queryset functions (for example to just return a list like the list functions in this project do rather than having to pass in an array).
Both projects are MIT licensed, so I could see this project benefiting from using some of the extensible query code in go-queryset.

[kyleconroy]

Hello, TiDB developer here. This is a cool project! We would love to help on this issue.
One aim of the parser is to provide the best MySQL compatible parser to all the Golang projects.

@shenli Thank you so much for commenting. I wanted to make sure we had buy-in from TiDB before we switched parsers. It sounds like both projects (vitess and TiDB) would like us to use the TiDB parser! Consider this an official sign-off on the switch.

but I use go-queryset ... But the schema is defined with go structs

@gregwebs sqlc is dedicated to being SQL-first. go-queryset looks cool, but I think we're set with the current direction of the project.

[kyleconroy]

Oh, one last question for you @shenli. What's the best way for us to report missing features in the pingcap MySQL parser? Open a GitHub issue?

[cmoog]

Consider this an official sign-off on the switch.

Great to hear. I'll get working on the conversion.

[jmhodges]

Okay, one problem I've seen with a conversion to the TiDB parser API is that it does not allow for returning the byte locations of syntax errors and adding that functionality would break its backwards compatibility.

The possible parse errors are exposed as package-level variables (e.g. ErrSyntax, ErrParse) and those would have to instead by dynamically created to add where in the sql statement that parsing failed.

[mightyguava]

meta: (just want to clarify that vitess didn't recommend using the tidb parser. I recommended that after chatting through some details with a vitess maintainer, and gauged that it may be more work to use the Vitess parser than to switch. Vitess would love for us to contribute to their parser and get it to be more mysql compatible. But for the moment we would be the ones doing the work to make it so and not break their existing guarantees)

[mightyguava]

The possible parse errors are exposed as package-level variables (e.g. ErrSyntax, ErrParse) and those would have to instead by dynamically created to add where in the sql statement that parsing failed.

It looks to me errors are returned as *terror.Error which has file and line. Extending it to return byte locations could be tedious, but I don't think it would be backwards incompatible. (Also, I think the current behavior is to match MySQL, which only tells you the line number when you have syntax errors... Not that's useful lol)

[jmhodges]

I don't see any place that the file , or line field is set. The code below emits

error: line 1 column 7 near "asfdadf" , *errors.errorString

which is just a fmt.Errorf, not a structured error type, it seems. Or did I get something wrong?

package main

import (
	"fmt"
	"log"

	"github.com/pingcap/errors"
	"github.com/pingcap/parser"
	_ "github.com/pingcap/tidb/types/parser_driver"
)

func main() {
	p := parser.New()
	_, _, err := p.Parse("asfdadf", "", "")
	if err != nil {
		log.Printf("error: %s, %T", errors.Unwrap(err), errors.Unwrap(err))
	}
	fmt.Println("worked but def shouldn't have")
}

[jmhodges]

Which is honestly odd, because I'm not sure why that's not getting a specific error type at all? (Before the Unwrap, it's an errors.withStack). With all that error code in the parser, it seems like all that gets thrown away before being returned?

I surely could be missing something, but I still don't see a good place to put this in. Maybe @cmoog is having better luck

[jmhodges]

Bah, okay, ErrSyntax is only returned if there's zero statements in a call to ParseOneStmt and ErrParse isn't used at all.

So, I was wrong about the backwards incompatibility!

We could expose this stuff! It looks like this error string is coming from Scanner.Errorf method and that seems like the part that needs to return a structured error type we can pull back apart.

[shenli]

Oh, one last question for you @shenli. What's the best way for us to report missing features in the pingcap MySQL parser? Open a GitHub issue?

Yes, please open an issue in pingcap/parser repo.

[shenli]

I don't see any place that the file , or line field is set. The code below emits

error: line 1 column 7 near "asfdadf" , *errors.errorString

which is just a fmt.Errorf, not a structured error type, it seems. Or did I get something wrong?

package main

import (
	"fmt"
	"log"

	"github.com/pingcap/errors"
	"github.com/pingcap/parser"
	_ "github.com/pingcap/tidb/types/parser_driver"
)

func main() {
	p := parser.New()
	_, _, err := p.Parse("asfdadf", "", "")
	if err != nil {
		log.Printf("error: %s, %T", errors.Unwrap(err), errors.Unwrap(err))
	}
	fmt.Println("worked but def shouldn't have")
}

Basically the line and col are set here: https://github.com/pingcap/parser/blob/master/lexer.go#L812 . The reader handles the raw SQL string.

[mightyguava]

@cmoog @jmhodges how's the transition progressing, if you are still working on it? I was hoping to get MySQL support for Kotlin.

[cmoog]

@mightyguava My first pass didn't go so well. That said, I should have an update by the end of this week or early next. If I can't get something together by then, I'd be all good with handing it off to another contributor.

[mightyguava]

Thanks for the hard work!

[kyleconroy]

At long last, I can close this issue! MySQL support will be released in version 1.6.0, which is going out in the next few days. Thanks to everyone that's worked on MySQL support.