Skip to content

Commit b1ca714

Browse files
dfinkelharshachinta
dfinkel
and
harshachinta
authored
fix(spanner/spansql): PROTO BUNDLE and protobuf type parsing fixes (#11279)
* fix: spansql: fix NOT NULL protobuf column type parsing The protobuf type-name parser was so greedy that it failed on NOT NULL columns. In particular, it wasn't aware that unquoted tokens should be separated by dots, and that quoted tokens shouldn't be concatenated with anything else. Fix this by adding a boolean to handle that alternation and only allowing quoted IDs if nothing else has been consumed. (then bailing immediately after a quoted ID so we don't try to consume anything else) * fix: spansql: fix invalid CAST tests A misreading of the spanner docs lead to tests that indicated that casting `AS ENUM` or `AS PROTO` was valid syntax (despite not specifying _which_ protobuf enum or message type to cast to). Replace these cases with ones that validate casting to specific enum/message types. Thanks to @apstndb for calling this out on #10945. * fix spansql: CREATE PROTO BUNDLE SQL with 0 types Fix a bug in CreateProtoBundle.SQL() which unintentionally generated the DDL when there were no types listed: ``` CREATE PROTO BUNDLE (``) ``` --------- Co-authored-by: Sri Harsha CH <57220027+harshachinta@users.noreply.github.com>
1 parent 5ce14e3 commit b1ca714

4 files changed

Lines changed: 55 additions & 4 deletions

File tree

‎spanner/spansql/parser.go‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3151,21 +3151,40 @@ func (p *parser) parseProtobufTypeName(consumed string) (string, *parseError) {
31513151
possibleProtoTypeName := strings.Builder{}
31523152
possibleProtoTypeName.WriteString(consumed)
31533153
ntok := p.next()
3154+
// Pretend the last token was a dot if either the "consumed" portion we
3155+
// were given was either empty, or it actually ended in a dot.
3156+
lastTokIsDot := len(consumed) == 0 || consumed[len(consumed)-1] == '.'
31543157
PROTO_TOK_CONSUME:
31553158
for ; ntok.err == nil; ntok = p.next() {
31563159
appendVal := ntok.value
31573160
switch ntok.typ {
31583161
case unquotedID:
3162+
// only consume an unquoted token if the last one was a dot
3163+
if !lastTokIsDot {
3164+
p.back()
3165+
break PROTO_TOK_CONSUME
3166+
}
3167+
lastTokIsDot = false
31593168
case quotedID:
3169+
// It isn't valid to only quote part of a protobuf
3170+
// type-name, back out if we encounter another quoted
3171+
// value.
3172+
if possibleProtoTypeName.Len() > 0 {
3173+
p.back()
3174+
break PROTO_TOK_CONSUME
3175+
}
31603176
if !fqProtoMsgName.MatchString(ntok.string) {
31613177
return "", p.errorf("got %q, want fully qualified protobuf type", ntok.string)
31623178
}
3163-
appendVal = ntok.string
3179+
// Once we've encountered a quoted type-name, we can't consume anything else for this type-name
3180+
possibleProtoTypeName.WriteString(ntok.string)
3181+
break PROTO_TOK_CONSUME
31643182
case unknownToken:
31653183
if ntok.value != "." {
31663184
p.back()
31673185
break PROTO_TOK_CONSUME
31683186
}
3187+
lastTokIsDot = true
31693188
default:
31703189
p.back()
31713190
break PROTO_TOK_CONSUME

‎spanner/spansql/parser_test.go‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -444,8 +444,8 @@ func TestParseExpr(t *testing.T) {
444444
// Functions
445445
{`STARTS_WITH(Bar, 'B')`, Func{Name: "STARTS_WITH", Args: []Expr{ID("Bar"), StringLiteral("B")}}},
446446
{`CAST(Bar AS STRING)`, Func{Name: "CAST", Args: []Expr{TypedExpr{Expr: ID("Bar"), Type: Type{Base: String}}}}},
447-
{`CAST(Bar AS ENUM)`, Func{Name: "CAST", Args: []Expr{TypedExpr{Expr: ID("Bar"), Type: Type{Base: Enum}}}}},
448-
{`CAST(Bar AS PROTO)`, Func{Name: "CAST", Args: []Expr{TypedExpr{Expr: ID("Bar"), Type: Type{Base: Proto}}}}},
447+
{`CAST(Bar AS fizzle.bit)`, Func{Name: "CAST", Args: []Expr{TypedExpr{Expr: ID("Bar"), Type: Type{Base: Proto, ProtoRef: "fizzle.bit"}}}}},
448+
{`CAST(Bar AS fizzle.bit.baz)`, Func{Name: "CAST", Args: []Expr{TypedExpr{Expr: ID("Bar"), Type: Type{Base: Proto, ProtoRef: "fizzle.bit.baz"}}}}},
449449
{`SAFE_CAST(Bar AS INT64)`, Func{Name: "SAFE_CAST", Args: []Expr{TypedExpr{Expr: ID("Bar"), Type: Type{Base: Int64}}}}},
450450
{`EXTRACT(DATE FROM TIMESTAMP AT TIME ZONE "America/Los_Angeles")`, Func{Name: "EXTRACT", Args: []Expr{ExtractExpr{Part: "DATE", Type: Type{Base: Date}, Expr: AtTimeZoneExpr{Expr: ID("TIMESTAMP"), Zone: "America/Los_Angeles", Type: Type{Base: Timestamp}}}}}},
451451
{`EXTRACT(DAY FROM DATE)`, Func{Name: "EXTRACT", Args: []Expr{ExtractExpr{Part: "DAY", Expr: ID("DATE"), Type: Type{Base: Int64}}}}},
@@ -1942,6 +1942,26 @@ func TestParseDDL(t *testing.T) {
19421942
},
19431943
},
19441944
},
1945+
{
1946+
"CREATE TABLE IF NOT EXISTS tname (id INT64, name `foo.bar.baz.ProtoName` NOT NULL) PRIMARY KEY (id)",
1947+
&DDL{
1948+
Filename: "filename",
1949+
List: []DDLStmt{
1950+
&CreateTable{
1951+
Name: "tname",
1952+
IfNotExists: true,
1953+
Columns: []ColumnDef{
1954+
{Name: "id", Type: Type{Base: Int64}, Position: line(1)},
1955+
{Name: "name", NotNull: true, Type: Type{Base: Proto, ProtoRef: "foo.bar.baz.ProtoName"}, Position: line(1)},
1956+
},
1957+
PrimaryKey: []KeyPart{
1958+
{Column: "id"},
1959+
},
1960+
Position: line(1),
1961+
},
1962+
},
1963+
},
1964+
},
19451965
{
19461966
`CREATE INDEX IF NOT EXISTS iname ON tname (cname)`,
19471967
&DDL{

‎spanner/spansql/sql.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,12 @@ func (ci CreateIndex) SQL() string {
9999
}
100100

101101
func (cp CreateProtoBundle) SQL() string {
102+
typeList := ""
103+
if len(cp.Types) > 0 {
104+
typeList = "`" + strings.Join(cp.Types, "`, `") + "`"
105+
}
102106
// Backtick-quote all the types so we don't need to check for SQL keywords
103-
return "CREATE PROTO BUNDLE (`" + strings.Join(cp.Types, "`, `") + "`)"
107+
return "CREATE PROTO BUNDLE (" + typeList + ")"
104108
}
105109

106110
func (cv CreateView) SQL() string {

‎spanner/spansql/sql_test.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -944,6 +944,14 @@ func TestSQL(t *testing.T) {
944944
"CREATE PROTO BUNDLE (`a.b.c`, `b.d.e`)",
945945
reparseDDL,
946946
},
947+
{
948+
&CreateProtoBundle{
949+
Types: []string(nil),
950+
Position: line(1),
951+
},
952+
"CREATE PROTO BUNDLE ()",
953+
reparseDDL,
954+
},
947955
{
948956
&CreateProtoBundle{
949957
Types: []string{"a"},

0 commit comments

Comments
 (0)