Support PROTO, ENUM #182

apstndb · 2024-08-17T13:03:04Z

This PR adds decoder logic for PROTO and ENUM.

spanner> WITH t AS (SELECT CAST('genre: POP, nationality: "Japan"' AS spanner.examples.music.SingerInfo) AS singer)
      -> SELECT t.singer, t.singer.genre, CAST(t.singer AS STRING) AS singer_string, CAST(t.singer.genre AS STRING) AS genre_string
      -> FROM t;
+--------------+-------+---------------------------------+--------------+
| singer       | genre | singer_string                   | genre_string |
+--------------+-------+---------------------------------+--------------+
| GgVKYXBhbiAA | 0     | nationality: "Japan" genre: POP | POP          |
+--------------+-------+---------------------------------+--------------+

Update dependency because it depends on "spanner: Add support for Proto Columns" in spanner client v1.62.0

fixes #174

apstndb · 2024-08-17T13:12:19Z

I am wondering that proto test case can be undeterministic(not canonical).

yfuruyama

Thank you for filing this PR. I left a comment about how spanner-cli should handle the proto column. Let's discuss in the comment.

yfuruyama · 2024-08-21T13:48:23Z

decoder_test.go

@@ -330,6 +372,64 @@ func TestDecodeColumn(t *testing.T) {
 }
 }

+func TestDecodeColumnProto(t *testing.T) {


Do we need these test cases?

I think spanner-cli should handle the proto column just as an arbitrary byte sequences (similar to BYTE column) and I'm not sure if we should take care of the validness of the parsed content (I feel it's a job for Spanner server test case).

Thank you. It is a fair comment.
This is a legacy of trying to put this test case into a table-driven test for TestDecodeColumn.

What we really want to test here is the following:

No error occurs

It is not mistakenly treated as NULL or an empty byte string

The input byte sequence does not change

Therefore, this test should simply test that the byte string is passed as is via spanner.GenericColumnValue.

I think it is what we need.
8752da0

I have noticed it can be written by table driven test using spanner.GenericColumnValue.

rewrited af031a4

apstndb · 2024-08-22T15:09:54Z

If you don't want to contain testdata/protos, I can rewrite ENUM test cases using hand-writtenspanner.GenericColumnValue.

yfuruyama · 2024-08-26T00:10:34Z

Sorry I don't have enough time to review this. Next ETA: end of this week.

yfuruyama · 2024-08-31T14:28:03Z

Sorry for the delayed review.

If you don't want to contain testdata/protos, I can rewrite ENUM test cases using hand-written spanner.GenericColumnValue.

This is another approach, but how about using existing enum type which implements protoreflect.Enum interface from external module? For example, descriptorpb.Edition can be used for this purpose.

This is just for testing, so I think any enum is fine.

yfuruyama · 2024-08-31T14:31:50Z

decoder_test.go

@@ -330,6 +370,56 @@ func TestDecodeColumn(t *testing.T) {
 }
 }

+// It should only contain test cases which can't be tested in TestDecodeColumn
+func TestDecodeColumnGCV(t *testing.T) {


I think we don't need to create a separate test function because spanner.NewRow accepts the spanner.GenericColumnValue as well.

ref: https://github.com/googleapis/google-cloud-go/blob/633dc86f615ee32c1ad1ec704bf6bc858a4d535c/spanner/value.go#L4424-L4428

I think that I don't want to test spanner.NewRow and (*spanner.Row).Column, so TestDecodeColumnGCV is more cleaner unit tests.
But I don't want to rewrite all existing test cases in TestDecodeColumn to equivalent hand written spanner.GenericColumnValue, so I agree to merge new test cases into TestDecodeColumn.

yfuruyama · 2024-09-01T12:49:10Z

decoder_test.go

+ {
+ desc: "null proto",
+ value: (*protos.SingerInfo)(nil),
+ want: "NULL",
+ },


Maybe we can replace this with the following test to stop dependency on the imported proto?

{ desc: "null proto", value: spanner.GenericColumnValue{ Type: &sppb.Type{ Code: sppb.TypeCode_PROTO, }, Value: structpb.NewNullValue(), }, want: "NULL", },

apstndb · 2024-09-01T15:14:26Z

This is another approach, but how about using existing enum type which implements protoreflect.Enum interface from external module? For example, descriptorpb.Edition can be used for this purpose.

All options can test we want to test.
If I use a existing enum type, I feel a enum type in well-known protobuf types like Syntax is better than other types.

apstndb · 2024-09-01T15:29:54Z

I leave ProtoTypeFqn=examples.spanner.music.SingerInfo because I think this field won't be empty in real data.

yfuruyama

LGTM. Thank you for your work on this PR! I think now the implementation is much clearer.

apstndb added 3 commits August 17, 2024 22:02

Support PROTOs

790725e

add comment

28626bf

add testdata

cec5a2f

apstndb requested a review from yfuruyama August 17, 2024 13:37

apstndb added 5 commits August 17, 2024 23:30

Fix typo

41ea767

Use non-deprecated package

c48484c

Change function order

d67206a

Use Enum() in test

b58f480

Add TestDecodeColumnProto and TestDecodeColumnProtoArray

3e1f12a

yfuruyama reviewed Aug 22, 2024

View reviewed changes

apstndb added 4 commits August 22, 2024 23:23

Rewrite TestDecodeColumnProto and TestDecodeColumnProtoArray

8752da0

Some cleanup

cbbc4eb

Tidy imports

59223ff

Replace PROTO tests to TestDecodeColumnGCV

af031a4

apstndb requested a review from yfuruyama August 22, 2024 14:54

Add comments

126ecff

yfuruyama requested changes Aug 31, 2024

View reviewed changes

yfuruyama reviewed Sep 1, 2024

View reviewed changes

apstndb added 3 commits September 2, 2024 00:23

Fix test case

0d6d978

Remove testdata/protos

d63935e

Update comments

5ae000c

apstndb requested a review from yfuruyama September 1, 2024 15:30

apstndb added 2 commits September 2, 2024 00:33

Reorder test cases

0f40b8b

Remove useless use of NullProtoEnum

eed6d0c

yfuruyama approved these changes Sep 3, 2024

View reviewed changes

yfuruyama merged commit abc1cf9 into cloudspannerecosystem:master Sep 3, 2024
2 checks passed

apstndb deleted the support-proto-enum branch September 5, 2024 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PROTO, ENUM #182

Support PROTO, ENUM #182

apstndb commented Aug 17, 2024 •

edited

Loading

apstndb commented Aug 17, 2024 •

edited

Loading

yfuruyama left a comment

yfuruyama Aug 21, 2024

apstndb Aug 22, 2024 •

edited

Loading

apstndb Aug 22, 2024

apstndb Aug 22, 2024

apstndb Aug 22, 2024

apstndb commented Aug 22, 2024

yfuruyama commented Aug 26, 2024 •

edited

Loading

yfuruyama commented Aug 31, 2024

yfuruyama Aug 31, 2024 •

edited

Loading

apstndb Sep 1, 2024 •

edited

Loading

yfuruyama Sep 1, 2024

apstndb commented Sep 1, 2024 •

edited

Loading

apstndb commented Sep 1, 2024

yfuruyama left a comment

Support PROTO, ENUM #182

Support PROTO, ENUM #182

Conversation

apstndb commented Aug 17, 2024 • edited Loading

apstndb commented Aug 17, 2024 • edited Loading

yfuruyama left a comment

Choose a reason for hiding this comment

yfuruyama Aug 21, 2024

Choose a reason for hiding this comment

apstndb Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

apstndb Aug 22, 2024

Choose a reason for hiding this comment

apstndb Aug 22, 2024

Choose a reason for hiding this comment

apstndb Aug 22, 2024

Choose a reason for hiding this comment

apstndb commented Aug 22, 2024

yfuruyama commented Aug 26, 2024 • edited Loading

yfuruyama commented Aug 31, 2024

yfuruyama Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

apstndb Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

yfuruyama Sep 1, 2024

Choose a reason for hiding this comment

apstndb commented Sep 1, 2024 • edited Loading

apstndb commented Sep 1, 2024

yfuruyama left a comment

Choose a reason for hiding this comment

apstndb commented Aug 17, 2024 •

edited

Loading

apstndb commented Aug 17, 2024 •

edited

Loading

apstndb Aug 22, 2024 •

edited

Loading

yfuruyama commented Aug 26, 2024 •

edited

Loading

yfuruyama Aug 31, 2024 •

edited

Loading

apstndb Sep 1, 2024 •

edited

Loading

apstndb commented Sep 1, 2024 •

edited

Loading