Skip to content

feat(Spanner): Add support for Protobuf Enums#15699

Draft
efevans wants to merge 1 commit into
googleapis:mainfrom
efevans:spanner/protobuf-enums
Draft

feat(Spanner): Add support for Protobuf Enums#15699
efevans wants to merge 1 commit into
googleapis:mainfrom
efevans:spanner/protobuf-enums

Conversation

@efevans

@efevans efevans commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

b/522532048

@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Jun 22, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for Spanner Enum types by adding a ProtobufEnumName property, a constructor, and helper methods to SpannerDbType, as well as supporting conversion of protobuf values to CLR enum types. The review feedback suggests correcting the string representation of the enum type from PROTO to ENUM, handling nullable enum types correctly during conversion, and fixing a typo in a parameter name.

Comment thread apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDbType.cs Outdated
@@ -110,6 +110,11 @@ internal object ConvertToClrType(Value protobufValue, System.Type targetClrType,
return Guid.Parse(ConvertToClrTypeImpl<string>(protobufValue, options));
}

if (targetClrType.IsEnum)
{
return System.Enum.ToObject(targetClrType, ConvertToClrTypeImpl<long>(protobufValue, options));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of a string are we sure it cannot be the enum member name (e.g. "Colors.Red") rather than the enum member value that is used? docs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to add some logic in SpannerDbType.StringParsing to handle enum member names.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of a string are we sure it cannot be the enum member name (e.g. "Colors.Red") rather than the enum member value that is used?

I understood this as being a conversion method for reading the response from Spanner from a SELECT query and from the type.proto definition it should come formatted as a string containing the decimal Enums representation:

  // Encoded as `string`, in decimal format.
  ENUM = 14;

Are there other uses for this?

You will need to add some logic in SpannerDbType.StringParsing to handle enum member names.

Good callout, will address this part after a discussion about Enums FQNs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update here, in the case where the Value.KindCase is StringValue, we use System.Enum.Parse which will attempt to convert from both the enum member name like "Red" and a stringified member integer constant. This adds operability for a Protobuf enum CLR type with a String column type, where writes use the enum member name (e.g. "Red")

@@ -41,7 +41,8 @@ public void CreateKeyFromParameterCollection()
{ "", SpannerDbType.PgOid, 2 },
{ "", SpannerDbType.String, "test" },
{ "", SpannerDbType.Timestamp, new DateTime(2021, 9, 10, 9, 37, 10, DateTimeKind.Utc) },
{ "", SpannerDbType.Uuid, Guid.Parse("8f8c4746-17b1-4d9f-a634-58e11942095f") }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add some integration tests to validate against a real spanner instance

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in (mostly extended) unit and integration tests to cover string and value conversion cases

@@ -315,6 +320,10 @@ internal static SpannerDbType FromProtobufType(V1.Type type)
return new SpannerDbType(type.StructType.Fields.Select(f => new StructField(f.Name, FromProtobufType(f.Type))).ToList());
case TypeCode.Proto:
return new SpannerDbType(type.ProtoTypeFqn);
case TypeCode.Enum:
// This is handled here because equality logic prevent enum type working in s_simpleTypes,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true, Spanner will need the fully qualified name to figure out what type to cast to on it's end.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing so far I haven't had a FQN to easily grab from unless I hardcoded it. Unlike Proto messages that expose the FQN as part of the C# compiled proto, Enums do not have this and as you point out our Spanner will need the FQN if we send a parameter as type Enum.

In my testing, if the FQN isn't hard coded then there's no trivial way to get it so I've been adding parameters as Int64:

cmdInsert.Parameters.Add("class", SpannerDbType.Int64, character.CharacterClass);

Will address this more after a discussion on how we will handle Enum FQNs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add some logic here to get the clr type given the enum typecode.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the CLR type is an Enum then I think we'd have to rely on reflection to find the real Enum type, the SpannerDbType object will need the FQN which is seemingly unreliable. If we go a route where we handle cases where TypeCode is Enum differently based on whether or not we have the FQN, we could do reflection for the FQN case and defer to the default typeof(Value); otherwise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this one out for now. Protobuf message types are similarly excluded from finding their CLR type.

@robertvoinescu-work

Copy link
Copy Markdown
Contributor

Looking good, I think writing the integration tests should help clear up how spanner is actually handling the enums. You've got the logic to handle enums coming from Spanner almost there, but we will also need to support the other direction of enum types flowing to spanner from the client.

@efevans

efevans commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

@robertvoinescu-work answered most of our comments but there's some discussion still worth having. I'll be going over with Amanda the issues I'm seeing with FQNs later today and should be able to respond again with more afterwards.

@efevans efevans force-pushed the spanner/protobuf-enums branch 3 times, most recently from ba4527f to ab7bd7c Compare July 10, 2026 22:30
@efevans efevans force-pushed the spanner/protobuf-enums branch 4 times, most recently from a064f1b to 7a1eca7 Compare July 15, 2026 18:00
@efevans efevans force-pushed the spanner/protobuf-enums branch from 7a1eca7 to 783c20e Compare July 15, 2026 18:22
{
return new SpannerDbType(descriptor.FullName);
}
if (type.IsEnum && ProtobufEnumCache.GetEnumDescriptor(type) is EnumDescriptor enumDescriptor)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProtobufEnumCache.GetEnumDescriptor will return null for enums that are not Protobuf Enums, so this flow is only for Protobuf enums. Normal enums will continue working as-is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants