Skip to content

feat(spanner): support for type UUID #2235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat(spanner): support for type UUID #2235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
af60f7c
feat(spanner): support for type UUID
alkatrivedi Feb 8, 2025
0824f39
Merge branch 'main' into uuid-support
alkatrivedi Feb 8, 2025
93b8fcb
Merge branch 'main' into uuid-support
alkatrivedi Feb 10, 2025
5738698
feat(spanner): support for type UUID
alkatrivedi Feb 8, 2025
fd19904
Merge branch 'main' into uuid-support
alkatrivedi Feb 17, 2025
4e6be00
Merge branch 'main' into uuid-support
alkatrivedi Feb 24, 2025
65f76b3
refactor
alkatrivedi Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/codec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import * as is from 'is';
import {common as p} from 'protobufjs';
import {google as spannerClient} from '../protos/protos';
import {GoogleError} from 'google-gax';
import * as uuid from 'uuid';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type Value = any;
Expand Down Expand Up @@ -697,6 +698,7 @@ const TypeCode: {
bool: 'BOOL',
int64: 'INT64',
pgOid: 'INT64',
uuid: 'UUID',
float32: 'FLOAT32',
float64: 'FLOAT64',
numeric: 'NUMERIC',
Expand Down Expand Up @@ -737,6 +739,7 @@ interface FieldType extends Type {
/**
* @typedef {object} ParamType
* @property {string} type The param type. Must be one of the following:
* - uuid
* - float32
* - float64
* - int64
Expand Down Expand Up @@ -814,6 +817,10 @@ function getType(value: Value): Type {
return {type: 'bool'};
}

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

}

if (is.string(value)) {
return {type: 'string'};
}
Expand Down
10 changes: 9 additions & 1 deletion src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1521,7 +1521,15 @@ export class Snapshot extends EventEmitter {
if (!is.empty(typeMap)) {
Object.keys(typeMap).forEach(param => {
const type = typeMap[param];
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] = codec.createTypeObject(type);
}
});
}

Expand Down
Loading
Loading