[SEA-NodeJS] Address code-review findings on params/metadata/getInfo

Code-review-squad #412 review (79/100). Validated each finding against the
kernel source + a live warehouse before fixing:

- F1 (HIGH): getPrimaryKeys turned an omitted catalog into `?? ''`, which the
  kernel's `Identifier::new` rejects with an opaque "identifier must not be
  empty" — a regression vs Thrift (which forwards undefined). The napi
  `getPrimaryKeys(catalog: string, …)` requires a non-empty catalog, so reject
  up front with a clear, actionable message and document the divergence. Added
  a test (omitted + empty-string catalog) asserting the kernel call is never
  reached. (getCrossReference is unaffected — its parent args are nullable and
  passed through without `?? ''`.)

- F3 (MED): mutual-exclusivity of ordinal/named params threw HiveDriverError
  with a different message than the Thrift path, which throws
  ParameterError('Driver does not support both ordinal and named parameters.')
  (ParameterError extends Error, not HiveDriverError — so a caller catching it
  worked on Thrift and silently failed on SEA). Now throws the identical
  ParameterError + message. Test updated.

- F2 (MED): empirically re-verified against the live warehouse over Thrift —
  the server returns the exact same three values we synthesize ("Spark SQL" /
  "Spark SQL" / "3.1.1") and errors on every other TGetInfoType (probed
  CLI_MAX_DRIVER_CONNECTIONS, CLI_DATA_SOURCE_NAME, …). So throwing on an
  unsupported type matches Thrift's effective behaviour rather than narrowing
  it; softened the "byte-for-byte" assertion into the validated fact in both
  the SeaServerInfo and getInfo doc comments.

- F4 (LOW): getInfo's rejection now names the accepted enum identifiers/values
  (CLI_SERVER_NAME (13), CLI_DBMS_NAME (17), CLI_DBMS_VER (18)) so an
  agent/SDK consumer can self-correct.

- F5 (LOW): metadata + bound-param execute failures now emit a debug-level
  breadcrumb (mapped error + session id) before throwing, via a shared
  `logAndMapError` helper, matching the rest of the SEA backend's logging.

- F7 (LOW): dropped the vestigial `nativeStatement!` non-null assertion in
  executeStatement (typed the local, mirroring runMetadata).

- F8 (test): added coverage for the getPrimaryKeys omitted-catalog path (F1),
  the INTERVAL *→INTERVAL collapse, and the DECIMAL precision clamp-to-38.

F6 (kernel diagnostic getters unconsumed) is tracked as a follow-up — wiring
them in must land with the errorDetailsJson size-guard, out of scope here.

Validated live: getPrimaryKeys(with/without catalog), the ParameterError
parity, and the named-identifier getInfo error all behave as asserted.

Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>

Author: msrathore-db

lib/sea/SeaServerInfo.ts

@@ -22,15 +22,22 @@ import { TGetInfoType, TGetInfoValue } from '../../thrift/TCLIService_types';
  * the values client-side.
  *
  * The Databricks Thrift server itself answers only three `TGetInfoType`s and
- * rejects every other value; we mirror that surface byte-for-byte so the SEA
- * path is a drop-in equivalent:
+ * rejects every other value; we mirror that surface so the SEA path is a
+ * drop-in equivalent:
  *
  *   | TGetInfoType        | Thrift server | SEA (here)        |
  *   |---------------------|---------------|-------------------|
  *   | CLI_SERVER_NAME (13)| "Spark SQL"   | "Spark SQL"        |
  *   | CLI_DBMS_NAME   (17)| "Spark SQL"   | "Spark SQL"        |
  *   | CLI_DBMS_VER    (18)| "3.1.1"       | "3.1.1"            |
  *   | (any other)         | error         | undefined → error  |
+ *
+ * Empirically re-verified against a live warehouse over the Thrift path: the
+ * three values above are byte-identical to the server's, and the server
+ * returns an error for every other `TGetInfoType` (probed CLI_MAX_DRIVER_-
+ * CONNECTIONS, CLI_DATA_SOURCE_NAME, CLI_FETCH_DIRECTION, … — all errored). So
+ * the SEA "undefined → throw" path matches Thrift's effective behaviour rather
+ * than narrowing it.
  */
 
 /** Canonical DBMS product name — identical to the Thrift server's value. */

lib/sea/SeaSessionBackend.ts

@@ -31,6 +31,8 @@ import {
 import Status from '../dto/Status';
 import InfoValue from '../dto/InfoValue';
 import HiveDriverError from '../errors/HiveDriverError';
+import ParameterError from '../errors/ParameterError';
+import { LogLevel } from '../contracts/IDBSQLLogger';
 import { SeaConnection, SeaNativeExecuteOptions, SeaStatement } from './SeaNativeLoader';
 import { decodeNapiKernelError } from './SeaErrorMapping';
 import SeaOperationBackend from './SeaOperationBackend';
@@ -85,18 +87,27 @@ export default class SeaSessionBackend implements ISessionBackend {
   /**
    * `getInfo` (JDBC `DatabaseMetaData` / ODBC `SQLGetInfo`) has no SEA/kernel
    * endpoint, so — exactly as JDBC does for `DatabaseMetaData` — we synthesize
-   * the answer client-side, mirroring the three `TGetInfoType`s the Databricks
-   * Thrift server answers (server name / DBMS name / DBMS version) and
-   * rejecting every other value the same way the server does. See
+   * the answer client-side for the three `TGetInfoType`s the Databricks server
+   * answers (server name / DBMS name / DBMS version) and reject the rest.
+   *
+   * This is NOT a SEA-only contract narrowing: probing the live warehouse over
+   * the Thrift path confirms the server itself returns an error for every
+   * other `TGetInfoType` (CLI_MAX_DRIVER_CONNECTIONS, CLI_DATA_SOURCE_NAME, …),
+   * and the three values it does answer are byte-identical to the constants we
+   * synthesize (`"Spark SQL"` / `"Spark SQL"` / `"3.1.1"`, re-verified live).
+   * So rejecting an unsupported type matches Thrift's effective behaviour — we
+   * just surface a clearer, typed error than the server's opaque one. See
    * {@link seaServerInfoValue}.
    */
   public async getInfo(infoType: number): Promise<InfoValue> {
     this.failIfClosed();
     const value = seaServerInfoValue(infoType);
     if (value === undefined) {
       throw new HiveDriverError(
-        `SEA getInfo: unsupported TGetInfoType ${infoType}. The SEA/kernel path answers only the ` +
-          `info types the Databricks server itself supports (server name, DBMS name, DBMS version).`,
+        `SEA getInfo: unsupported TGetInfoType ${infoType}. Only the info types the Databricks ` +
+          `server itself answers are supported: CLI_SERVER_NAME (13), CLI_DBMS_NAME (17), ` +
+          `CLI_DBMS_VER (18). The server rejects every other type on the Thrift path too, so this ` +
+          `is not a SEA-specific restriction.`,
       );
     }
     return new InfoValue(value);
@@ -123,14 +134,14 @@ export default class SeaSessionBackend implements ISessionBackend {
     this.failIfClosed();
 
     // Positional (`?`) and named (`:name`) parameters are mutually exclusive —
-    // the kernel param codec binds exactly one placeholder style per
-    // statement, so passing both is a caller error we surface up-front.
+    // the kernel param codec binds exactly one placeholder style per statement.
+    // Use the SAME error type and message as the Thrift backend
+    // (`ThriftSessionBackend.getQueryParameters`) so a caller catching
+    // `ParameterError` for this case behaves identically across backends.
     const positionalParams = buildSeaPositionalParams(options.ordinalParameters);
     const namedParams = buildSeaNamedParams(options.namedParameters);
     if (positionalParams !== undefined && namedParams !== undefined) {
-      throw new HiveDriverError(
-        'SEA executeStatement: ordinalParameters and namedParameters are mutually exclusive — pass only one.',
-      );
+      throw new ParameterError('Driver does not support both ordinal and named parameters.');
     }
 
     if (options.queryTimeout !== undefined) {
@@ -168,16 +179,16 @@ export default class SeaSessionBackend implements ISessionBackend {
       execOptions = { positionalParams, namedParams };
     }
 
-    let nativeStatement;
+    let nativeStatement: SeaStatement;
     try {
       nativeStatement =
         execOptions === undefined
           ? await this.connection.executeStatement(statement)
           : await this.connection.executeStatement(statement, execOptions);
     } catch (err) {
-      throw decodeNapiKernelError(err);
+      throw this.logAndMapError('executeStatement', err);
     }
-    return this.wrapStatement(nativeStatement!);
+    return this.wrapStatement(nativeStatement);
   }
 
   /** Wrap a napi `Statement` (from execute or a metadata call) as an operation backend. */
@@ -244,8 +255,19 @@ export default class SeaSessionBackend implements ISessionBackend {
 
   public async getPrimaryKeys(request: PrimaryKeysRequest): Promise<IOperationBackend> {
     this.failIfClosed();
+    // The kernel requires a catalog for primary-key lookup (`Identifier::new`
+    // rejects an empty string). The Thrift backend can forward an undefined
+    // catalog and let the server resolve a default; the SEA/kernel path cannot,
+    // so reject up front with a clear, actionable message rather than passing
+    // `''` and surfacing the kernel's opaque "identifier must not be empty".
+    if (request.catalogName === undefined || request.catalogName === '') {
+      throw new HiveDriverError(
+        'SEA getPrimaryKeys requires a catalog — pass `catalogName` explicitly. (The Thrift backend ' +
+          'can omit it and let the server resolve a default; the SEA kernel path requires it.)',
+      );
+    }
     return this.runMetadata(() =>
-      this.connection.getPrimaryKeys(request.catalogName ?? '', request.schemaName, request.tableName),
+      this.connection.getPrimaryKeys(request.catalogName as string, request.schemaName, request.tableName),
     );
   }
 
@@ -265,15 +287,27 @@ export default class SeaSessionBackend implements ISessionBackend {
 
   /** Run a napi metadata call, mapping kernel errors and wrapping the result handle. */
   private async runMetadata(call: () => Promise<SeaStatement>): Promise<IOperationBackend> {
-    let nativeStatement;
+    let nativeStatement: SeaStatement;
     try {
       nativeStatement = await call();
     } catch (err) {
-      throw decodeNapiKernelError(err);
+      throw this.logAndMapError('metadata', err);
     }
     return this.wrapStatement(nativeStatement);
   }
 
+  /**
+   * Map a napi/kernel error to a typed driver error and emit a debug breadcrumb
+   * first, matching the rest of the SEA backend's logging convention
+   * (`SeaOperationLifecycle` / `SeaOperationBackend`). Metadata and bound-param
+   * execute failures otherwise threw with no on-call signal.
+   */
+  private logAndMapError(op: string, err: unknown): Error {
+    const mapped = decodeNapiKernelError(err);
+    this.context.getLogger().log(LogLevel.debug, `SEA ${op} failed for session ${this._id}: ${mapped.message}`);
+    return mapped;
+  }
+
   public async close(): Promise<Status> {
     if (this.closed) {
       return Status.success();

tests/unit/sea/execution.test.ts

@@ -21,6 +21,7 @@ import { SeaNativeBinding, SeaConnection, SeaStatement } from '../../../lib/sea/
 import IClientContext, { ClientConfig } from '../../../lib/contracts/IClientContext';
 import IDBSQLLogger, { LogLevel } from '../../../lib/contracts/IDBSQLLogger';
 import HiveDriverError from '../../../lib/errors/HiveDriverError';
+import ParameterError from '../../../lib/errors/ParameterError';
 import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient';
 
 // -----------------------------------------------------------------------------
@@ -434,7 +435,7 @@ describe('SeaSessionBackend', () => {
     expect(connection.lastOptions).to.equal(undefined);
   });
 
-  it('executeStatement rejects mixing ordinal and named parameters (mutually exclusive)', async () => {
+  it('executeStatement rejects mixing ordinal and named parameters with the same ParameterError as Thrift', async () => {
     const connection = new FakeNativeConnection();
     const session = makeSession(connection);
     let thrown: unknown;
@@ -443,8 +444,10 @@ describe('SeaSessionBackend', () => {
     } catch (err) {
       thrown = err;
     }
-    expect(thrown).to.be.instanceOf(HiveDriverError);
-    expect((thrown as Error).message).to.match(/mutually exclusive/);
+    // Cross-backend parity: ThriftSessionBackend throws ParameterError with this
+    // exact message, so a caller catching ParameterError behaves identically.
+    expect(thrown).to.be.instanceOf(ParameterError);
+    expect((thrown as Error).message).to.equal('Driver does not support both ordinal and named parameters.');
   });
 
   it('executeStatement rejects queryTimeout (M1)', async () => {
@@ -521,6 +524,24 @@ describe('SeaSessionBackend', () => {
     ]);
   });
 
+  it('getPrimaryKeys rejects an omitted catalog up front (the kernel requires one)', async () => {
+    const connection = new FakeNativeConnection();
+    const session = makeSession(connection);
+    for (const request of [{ schemaName: 'def', tableName: 't' }, { catalogName: '', schemaName: 'def', tableName: 't' }]) {
+      let thrown: unknown;
+      try {
+        // eslint-disable-next-line no-await-in-loop
+        await session.getPrimaryKeys(request);
+      } catch (err) {
+        thrown = err;
+      }
+      expect(thrown, `expected reject for ${JSON.stringify(request)}`).to.be.instanceOf(HiveDriverError);
+      expect((thrown as Error).message).to.match(/requires a catalog/);
+    }
+    // The kernel call must NOT be reached (no empty-identifier sent over FFI).
+    expect(connection.metadataCalls.filter((c) => c[0] === 'getPrimaryKeys')).to.have.length(0);
+  });
+
   it('getInfo synthesizes the three server-answered info types and rejects the rest', async () => {
     const connection = new FakeNativeConnection();
     const session = makeSession(connection);

tests/unit/sea/positionalParams.test.ts

@@ -39,6 +39,27 @@ describe('SeaPositionalParams.buildSeaPositionalParams', () => {
     ).to.deep.equal([{ sqlType: 'DECIMAL(3,0)', value: '-123' }]);
   });
 
+  it('clamps DECIMAL precision to the Databricks max of 38', () => {
+    // 40 integer digits → precision clamped to 38 (scale 0).
+    expect(
+      buildSeaPositionalParams([
+        new DBSQLParameter({ type: DBSQLParameterType.DECIMAL, value: '1234567890123456789012345678901234567890' }),
+      ]),
+    ).to.deep.equal([{ sqlType: 'DECIMAL(38,0)', value: '1234567890123456789012345678901234567890' }]);
+  });
+
+  it('collapses every INTERVAL subtype to the kernel codec\'s single "INTERVAL" type name', () => {
+    expect(
+      buildSeaPositionalParams([
+        new DBSQLParameter({ type: DBSQLParameterType.INTERVALMONTH, value: '13' }),
+        new DBSQLParameter({ type: DBSQLParameterType.INTERVALDAY, value: '1 02:03:04' }),
+      ]),
+    ).to.deep.equal([
+      { sqlType: 'INTERVAL', value: '13' },
+      { sqlType: 'INTERVAL', value: '1 02:03:04' },
+    ]);
+  });
+
   it('maps NULL to a value-less VOID input', () => {
     expect(buildSeaPositionalParams([null])).to.deep.equal([{ sqlType: 'VOID' }]);
   });