feat(spanner): support for type UUID #2482
Conversation
Summary of ChangesHello @alkatrivedi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Spanner client library by adding full support for the UUID data type. It integrates UUID handling into the core codec, updates parameter binding mechanisms, and includes thorough testing to ensure reliability and correctness across different Spanner dialects. This feature allows developers to work with UUIDs natively within their applications when interacting with Spanner databases. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for the UUID data type in Spanner. The changes span across the codec for type handling, transaction logic for parameter encoding, and comprehensive additions to both unit and system tests. The approach for type inference is to let Spanner infer the type for UUID-like strings, which provides flexibility. The new tests are well-written and cover various scenarios. I've identified a couple of minor issues in the system tests where assertions for field counts were not updated, and a small inefficiency in the transaction processing logic. After addressing these points, the PR should be ready for merging.
| assert.strictEqual(metadata.rowType!.fields![2].name, 'UUID'); | ||
| assert.strictEqual(metadata.rowType!.fields![3].name, 'Float32'); | ||
| assert.strictEqual(metadata.rowType!.fields![4].name, 'Float'); | ||
| assert.strictEqual(metadata.rowType!.fields![5].name, 'Int'); | ||
| assert.strictEqual(metadata.rowType!.fields![6].name, 'Info'); | ||
| assert.strictEqual(metadata.rowType!.fields![7].name, 'Created'); | ||
| assert.strictEqual(metadata.rowType!.fields![8].name, 'DOB'); | ||
| assert.strictEqual(metadata.rowType!.fields![9].name, 'Accents'); | ||
| assert.strictEqual(metadata.rowType!.fields![10].name, 'PhoneNumbers'); | ||
| assert.strictEqual(metadata.rowType!.fields![11].name, 'HasGear'); |
There was a problem hiding this comment.
While you've correctly updated the assertions for the field names, the assertion for the total number of fields on line 4252 is now incorrect. With the addition of the UUID column, the total number of fields is 12, but the test still asserts 11. Please update that line as well:
assert.strictEqual(metadata.rowType!.fields!.length, 12);
system-test/spanner.ts
Outdated
| assert.strictEqual(metadata.rowType!.fields![2].name, 'UUID'); | ||
| assert.strictEqual(metadata.rowType!.fields![3].name, 'Float32'); | ||
| assert.strictEqual(metadata.rowType!.fields![4].name, 'Float'); | ||
| assert.strictEqual(metadata.rowType!.fields![5].name, 'Int'); | ||
| assert.strictEqual(metadata.rowType!.fields![6].name, 'Info'); | ||
| assert.strictEqual(metadata.rowType!.fields![7].name, 'Created'); | ||
| assert.strictEqual(metadata.rowType!.fields![8].name, 'HasGear'); |
There was a problem hiding this comment.
Similar to the previous comment, the assertion for the total number of fields on line 4284 is now incorrect. With the addition of the UUID column, the total number of fields is 9, but the test still asserts 8. Please update that line as well:
assert.strictEqual(metadata.rowType!.fields!.length, 9);| const typeObject = codec.createTypeObject(type); | ||
| if ( | ||
| (type.child && | ||
| typeObject.code === 'ARRAY' && | ||
| typeObject.arrayElementType?.code !== 'TYPE_CODE_UNSPECIFIED') || | ||
| (!type.child && typeObject.code !== 'TYPE_CODE_UNSPECIFIED') | ||
| ) { | ||
| paramTypes[param] = codec.createTypeObject(type); | ||
| } |
There was a problem hiding this comment.
The codec.createTypeObject(type) function is called twice here, once to create typeObject for the if condition and then again inside the if block. This is inefficient. You can reuse the typeObject that was already created.
| const typeObject = codec.createTypeObject(type); | |
| if ( | |
| (type.child && | |
| typeObject.code === 'ARRAY' && | |
| typeObject.arrayElementType?.code !== 'TYPE_CODE_UNSPECIFIED') || | |
| (!type.child && typeObject.code !== 'TYPE_CODE_UNSPECIFIED') | |
| ) { | |
| paramTypes[param] = codec.createTypeObject(type); | |
| } | |
| const typeObject = codec.createTypeObject(type); | |
| if ( | |
| (type.child && | |
| typeObject.code === 'ARRAY' && | |
| typeObject.arrayElementType?.code !== 'TYPE_CODE_UNSPECIFIED') || | |
| (!type.child && typeObject.code !== 'TYPE_CODE_UNSPECIFIED') | |
| ) { | |
| paramTypes[param] = typeObject; | |
| } |
66a939f to
2708bba
Compare
|
Warning: This pull request is touching the following templated files:
|
2c56455 to
ed0352a
Compare
ed0352a to
c74d78c
Compare
surbhigarg92
left a comment
There was a problem hiding this comment.
Approving - The changes were already reviewed as part of #2235
🤖 I have created a release *beep* *boop* --- ## [8.4.0](https://togithub.com/googleapis/nodejs-spanner/compare/v8.3.1...v8.4.0) (2026-01-09) ### Features * Add a ClientContext field to Spanner requests ([#2493](https://togithub.com/googleapis/nodejs-spanner/issues/2493)) ([37504ad](https://togithub.com/googleapis/nodejs-spanner/commit/37504adcc37a7e95acfb2530313ff783d0c1fe7d)) * Exposing total CPU related fields in AutoscalingConfig ([#2490](https://togithub.com/googleapis/nodejs-spanner/issues/2490)) ([508f0ff](https://togithub.com/googleapis/nodejs-spanner/commit/508f0ff95636b004f4200522018a199263eda8ca)) * **spanner:** Support for type UUID ([#2482](https://togithub.com/googleapis/nodejs-spanner/issues/2482)) ([0047e94](https://togithub.com/googleapis/nodejs-spanner/commit/0047e9407d86521571626c69011b70307f83f8ba)) ### Bug Fixes * **deps:** Update dependency google-gax to v5.0.6 ([#2452](https://togithub.com/googleapis/nodejs-spanner/issues/2452)) ([f9e6b86](https://togithub.com/googleapis/nodejs-spanner/commit/f9e6b86ff4da03110642c17e5ebc8fac8d903d3a)) * Flaky metric test ([#2472](https://togithub.com/googleapis/nodejs-spanner/issues/2472)) ([e169cc5](https://togithub.com/googleapis/nodejs-spanner/commit/e169cc5344d38812b1ebf20c7a987715a73d6f79)) * Memory leak and deadlock due to error event in multiplexed session ([#2477](https://togithub.com/googleapis/nodejs-spanner/issues/2477)) ([c624619](https://togithub.com/googleapis/nodejs-spanner/commit/c624619a3960892b1d2d412ff79faa5a74de45df)) * Presubmit failure for samples backups test ([#2492](https://togithub.com/googleapis/nodejs-spanner/issues/2492)) ([01eb3d5](https://togithub.com/googleapis/nodejs-spanner/commit/01eb3d5801ddb21517f185b9d585fbce4fa1475c)) * Type check for key in deleteRows ([#2486](https://togithub.com/googleapis/nodejs-spanner/issues/2486)) ([7347a16](https://togithub.com/googleapis/nodejs-spanner/commit/7347a1628ad8635b8f84b36ad1d3850b78862ac7)) * Type mismatch in Snapshot.run error handler ([#2487](https://togithub.com/googleapis/nodejs-spanner/issues/2487)) ([4ac0360](https://togithub.com/googleapis/nodejs-spanner/commit/4ac036047e3a03c073300f288c746389a38d8e42)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
This PR is to add support for type UUID.