We are going to support only proto2 for Verily Private GA. Not sure if there is any implication to use proto3.Value here instead of proto2.Value. Can you verify?

I guess proto3 here means the Spanner proto3 api definition not the protobuf proto3.

What happens if y is not a valid enum value? Is the behavior expected.

Please add a comment about the behavior.

At client library end, as we do not have the schema of the enum it is difficult to validate the valid range of enum value.
I guess validation should be done from the backend side while inserting enum. If the enum is validated during insertion, then we can assume the value y is always valid.
However, if y is not a valid enum value, then there will not be any exception in above code. The integer value gets assigned to the enum and when user tries to print the enum it shows an integer.
For ex: Consider the following schema of Enum:

enum Genre {
  POP = 0;
  JAZZ = 1;
  FOLK = 2;
  ROCK = 3;
}

Here valid range is 0 to 3.
If y is 2, then fmt.Println(p) prints FOLK.
If y is 6 which is invalid, then fmt.Println(p) prints 6

SG if that's the behavior for go.

If v is nil, it seems protoEnumfqn is not set. Is this expected?

Do we have a test for this? If so, which line. Thanks!

If v is nil, then protoEnumfqn by default gets assigned with default value of string which is "".

Is it expected? We need a test for this.

Can we get the descriptor even if the value is nil as the descriptor just depends on the type?

The only way we can pass nil here is to directly pass nil while inserting data. We cannot create a nil instance of an enum (pb.Genre_Rock or pb.Genre) or proto (pb.Book{}) as they create default objects in every possibility of declaration. So the only possibility is to pass nil and we cannot get descriptor fn for nil. So assigning "" in that case.

Also from above, if nil is passed it will be handled by client library as a NULL_VALUE at the beginning of switch case and it will not come inside the protoreflect.Enum case. This means protoEnumfqn will not be set to "" as there is no way to send a nil instance of an enum or proto.

The only way to pass a nil type of proto enum is by type casting and passing like (*pb.Genre)(nil). As it is a nil type, calling v.Descriptor() raises a panic in golang with exception as follows:

panic: value method Genre.Descriptor called using nil *Genre pointer

So, the only option is to set protoEnumfqn as "".
I have added a unit test for the same Test

So a nil value of type pb.Genre is possible.

In golang, is it possible to do something like:
v := pb.Genre_ROCK
vp := &v
vp = nil
If so, what is the type of v now?

Is it reasonable that a user passing a nil value of type pb.Genre would expect Spanner to know the right type? If that's the case and we cannot do it, maybe we should give them an error instead of silently doing something unexpected. Reasonable?

And is ENUM with empty fqn a good candidate and why?

If we expect users to use nullProto instead of a pb.Genre nil value, we should point that out in the error msg.

It seems that we can get descriptor without a concrete value with:
pb.Genre.Descriptor()

I will rely on you to help to see if a nil enum value would give us the descriptor.

The following code will give the same type (*pb.Genre)(nil) as pointed above

v := pb.Genre_ROCK
vp := &v
vp = nil

golang throws an exception when any method is called using nil pointer. The only reasonable option would be to throw an error to user indicating that nil types cannot be used.
I have added the code changes @Shua1. Can you please have a look?

Same as above.

If v is nil, then protoMessagefqn by default gets assigned with default value of string which is "".

Added test.
As per protoreflect documentation, (*pb.SingerInfo)(nil) is an invalid message type. I verified it by using proto.MessageReflect(v).isValid() and it returns false for (*pb.SingerInfo)(nil) .So setting protoMessagefqn as "" is valid.

Text from Protoreflect document:
An invalid message often corresponds to a nil pointer of the concrete message type

This is fine for now. But we need to keep in mind that some user might want to decode a INT64 value into enum.

Maybe, we should make it happen for now. To be consistent with

1. the write path, where integer can be written into ENUM column.
2. the proto and bytes compatible behavior.

What do you think?

@Shua1
I have added the code changes and integration tests to validate the following compatibility behaviours between Int64 and Enum. Can you please verify?

1. Write Enum into Int64 column
2. Write Int64 into Enum column
3. Read Int64 column value using Enum
4. Read Enum column value using Int64

Link to tests: Compatibility Behaviour Integration tests

It looks good.

To make the case completely solid, could you please also verify this is the expected behavior internally on your side?

Yes @Shua1. This behaviour is valid on our side also.

Different from the ENUM case, we do need to support decoding bytes typed result into PROTO. BYTES values and PROTO values are compatible with each other.

Please also add tests for both direction.

@Shua1
Supporting compatibility between BYTES and PROTO values involves code changes in the decoding function of BYTES also. This needs a bit more testing and handling corner cases.
I will make a note of this and will support this case in my subsequent PR. Does that sound good?

@Shua1
I have added the code changes and integration tests to validate the following compatibility behaviours between Bytes and Proto.

1. Write Bytes into Proto column
2. Write Proto into Bytes column
3. Read Proto column value using Bytes
4. Read Bytes column value using Proto

Link to tests: Compatibility Behaviour Integration tests

Somehow, I must have lost the comment I left here previously.

Consider making this consistent with the protoMessageProto method below.

That being said, it does feel strange that in the API TypeCode_PROTO means proto message. But protoMessage does look more accurate. What do you think?

I have followed the convention used in API. As it was TypeCode_PROTO, I have named the methods as protoType(). However, using protoMessageType() looks more meaningful.
I have added the necessary changes @Shua1

I guess proto3 here means the Spanner proto3 api definition not the protobuf proto3.

It seems that we are mixing usage of pb.Genre_ROCK and singerProtoEnum. What's the rationale for it? Is it easier to debug if we use Genre_ROCK in all places?

This is to differentiate the data passed in val and want. pb.Genre_ROCK is passed in val, and singerProtoEnum is passed in want in all the tests. Also in some places (in future PR changes) we need to pass a pointer like &singerProtoEnum instead of just the constant in want. So to maintain consistency across all the tests I have used singerProtoEnum in want.

Also want here shows how the user passes a variable to read the data. This helps to debug from user's point of view.

Same as above.

Modified

SG if that's the behavior for go.

It looks good.

To make the case completely solid, could you please also verify this is the expected behavior internally on your side?

Is it expected? We need a test for this.

Can we get the descriptor even if the value is nil as the descriptor just depends on the type?