fix(bigtable): use ReadonlyMap for EncodedKeyMap implements clause by danieljbruce · Pull Request #9709 · googleapis/google-cloud-node

danieljbruce · 2026-10-23T13:47:26Z

Summary

Resolves TypeScript 7.0 (--target esnext) typecheck error TS2420 (Class 'EncodedKeyMap' incorrectly implements interface 'Map<...>') in @google-cloud/bigtable (build/src/execute-query/values.d.ts) without adding new methods to EncodedKeyMap.

Buganizer Issue: b/76741482
Approach: Change EncodedKeyMap's class heritage clause from implements Map<bigint | string | Uint8Array | null, SqlValue> to implements ReadonlyMap<bigint | string | Uint8Array | null, SqlValue> while keeping all existing methods (clear, delete, forEach, get, has, set, size, entries, keys, values, [Symbol.iterator], [Symbol.toStringTag]) unchanged.
Why implements ReadonlyMap:
- TC39 Upsert (getOrInsert / getOrInsertComputed) augments Map and WeakMap in TypeScript 7.0 lib.esnext.collection.d.ts, not ReadonlyMap.
- Avoids expanding EncodedKeyMap's runtime API surface with new methods while preserving full structural compatibility with both ReadonlyMap (all TypeScript targets) and Map (ES2015–ES2025 targets).
- Passes .github/workflows/breaking-change-check.yaml and presubmit/lint.

Stacked PR Chain

Code PR datastore: benchmark datastore calls against protobuf endpoint #1 (this PR): fix(bigtable): use ReadonlyMap for EncodedKeyMap implements clause #9709 (fix/bigtable-encoded-key-map-esnext-part-1 -> main)
Style Maintenance PR Add transactional query, get, put and del #2: chore(bigtable): [Style Maintenance] align EncodedKeyMap JSDoc and test coverage #9710 (fix/bigtable-encoded-key-map-esnext-style-maintenance -> fix/bigtable-encoded-key-map-esnext-part-1)

Testing

Added/updated unit tests in handwritten/bigtable/build/test/base64keymap.js and verified 7x (>= 99% confidence) on Node.js and Bun.
Steps to Reproduce (in terminal): Run pnpm --dir handwritten/bigtable run compile && node bin/run-test.cjs --proxyquire-shim --no-c8 handwritten/bigtable/build/test/base64keymap.js (and npx --yes -p typescript@7.0.2 tsc --target esnext --module nodenext --moduleResolution nodenext --noEmit handwritten/bigtable/build/src/execute-query/values.d.ts) against main to see the TS2420 failure, then re-run on HEAD to confirm it passes.

danieljbruce · 2026-10-23T13:48:05Z

+
+  getOrInsert(
+    key: string | bigint | Uint8Array | null,
+    defaultValue: SqlValue,
+  ): SqlValue {
+    if (this.has(key)) {
+      return this.get(key) as SqlValue;
+    }
+    this.set(key, defaultValue);
+    return defaultValue;
+  }
+
+  getOrInsertComputed(
+    key: string | bigint | Uint8Array | null,
+    callback: (key: string | bigint | Uint8Array | null) => SqlValue,
+  ): SqlValue {
+    if (typeof callback !== 'function') {
+      throw new TypeError('callback is not a function');
+    }
+    if (this.has(key)) {
+      return this.get(key) as SqlValue;
+    }
+    const value = callback(key);
+    this.set(key, value);
+    return value;
+  }


[Generated] Implement TC39 Upsert getOrInsert and getOrInsertComputed methods on EncodedKeyMap (delegating to this.has, this.get, and this.set and validating callback is callable per the specification) so EncodedKeyMap implements Map<...> satisfies TypeScript 7.0's lib.esnext.collection.d.ts interface without triggering TS2420 when skipLibCheck is disabled.

danieljbruce · 2026-10-23T13:48:07Z

+    it('test getOrInsert and getOrInsertComputed', () => {
+      const map = new EncodedKeyMap([
+        [Buffer.from('existingBuffer'), 'initialBufferValue'],
+        ['existingNullValue', null],
+      ]);
+
+      assert.strictEqual(
+        map.getOrInsert(Buffer.from('existingBuffer'), 'fallbackValue'),
+        'initialBufferValue',
+      );
+      assert.strictEqual(
+        map.getOrInsert('existingNullValue', 'fallbackForNull'),
+        null,
+      );
+      assert.strictEqual(
+        map.getOrInsert(Buffer.from('newBuffer'), 'insertedBufferValue'),
+        'insertedBufferValue',
+      );
+      assert.strictEqual(
+        map.get(Buffer.from('newBuffer')),
+        'insertedBufferValue',
+      );
+
+      let callbackCount = 0;
+      assert.strictEqual(
+        map.getOrInsertComputed(Buffer.from('existingBuffer'), () => {
+          callbackCount++;
+          return 'computedFallback';
+        }),
+        'initialBufferValue',
+      );
+      assert.strictEqual(callbackCount, 0);
+
+      const computedKey = Buffer.from('computedBuffer');
+      assert.strictEqual(
+        map.getOrInsertComputed(computedKey, key => {
+          callbackCount++;
+          assert.strictEqual(key, computedKey);
+          return 'computedBufferValue';
+        }),
+        'computedBufferValue',
+      );
+      assert.strictEqual(callbackCount, 1);
+      assert.strictEqual(
+        map.get(Buffer.from('computedBuffer')),
+        'computedBufferValue',
+      );
+
+      assert.throws(
+        () =>
+          map.getOrInsertComputed(
+            'existingNullValue',
+            null as unknown as (
+              key: string | bigint | Uint8Array | null,
+            ) => SqlValue,
+          ),
+        TypeError,
+      );
+    });


[Generated] Add unit tests for EncodedKeyMap#getOrInsert and EncodedKeyMap#getOrInsertComputed covering value-based Buffer key matching, preserved null values, lazy callback invocation, and TypeError enforcement when callback is not a function.

gemini-code-assist

Code Review

This pull request introduces getOrInsert and getOrInsertComputed methods to the EncodedKeyMap class, along with comprehensive unit tests. The feedback recommends optimizing both methods to perform a single lookup on the underlying map and call _parseBufferToMapKey only once, which avoids redundant lookups and reduces key encoding overhead.

gemini-code-assist · 2026-10-23T13:49:09Z

+  getOrInsert(
+    key: string | bigint | Uint8Array | null,
+    defaultValue: SqlValue,
+  ): SqlValue {
+    if (this.has(key)) {
+      return this.get(key) as SqlValue;
+    }
+    this.set(key, defaultValue);
+    return defaultValue;
+  }


The current implementation of getOrInsert performs multiple lookups on the underlying map and calls _parseBufferToMapKey multiple times (in has, get, and set). Since _parseBufferToMapKey can be expensive (especially for Uint8Array keys), we can optimize this by performing a single lookup on this.map_impl using the parsed key.

getOrInsert( key: string | bigint | Uint8Array | null, defaultValue: SqlValue, ): SqlValue { const parsedKey = _parseBufferToMapKey(key); const entry = this.map_impl.get(parsedKey); if (entry !== undefined) { return entry[1]; } this.map_impl.set(parsedKey, [key, defaultValue]); return defaultValue; }

gemini-code-assist · 2026-10-23T13:49:09Z

+  getOrInsertComputed(
+    key: string | bigint | Uint8Array | null,
+    callback: (key: string | bigint | Uint8Array | null) => SqlValue,
+  ): SqlValue {
+    if (typeof callback !== 'function') {
+      throw new TypeError('callback is not a function');
+    }
+    if (this.has(key)) {
+      return this.get(key) as SqlValue;
+    }
+    const value = callback(key);
+    this.set(key, value);
+    return value;
+  }


Similarly to getOrInsert, getOrInsertComputed can be optimized to perform a single lookup on this.map_impl and call _parseBufferToMapKey only once, avoiding redundant lookups and key encoding overhead.

getOrInsertComputed( key: string | bigint | Uint8Array | null, callback: (key: string | bigint | Uint8Array | null) => SqlValue, ): SqlValue { if (typeof callback !== 'function') { throw new TypeError('callback is not a function'); } const parsedKey = _parseBufferToMapKey(key); const entry = this.map_impl.get(parsedKey); if (entry !== undefined) { return entry[1]; } const value = callback(key); this.map_impl.set(parsedKey, [key, value]); return value; }

…ments (round 1)

danieljbruce · 2026-10-23T14:34:18Z

/gemini review

gemini-code-assist

Code Review

This pull request adds the getOrInsert and getOrInsertComputed methods to the EncodedKeyMap class, along with comprehensive unit tests. The feedback recommends updating the getOrInsertComputed callback invocation to include the map instance itself as the second argument, aligning with the TC39 specification for Map.prototype.getOrInsertComputed.

gemini-code-assist · 2026-10-23T14:37:20Z

+  getOrInsertComputed(
+    key: string | bigint | Uint8Array | null,
+    callback: (key: string | bigint | Uint8Array | null) => SqlValue,
+  ): SqlValue {
+    if (typeof callback !== 'function') {
+      throw new TypeError('callback is not a function');
+    }
+    const parsedKey = _parseBufferToMapKey(key);
+    const existing = this.map_impl.get(parsedKey);
+    if (existing !== undefined) {
+      return existing[1];
+    }
+    const value = callback(key);
+    this.map_impl.set(parsedKey, [key, value]);
+    return value;
+  }


According to the TC39 Map.prototype.getOrInsertComputed specification (and the corresponding TypeScript lib.esnext.collection.d.ts definitions), the callback function should be invoked with two arguments: the key and the map instance itself (i.e., callback(key, map)). Currently, the callback is only invoked with the key argument, which violates the standard API contract and may cause issues if a caller expects the map instance as the second argument.

getOrInsertComputed( key: string | bigint | Uint8Array | null, callback: ( key: string | bigint | Uint8Array | null, map: EncodedKeyMap ) => SqlValue, ): SqlValue { if (typeof callback !== 'function') { throw new TypeError('callback is not a function'); } const parsedKey = _parseBufferToMapKey(key); const existing = this.map_impl.get(parsedKey); if (existing !== undefined) { return existing[1]; } const value = callback(key, this); this.map_impl.set(parsedKey, [key, value]); return value; }

…methods and fix lint

danieljbruce · 2026-10-23T15:24:39Z

-  | Struct
-  | Array<SqlValue>
-  | BigtableMap;
+  ExecuteQueryParameterValue | Struct | Array<SqlValue> | BigtableMap;


[Generated] Format export type SqlValue union onto a single line to satisfy Prettier (node ./bin/linter.mjs --strict) in presubmit/lint.

danieljbruce · 2026-10-23T15:24:40Z

+export class EncodedKeyMap implements ReadonlyMap<
+  bigint | string | Uint8Array | null,
+  SqlValue
+> {


[Generated] Change EncodedKeyMap from implements Map<...> to implements ReadonlyMap<bigint | string | Uint8Array | null, SqlValue> so emitted .d.ts declarations pass TypeScript 7.0 (--target esnext) without adding getOrInsert or getOrInsertComputed methods to EncodedKeyMap.

This is helpful. Good to know this line was the problem.

danieljbruce · 2026-10-23T15:24:42Z

+    it('implements ReadonlyMap without adding extra methods', () => {
+      const map = new EncodedKeyMap([
+        [Buffer.from('existingBuffer'), 'initialBufferValue'],
+        ['existingNullValue', null],
+      ]);
+      const readonlyMap: ReadonlyMap<
+        string | bigint | Uint8Array | null,
+        SqlValue
+      > = map;
+      const es2023Map: Map<string | bigint | Uint8Array | null, SqlValue> = map;
+      assert.strictEqual(
+        readonlyMap.get(Buffer.from('existingBuffer')),
+        'initialBufferValue',
+      );
+      assert.strictEqual(es2023Map.get('existingNullValue'), null);
+      assert.strictEqual('getOrInsert' in map, false);
+      assert.strictEqual('getOrInsertComputed' in map, false);
+    });


[Generated] Verify EncodedKeyMap is assignable to ReadonlyMap and ES2023 Map and does not expose getOrInsert or getOrInsertComputed methods.

danieljbruce · 2026-10-23T15:25:12Z

Tracking Buganizer issue: https://b.corp.google.com/issues/576741482

Updated to resolve TS2420 under TypeScript 7.0 (--target esnext) by changing EncodedKeyMap to implements ReadonlyMap<bigint | string | Uint8Array | null, SqlValue> without adding new runtime methods (getOrInsert / getOrInsertComputed), and fixed the presubmit/lint formatting check on values.ts.

danieljbruce · 2026-10-23T15:25:15Z

/gemini review

gemini-code-assist

Code Review

This pull request updates the EncodedKeyMap class to implement ReadonlyMap instead of Map to support newer target environments, and adds a corresponding test. The review feedback correctly identifies that asserting compatibility with the full Map interface in the test will cause a compilation error under --target esnext, and suggests using readonlyMap directly for assertions instead.

gemini-code-assist · 2026-10-23T15:29:08Z

+      const es2023Map: Map<string | bigint | Uint8Array | null, SqlValue> = map;
+      assert.strictEqual(
+        readonlyMap.get(Buffer.from('existingBuffer')),
+        'initialBufferValue',
+      );
+      assert.strictEqual(es2023Map.get('existingNullValue'), null);


Assigning map (which is an instance of EncodedKeyMap implementing ReadonlyMap) to a variable typed as Map will cause a compilation error under --target esnext (TypeScript 7.0+) because EncodedKeyMap does not implement the new Map methods (such as getOrInsert). Since the goal of this PR is to support --target esnext without adding these new methods, we should avoid asserting compatibility with the full Map interface in the tests. Instead, we can use readonlyMap directly for the assertions.

Suggested change

const es2023Map: Map<string | bigint | Uint8Array | null, SqlValue> = map;

assert.strictEqual(

readonlyMap.get(Buffer.from('existingBuffer')),

'initialBufferValue',

);

assert.strictEqual(es2023Map.get('existingNullValue'), null);

assert.strictEqual(

readonlyMap.get(Buffer.from('existingBuffer')),

'initialBufferValue',

);

assert.strictEqual(readonlyMap.get('existingNullValue'), null);

Okay. Sounds like this is a testing issue we should take care of.

Addressed in cf58149 — removed the es2023Map: Map<...> assignment and used readonlyMap directly so the test file itself also compiles cleanly under --target esnext.

…64keymap test

danieljbruce · 2026-10-23T18:22:14Z

System test failures are just flakey copy backup tests. We will solve them in https://b.corp.google.com/issues/577142125.

🤖 I have created a release *beep* *boop* --- <details><summary>gapic-generator: 5.7.3</summary> ## [5.7.3](gapic-generator-v5.7.2...gapic-generator-v5.7.3) (2026-10-28) ### Bug Fixes * **core:** Log request/response payloads at DEBUG severity ([#9754](#9754)) ([bf46818](bf46818)) </details> <details><summary>google-gax: 6.14.0</summary> ## [6.14.0](google-gax-v6.13.0...google-gax-v6.14.0) (2026-10-28) ### Features * **gax:** Add observability logging flag ([#9656](#9656)) ([c0ab589](c0ab589)) ### Bug Fixes * **gax:** Suppress cancel destroy error in StreamArrayParser and add Bun certification matrix ([#9652](#9652)) ([a802909](a802909)) </details> <details><summary>gcp-metadata: 9.2.3</summary> ## [9.2.3](gcp-metadata-v9.2.2...gcp-metadata-v9.2.3) (2026-10-28) ### Bug Fixes * **core:** Log request/response payloads at DEBUG severity ([#9754](#9754)) ([bf46818](bf46818)) </details> <details><summary>google-auth-library: 11.3.2</summary> ## [11.3.2](google-auth-library-v11.3.1...google-auth-library-v11.3.2) (2026-10-28) ### Bug Fixes * **core:** Log request/response payloads at DEBUG severity ([#9754](#9754)) ([bf46818](bf46818)) * **google-auth-library:** Add credential deprecation and validation warnings ([#9720](#9720)) ([569678a](569678a)) </details> <details><summary>google-logging-utils: 2.1.1</summary> ## [2.1.1](google-logging-utils-v2.1.0...google-logging-utils-v2.1.1) (2026-10-28) ### Bug Fixes * **core:** Log request/response payloads at DEBUG severity ([#9754](#9754)) ([bf46818](bf46818)) * **logging-utils:** Export LogSeverity as value and treat 'true' as '*' alias ([#9718](#9718)) ([e8da4f0](e8da4f0)) </details> <details><summary>gapic-tools: 2.2.1</summary> ## [2.2.1](gapic-tools-v2.2.0...gapic-tools-v2.2.1) (2026-10-28) ### Bug Fixes * **gapic-tools:** Strip JS method bodies before pbts and parallelize minification ([#9737](#9737)) ([c7884b8](c7884b8)) </details> <details><summary>test-utils: 0.4.0</summary> ## [0.4.0](test-utils-v0.3.1...test-utils-v0.4.0) (2026-10-28) ### Features * **test-utils:** Add createProxyquire and proxyquire helper ([#9759](#9759)) ([763d1bd](763d1bd)) </details> <details><summary>bigtable: 7.5.1</summary> ## [7.5.1](bigtable-v7.5.0...bigtable-v7.5.1) (2026-10-28) ### Bug Fixes * **bigtable:** Use ReadonlyMap for EncodedKeyMap implements clause ([#9709](#9709)) ([f803bff](f803bff)) </details> <details><summary>pubsub: 6.3.2</summary> ## [6.3.2](pubsub-v6.3.1...pubsub-v6.3.2) (2026-10-28) ### Bug Fixes * **pubsub:** Reuse GoogleAuth instance and deprecate credential options ([#9721](#9721)) ([4c4ba34](4c4ba34)) </details> <details><summary>datamanager: 1.5.0</summary> ## [1.5.0](datamanager-v1.4.0...datamanager-v1.5.0) (2026-10-28) ### Features * **datamanager:** Update release_level to stable in .repo-metadata.json ([#9725](#9725)) ([6eb9340](6eb9340)), refs [#9685](#9685) </details> <details><summary>observability: 0.1.0</summary> ## 0.1.0 (2026-10-28) ### Features * **google/cloud/observability/v1:** Onboard a new library ([#9689](#9689)) ([5e53e2e](5e53e2e)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

fix(bigtable): add getOrInsert and getOrInsertComputed to EncodedKeyMap

337fecb

product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Oct 23, 2026

danieljbruce commented Oct 23, 2026

View reviewed changes

gemini-code-assist Bot reviewed Oct 23, 2026

View reviewed changes

danieljbruce mentioned this pull request Oct 23, 2026

chore(bigtable): [Style Maintenance] align EncodedKeyMap JSDoc and test coverage #9710

Draft

[Independent review follow-ups] address independent Gemini review com…

dc750b2

…ments (round 1)

gemini-code-assist Bot reviewed Oct 23, 2026

View reviewed changes

[Address CI errors] use ReadonlyMap implements clause without adding …

5da790b

…methods and fix lint

danieljbruce changed the title ~~fix(bigtable): add getOrInsert and getOrInsertComputed to EncodedKeyMap~~ fix(bigtable): use ReadonlyMap for EncodedKeyMap implements clause Oct 23, 2026

danieljbruce commented Oct 23, 2026

View reviewed changes

gemini-code-assist Bot reviewed Oct 23, 2026

View reviewed changes

[Address CI errors] address gemini-code-assist review comment on base…

cf58149

…64keymap test

danieljbruce marked this pull request as ready for review October 23, 2026 17:10

danieljbruce requested a review from a team as a code owner October 23, 2026 17:10

github-actions Bot requested a review from feywind October 23, 2026 17:10

danieljbruce requested a review from quirogas October 23, 2026 17:12

quirogas approved these changes Oct 23, 2026

View reviewed changes

danieljbruce merged commit f803bff into googleapis:main Oct 23, 2026
50 of 51 checks passed

release-please Bot mentioned this pull request Oct 23, 2026

chore: release main #9716

Merged

Conversation

danieljbruce commented Oct 23, 2026 • edited by quirogas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Stacked PR Chain

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

danieljbruce commented Oct 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Oct 23, 2026

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!

danieljbruce commented Oct 23, 2026

Uh oh!

danieljbruce commented Oct 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Oct 23, 2026

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!

danieljbruce commented Oct 23, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danieljbruce commented Oct 23, 2026 •

edited by quirogas

Loading