feat(spanner-driver): integrate native spannerlib-node wrapper and implement type system - #9141
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates the native CGO bridge (spannerlib-node) into the Spanner driver, enabling query execution, connection pooling, and a custom type parser system (TypeOverrides and Codec) compatible with node-postgres. Feedback focuses on preventing resource leaks by wrapping result set processing and connection creation in appropriate try...finally and try...catch blocks. Additionally, it is recommended to relax the numeric OID checks in TypeOverrides to support GoogleSQL string descriptors and to improve array element type inference when the first element is null.
7a2a255 to
5b37e00
Compare
5b37e00 to
ae110fb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates the native Go CGO bridge (spannerlib-node) into the Spanner Node.js driver, enabling native connection management, query execution, parameter serialization, and custom type parsing. The reviewer provided valuable feedback pointing out several critical issues, including a potential native resource leak during concurrent client teardown, a crash risk with invalid Date parameters, and a TypeError that breaks GoogleSQL queries. Additionally, the reviewer highlighted architectural concerns regarding nested native pools and memory buffering during streaming, a naive array parser that fails on commas, a regression in connect() compatibility, and a packaging issue with local dependency paths.
e117502 to
7ffb333
Compare
7cb6ac0 to
1582dc1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates the native CGO bridge (spannerlib-node) into the Cloud Spanner Node.js driver, adding transaction support, connection pooling, a custom type system, and parameter serialization. It also introduces a Codec utility for protobuf wire format mapping, updates the documentation, and adds comprehensive unit and system tests. The review feedback highlights several critical issues: a potential crash in TypeOverrides.getTypeParser when handling non-numeric OIDs, silent query failures when nativeConnection is missing, unhandled errors in the finally block of _executeOnNativeConnection, the use of dynamic require() paths in commented-out code, and a relative file: path in optionalDependencies that will break external installations.
7dc6e9f to
36e43a5
Compare
…plement type system - Integrate Client and Pool directly with native CGO/N-API bindings via spannerlib-node.
ec38890 to
5f146fb
Compare
5f146fb to
a8ad609
Compare
a8ad609 to
728139f
Compare
| if (Number.isInteger(val)) { | ||
| return { | ||
| valueProto: {stringValue: String(val)}, | ||
| typeProto: {code: TypeCode.INT64}, | ||
| }; | ||
| } |
There was a problem hiding this comment.
I worry about how this will work in real life. If an application tries to insert the value 1.0 into a FLOAT64 column, what will happen then? Do we have an end-to-end test for that?
| return { | ||
| valueProto: {numberValue: val}, | ||
| typeProto: {code: TypeCode.FLOAT64}, | ||
| }; |
There was a problem hiding this comment.
This is also risky. What if the application is trying to insert the value 1.2 into a float32 column. Does Spanner accept this?
Also, we probably need to manually check if the value is NaN, Inf or -Inf. Those values should also be sent as strings to Spanner.
There was a problem hiding this comment.
Actually, my question about float32 can probably be ignored. I see now that all data types are removed before the query parameters are sent to Spanner, so the float64 vs float32 problem should not be there.
This PR integrates the native
spannerlib-nodewrapper into@google-cloud/spanner-driverand implements the complete PostgreSQL-compatible data type codec and parsing system following standardnode-postgres(pg) conventions. It enables connecting to Cloud Spanner PostgreSQL-dialect databases, executing parameterized queries, decoding all Cloud Spanner scalar and array data types, streaming rows/events, managing transactions (BEGIN, COMMIT, ROLLBACK), and pooling connections.Key Changes
1. Native Bridge Integration (spannerlib-node)
Connection & Pool Bridge: Integrated Connection, Pool, and Rows from
spannerlib-nodeinsrc/lib/native.tsExecution & Teardown Lifecycle: Implemented connection handshake, sequential query task queueing, and safe resource closure in Client and Pool.
2. Comprehensive Type System & Codecs
mapSpannerTypeToPgOidinsrc/lib/pg/types.ts, mapping all Spanner TypeCodes and TypeAnnotationCodes to standard numeric PostgreSQL OIDs.src/lib/pg/utilities.ts