Skip to content

Conversation

@alkatrivedi
Copy link
Contributor

This PR contains support for UUID type on Cloud Spanner.

@alkatrivedi alkatrivedi requested review from a team as code owners February 8, 2025 07:40
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/nodejs-spanner API. labels Feb 8, 2025
@alkatrivedi alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Feb 8, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 8, 2025
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 8, 2025
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Feb 11, 2025
@alkatrivedi alkatrivedi force-pushed the uuid-support branch 12 times, most recently from 69cf752 to 5738698 Compare February 16, 2025 18:54
@alkatrivedi alkatrivedi added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Feb 17, 2025
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 17, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 17, 2025
@sakthivelmanii sakthivelmanii added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 17, 2025
}

if (uuid.validate(value)) {
return {type: 'unspecified'};
Copy link
Contributor

Choose a reason for hiding this comment

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

why unspecified here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cause we are not sure if the customer has inserted the UUID value in the column of type UUID/String. Hence, we are returning type as unspecified implicitly, but while making gRPC call we are making sure that if the type is unspecified we don't pass param type with the gaxOptions, this way are letting backend to figure out the type

Copy link
Contributor

Choose a reason for hiding this comment

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

@sakthivelmanii uuid value can be send for String column as well. So we want to make use of "unspecified" type and let backend decide it.
Ideally we should not have removed the type mapping when backend started suplorting untyped params, but we did not remove this because of breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

But will this work for parameterized queries. I think parameterized queries does not support UNSPECIFIED type.
@alkatrivedi can you check if parameterized query is working when passing a UUID value?

Copy link
Contributor

Choose a reason for hiding this comment

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

The case that we are talking about here is when customer triggers a following parameterized query without an explicit param type
const query = {
sql: 'SELECT @v',
params: {
v: values,
},
};
I think this fails if value is of UUID type and param type is not mentioned because client now sends a UNSPECIFIED type to backend

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion here is
Ideally, if param type is not mentioned then treat the value as string.
If customer wants to treat that value as UUID then they can explicitly specify the param type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

based on the discussion with @harshachinta and @surbhigarg92

we will return the type as unspecified, allowing the backend to determine the type of the value provided

regarding the query - 'SELECT @v'

failure in this case, where the parameter type is not inferred, is expected behavior. since the query does not target a specific table, deriving type dynamically is not a valid scenario

Choose a reason for hiding this comment

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

Flash forward one year later, when this has been merged: it breaks usage of implicitly-string parameters containing a UUID, at least with the emulator.

They are detected as UUID by this line of code, and this results in a type-less struct type being passed to the backend, which it doesn't like. For example, the parameter user contains a UUID (the same query worked fine before UUID "support" was introduced):

Error: 3 INVALID_ARGUMENT: Type code must be specified, found ; 
    When parsing field #0 of code: STRUCT
    struct_type {
      fields {
        name: \"user\"
        type {
        }
      }
    }

My suggestion here is
Ideally, if param type is not mentioned then treat the value as string.
If customer wants to treat that value as UUID then they can explicitly specify the param type.

This seemed like a much more reasonable way of introducing a new feature and preserving backward compatibility.

I'll open an issue when I have time, but I'd highly appreciate proactive effort from the team to fix this, as the issue had been identified during the review process and was ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flovouin Thanks for flagging this! I will take a look

}

if (uuid.validate(value)) {
return {type: 'unspecified'};
Copy link
Contributor

Choose a reason for hiding this comment

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

@sakthivelmanii uuid value can be send for String column as well. So we want to make use of "unspecified" type and let backend decide it.
Ideally we should not have removed the type mapping when backend started suplorting untyped params, but we did not remove this because of breaking changes.

surbhigarg92
surbhigarg92 previously approved these changes Feb 25, 2025
@surbhigarg92 surbhigarg92 added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Feb 25, 2025
@alkatrivedi alkatrivedi force-pushed the uuid-support branch 2 times, most recently from 10f3bc6 to b686046 Compare December 10, 2025 08:39
@alkatrivedi
Copy link
Contributor Author

closing this PR as I have raised a new PR

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 googleapis/nodejs-spanner API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. size: l Pull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants