sqlparser: parse empty statements as nil

[dt]

Cases like /* a comment */; /* another comment */; end up becoming
empty statements after the comments are skipped by the lexer, which then
surface as syntax errors.

This changes the parser to return a nil statement rather than an error.

Cases like this appear in the default output of mysqldump.

[sougou]

Looks like TestGetPlanPanicDuetoEmptyQuery needs to be updated. The error returned is different now.

[dt]

hm, that test has me thinking that some callers might not like this change -- they might be assuming once they make it though the err != nil check that they have a non-nil statement on which to operate.

Maybe it'd be safer to return a non-nil error value instead -- it could always be a sentinel value, so those callers that explicitly want to check for and handle (i.e. ignore) empty statements can still do so, but existing callers wouldn't risk hitting nils.

[sougou]

That's a good point. I'm thinking you can introduce an Empty statement type categorized it as iStatement. Then we can chase down all callers and add specific handling as needed. Here's the full list of callers (most are tests):

go/vt/sqlparser/ast.go|51 col 6| references to func Parse(sql string) (Statement, error)
go/vt/sqlparser/analyzer.go|268 col 15| stmt, err := Parse(sql)
go/vt/sqlparser/redact_query.go|10 col 15| stmt, err := Parse(sqlStripped)
go/vt/mysqlproxy/mysqlproxy.go|177 col 26| stmt, err := sqlparser.Parse(query)
go/vt/mysqlproxy/mysqlproxy.go|193 col 26| stmt, err := sqlparser.Parse(query)
go/cmd/query_analyzer/query_analyzer.go|104 col 24| ast, err := sqlparser.Parse(dml)
go/vt/schemamanager/tablet_executor.go|104 col 26| stat, err := sqlparser.Parse(sql)
go/vt/vttablet/tabletserver/planbuilder/ddl.go|33 col 30| statement, err := sqlparser.Parse(sql)
go/vt/vttablet/tabletserver/planbuilder/plan.go|313 col 30| statement, err := sqlparser.Parse(sql)
go/vt/vttablet/tabletserver/splitquery/split_params.go|150 col 30| statement, err := sqlparser.Parse(query.Sql)
go/vt/binlog/keyrange_filter.go|120 col 30| statement, err := sqlparser.Parse(sql)
go/vt/vtgate/planbuilder/builder.go|120 col 25| stmt, err := sqlparser.Parse(query)
go/vt/vtgate/executor.go|249 col 27| stmt, err := sqlparser.Parse(query)
go/vt/vtgate/executor.go|313 col 23| stmt, _ := sqlparser.Parse(sql)
go/vt/vtgate/executor.go|730 col 25| stmt, err := sqlparser.Parse(sql)
go/vt/vtgate/executor.go|943 col 25| stmt, err := sqlparser.Parse(sql)
go/vt/vtgate/executor.go|1084 col 25| stmt, err := sqlparser.Parse(sql)
go/vt/vtgate/executor.go|1244 col 25| stmt, err := sqlparser.Parse(sql)
go/vt/vttablet/tabletserver/query_engine.go|331 col 30| statement, err := sqlparser.Parse(sql)
go/vt/vttablet/tabletserver/tabletserver.go|1657 col 24| ast, err := sqlparser.Parse(sql)
go/vt/vtexplain/vtexplain.go|178 col 26| stmt, err := sqlparser.Parse(sql)
go/vt/vtexplain/vtexplain_vttablet.go|483 col 26| stmt, err := sqlparser.Parse(query)
go/vt/vttablet/tabletserver/splitquery/split_params_test.go|256 col 29| statement, _ := sqlparser.Parse(testCase.SQL)
go/vt/vttablet/tabletserver/planbuilder/permission_test.go|172 col 26| stmt, err := sqlparser.Parse(tcase.input)
go/vt/vttablet/tabletserver/planbuilder/plan_test.go|85 col 31| statement, err := sqlparser.Parse(tcase.input)
go/vt/vttablet/tabletserver/planbuilder/plan_test.go|133 col 33| statement, err := sqlparser.Parse(tcase.input)
go/vt/sqlparser/analyzer_test.go|128 col 16| tree, err := Parse(tc.in)
go/vt/sqlparser/ast_test.go|32 col 15| tree, err := Parse(query)
go/vt/sqlparser/ast_test.go|52 col 15| tree, err := Parse("select * from t where a = 1")
go/vt/sqlparser/ast_test.go|90 col 14| tree, err = Parse("select * from t where a = 1 or b = 1")
go/vt/sqlparser/ast_test.go|118 col 16| tree, err := Parse(query)
go/vt/sqlparser/ast_test.go|135 col 14| src, err := Parse("select foo, bar from baz order by foo")
go/vt/sqlparser/ast_test.go|140 col 14| dst, err := Parse("select * from t")
go/vt/sqlparser/ast_test.go|151 col 13| dst, err = Parse("select * from t union select * from s")
go/vt/sqlparser/ast_test.go|165 col 14| src, err := Parse("select foo, bar from baz limit 4")
go/vt/sqlparser/ast_test.go|170 col 14| dst, err := Parse("select * from t")
go/vt/sqlparser/ast_test.go|181 col 13| dst, err = Parse("select * from t union select * from s")
go/vt/sqlparser/ast_test.go|350 col 16| tree, err := Parse(tcase.in)
go/vt/sqlparser/comments_test.go|307 col 14| stmt, _ := Parse(sql)
go/vt/sqlparser/comments_test.go|351 col 13| stmt, _ := Parse("insert /*vt+ SKIP_QUERY_PLAN_CACHE=1 */ into user(id) values (1), (2)")
go/vt/sqlparser/comments_test.go|356 col 12| stmt, _ = Parse("insert into user(id) values (1), (2)")
go/vt/sqlparser/comments_test.go|361 col 12| stmt, _ = Parse("update /*vt+ SKIP_QUERY_PLAN_CACHE=1 */ users set name=1")
go/vt/sqlparser/comments_test.go|366 col 12| stmt, _ = Parse("select /*vt+ SKIP_QUERY_PLAN_CACHE=1 */ * from users")
go/vt/sqlparser/comments_test.go|371 col 12| stmt, _ = Parse("delete /*vt+ SKIP_QUERY_PLAN_CACHE=1 */ from users")
go/vt/sqlparser/normalizer_test.go|173 col 16| stmt, err := Parse(tc.in)
go/vt/sqlparser/normalizer_test.go|191 col 15| stmt, err := Parse("select * from t where :v1 = :v2 and :v2 = :v3 and :v4 in ::v5")
go/vt/sqlparser/parse_test.go|1319 col 16| tree, err := Parse(tcase.input)
go/vt/sqlparser/parse_test.go|1430 col 16| tree, err := Parse(tcase.input)
go/vt/sqlparser/parse_test.go|1513 col 16| tree, err := Parse(tcase.input)
go/vt/sqlparser/parse_test.go|1586 col 16| tree, err := Parse(tcase.input)
go/vt/sqlparser/parse_test.go|1618 col 13| _, err := Parse(tcase.input)
go/vt/sqlparser/parse_test.go|1652 col 16| tree, err := Parse(tcase.input)
go/vt/sqlparser/parse_test.go|1867 col 15| tree, err := Parse(sql)
go/vt/sqlparser/parse_test.go|2063 col 13| _, err := Parse(tcase.input)
go/vt/sqlparser/parse_test.go|2077 col 15| ast, err := Parse(sql)
go/vt/sqlparser/parse_test.go|2088 col 15| ast, err := Parse(sql)
go/vt/sqlparser/parse_test.go|2122 col 16| if _, err := Parse(benchQuery); err != nil {
go/vt/sqlparser/parsed_query_test.go|29 col 15| stmt, err := Parse("select * from a where id =:id")
go/vt/sqlparser/parsed_query_test.go|138 col 16| tree, err := Parse(tcase.query)
go/vt/sqlparser/precedence_test.go|51 col 16| tree, err := Parse(tcase.input)
go/vt/sqlparser/precedence_test.go|75 col 16| tree, err := Parse(tcase.input)
go/vt/sqlparser/precedence_test.go|102 col 16| tree, err := Parse(tcase.input)

[dt]

hmm, I gave the sentinel error approach try, so that existing callers that expect an error from an empty statement will still get one and should not change their behavior (beyond the error message) -- I've pushed a revision with that approach, which changes a relatively small number of call sites. What do you think?

[sougou]

I actually looked at the call sites. You would have to change only a few places, that I could easily identify.

But this is good for now because it's a smaller blast radius. I'll think about changing it later if I get back in here.