google-cloud-go icon indicating copy to clipboard operation
google-cloud-go copied to clipboard

feat(spanner/spansql): add support for protobuf column types & Proto bundles

Open dfinkel opened this issue 1 year ago • 3 comments

feat(spansql): CREATE/ALTER/DROP PROTO BUNDLE

Add support for parsing and serializing CREATE, ALTER and DROP PROTO BUNDLE DDL statements.

feat(spanner/spansql): support for protobuf types

Now that Spanner supports protobuf message and enum-typed columns and casts, add support for parsing those those types.

Since protobuf columns aren't distinguished by a keyword, adjust the parser to see any unquoted identifier that's not a known type as a possible protobuf type and loop, consuming .s and identifiers until it hits a non-ident/. token. (to match the proto namespace components up through the message or enum names)

To track the fully-qualified message/enum type-name add an additional field to the Type struct (tentatively) called ProtoRef so we can recover the message/enum name if canonicalizing everything.

closes: #10944

dfinkel avatar Oct 02 '24 14:10 dfinkel

@dfinkel tests failed, please check and fix them Screenshot 2024-10-03 at 4 19 57 PM

rahul2393 avatar Oct 03 '24 10:10 rahul2393

Thanks @rahul2393 for the prompt review and approval!

Sorry about the test failures! It looks like an else if block got lost while I was cleaning up the commits before sending it out for review. (then I absentmindedly ran the tests for the wrong directory :face_with_diagonal_mouth: )

The tests for this package now pass locally.

dfinkel avatar Oct 03 '24 13:10 dfinkel

Friendly ping @rahul2393

(thanks for the prompt approval)

dfinkel avatar Oct 15 '24 13:10 dfinkel

@rahul2393 if you have a few minutes, can you make another pass over this PR?

dfinkel avatar Oct 23 '24 03:10 dfinkel

Thanks @rahul2393 !

dfinkel avatar Nov 06 '24 13:11 dfinkel

I have re-used this tests for another purpose( https://github.com/cloudspannerecosystem/memefish/issues/115 ) , so it is better to contain only valid cases. Would you fix it? @dfinkel If not, I will send PR.

apstndb avatar Nov 13 '24 09:11 apstndb

Thanks @apstndb, I definitely copied those cases from the documentation. I can put together a PR to switch those to something that's actually valid tomorrow.

(I think I meant to verify that those were valid before sending this PR out but as usual other things intervened and that got lost)

dfinkel avatar Nov 13 '24 22:11 dfinkel

I just opened a PR with fixes for some bugs I ran into while using the features in this PR (including fixing the tests that @apstndb pointed out): https://github.com/googleapis/google-cloud-go/pull/11279

The most problematic bug is the fact that NOT NULL protobuf-typed columns don't get parsed correctly because I had accidentally made the parser too greedy.

Thanks in advance!

dfinkel avatar Dec 12 '24 20:12 dfinkel