feat(spanner-driver): integrate native spannerlib-node wrapper and implement type system by surbhigarg92 · Pull Request #9141 · googleapis/google-cloud-node

surbhigarg92 · 2026-08-13T06:35:57Z

No description provided.

gemini-code-assist

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.

surbhigarg92 · 2026-08-14T05:32:09Z

/gemini review

gemini-code-assist

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.

…plement type system - Integrate Client and Pool directly with native CGO/N-API bindings via spannerlib-node.

olavloite · 2026-08-14T12:22:34Z

+}
+
+// -----------------------------------------------------------------------------
+// Binary Format Parsers (matches pg-types/lib/binaryParsers.js)


Technically, these will never be used in the Spanner driver. So if we keep them, that is only for code compatibility with node-pg. But I don't think any application code would ever use this, as these functions return the low-level PG wire-protocol binary representation of a value. That is not something I would expect an application to use. We can keep them here if that is better for compatibility, but we should preferably try to make sure that no-one tries to use them with the driver, as that would fail.

olavloite · 2026-08-14T12:23:36Z

+
+const defaultTypeParsers: Record<string, Record<number, TypeParser>> = {
+  text: textParsers,
+  binary: binaryParsers,


I think that it would be better to let this just default to the textParsers. Anyone who tries to use this with this driver, will run into problems.

olavloite · 2026-08-14T12:30:58Z

+   * @returns EncodedParam containing `valueProto` and `typeProto`.
+   */
+  static encodeValue(val: unknown): EncodedParam {
+    if (val === null || val === undefined) {


This should handle .toPostgres() first, as otherwise an array of .toPostgres() instances will be treated as an object:

// 1. Unwrap custom objects implementing .toPostgres() first (handles both top-level and nested array items) if ( typeof val === 'object' && val !== null && typeof (val as {toPostgres?: unknown}).toPostgres === 'function' ) { return Codec.encodeValue( (val as {toPostgres: () => unknown}).toPostgres(), ); }

Test case:

it('should unwrap custom objects with .toPostgres() inside array parameters', () => { // Custom ORM / domain model wrappers const customId1 = { toPostgres: () => 101 }; const customId2 = { toPostgres: () => 102 }; // Pass array of custom objects to parameter $1 const { fields } = Codec.encodeParams([[customId1, customId2]], 'pg'); assert.deepStrictEqual(fields.p1, { listValue: { values: [ { stringValue: '101' }, { stringValue: '102' }, ], }, }); });

olavloite · 2026-08-14T13:34:49Z

+      const pool = await Pool.create(this.dsn);
+      this.nativePool = pool;


We should not create a new Pool for each new Client. A Pool creates an actual connection to Spanner (e.g. a gRPC channel pool), and each pool can create many Connection instances. A Connection instance is lightweight in SpannerLib. Pool is not a lightweight object.

Compare to the implementation of the .NET driver here: https://github.com/googleapis/dotnet-spanner-entity-framework/blob/e4dd7124a6b60c70f0f38db3f7507d2bb087d629/spanner-ado-net/spanner-ado-net/SpannerConnection.cs#L371

Each Pool is shared across every connection that uses the same connection string (DSN). So we should introduce something similar here, where clients that are created like this create or pick a pool based on the DSN.

We can do this in a follow-up PR, but we should put a TODO here in that case.

olavloite · 2026-08-14T14:12:03Z

   * @param callback - Optional Node callback function receiving `(err, result)`.
   * @returns Executable `Query` instance implementing Thenable interface and EventEmitter.
   */
  public query<R = Record<string, unknown>>(


Can we split this function into smaller parts? It is getting very long and hard to read.

olavloite · 2026-08-14T14:35:50Z

+    if (v.numberValue !== undefined && v.numberValue !== null) {
+      return String(v.numberValue);
+    }
+    if (v.boolValue !== undefined && v.boolValue !== null) {
+      return v.boolValue ? 't' : 'f';
+    }


This seems to 'emulate' the PostgreSQL wire-protocol. I don't think we need that (and it makes it less efficient). Now a boolean is converted from bool => string => bool. A number is converted from number => string => number.

olavloite · 2026-08-14T14:36:12Z

+      return v.boolValue ? 't' : 'f';
+    }
+    if (v.structValue) {
+      return JSON.stringify(v.structValue);


This will return something like this:

{ "fields": { "user_id": { "stringValue": "100" }, "score": { "numberValue": 98.5 } } }

Is that really what we would want here?

olavloite · 2026-08-14T14:41:37Z

-// Streaming row events
+await client.connect();
+
+// Stream rows as they arrive from Spanner gRPC stream


This comment is a bit misleading. We are indeed streaming the rows as they come from the Spanner gRPC stream. But the driver is also collecting all of them in memory, so if the query returns a large number of rows, then you will get an OOM.

olavloite · 2026-08-14T14:42:59Z

+        : elementParser(item);
+    });
+  }
+  if (typeof source === 'string') {


Spanner will never return this, so in that sense, we can remove everything from here in this function.

olavloite · 2026-08-14T14:51:47Z

+
+      // Seed AllTypes table
+      await client.query('DELETE FROM AllTypes WHERE Id = 1');
+      await client.query(


Can we also add rows here with:

NULL values (to verify that NULL parameter values work correctly)

Empty arrays (to verify that an empty array is correctly sent to Spanner)

Arrays with NULL elements in the array. So for example [1, NULL, 2]

Can we also add a test for using .toPostgres()

gemini-code-assist Bot reviewed Aug 13, 2026

View reviewed changes

surbhigarg92 force-pushed the spanner-driver-native-bridge branch 6 times, most recently from 7a2a255 to 5b37e00 Compare August 13, 2026 15:57

surbhigarg92 changed the title ~~Spanner driver native bridge~~ feat(spanner-driver): integrate native spannerlib-node wrapper and implement type system Aug 13, 2026

surbhigarg92 force-pushed the spanner-driver-native-bridge branch from 5b37e00 to ae110fb Compare August 13, 2026 17:19

gemini-code-assist Bot reviewed Aug 14, 2026

View reviewed changes

surbhigarg92 force-pushed the spanner-driver-native-bridge branch 2 times, most recently from e117502 to 7ffb333 Compare August 14, 2026 05:48

surbhigarg92 marked this pull request as ready for review August 14, 2026 06:07

surbhigarg92 requested a review from a team as a code owner August 14, 2026 06:07

surbhigarg92 force-pushed the spanner-driver-native-bridge branch 5 times, most recently from edf2661 to 7cb6ac0 Compare August 14, 2026 10:29

feat(spanner-driver): integrate native spannerlib-node wrapper and im…

1582dc1

…plement type system - Integrate Client and Pool directly with native CGO/N-API bindings via spannerlib-node.

surbhigarg92 force-pushed the spanner-driver-native-bridge branch from 7cb6ac0 to 1582dc1 Compare August 14, 2026 10:41

olavloite reviewed Aug 14, 2026

View reviewed changes

		const pool = await Pool.create(this.dsn);
		this.nativePool = pool;

Uh oh!

Conversation

surbhigarg92 commented Aug 13, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

surbhigarg92 commented Aug 14, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants