[SEA-NodeJS] Address #413 review: TIMESTAMP_LTZ wire type + Int64 queryTimeout coercion

Code-review #413 (gopalldb). Two P1s:

- TIMESTAMP_LTZ was sent verbatim on the wire, but Spark has no distinct
  TIMESTAMP_LTZ type (TIMESTAMP already carries LTZ semantics) — so a Thrift
  caller got an opaque server bind error, and the enum comment falsely claimed
  NTZ/LTZ "degrade to a plain TIMESTAMP bind" (there was no such logic).
  `toSparkParameter` now maps TIMESTAMP_LTZ → `TIMESTAMP` (valid on both Thrift
  and kernel); TIMESTAMP_NTZ stays native (a real Spark type). Comment corrected.
  Added DBSQLParameter tests for both wire types (the Thrift behaviour the
  review flagged as untested) and updated the kernel positional-params test.

- queryTimeout (`number | bigint | Int64`) was coerced with `Number(...)`, which
  yields NaN for an Int64 (node-int64 has no valueOf) → the client-side deadline
  was silently disabled for Int64 inputs. Now uses
  `numberToInt64(...).toNumber()`, matching the Thrift backend. Added a
  regression test that an `Int64(1)` queryTimeout actually fires the deadline
  (OperationStateError(Timeout)) rather than polling forever.

(P1 "queryTimeout silently dropped on submit" and the unbounded-poll note were
already resolved earlier by the client-side deadline fix; doc comment updated to
match. P2 polarity/Date-NTZ items noted for the public-surface follow-up.)

Validated live: NTZ binds natively and LTZ binds as TIMESTAMP on the kernel path.

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

Author: msrathore-db

lib/DBSQLParameter.ts

@@ -8,13 +8,14 @@ export enum DBSQLParameterType {
   STRING = 'STRING',
   DATE = 'DATE',
   TIMESTAMP = 'TIMESTAMP',
-  // Timezone-explicit timestamp variants. A bare `Date` value defaults to
-  // `TIMESTAMP`; set one of these explicitly to bind a TIMESTAMP_NTZ (no
-  // timezone, wall-clock) or TIMESTAMP_LTZ (local timezone) parameter. These
-  // are SEA-path types the kernel param codec accepts; the Thrift wire only
-  // has `TIMESTAMP`, so on the Thrift backend they degrade to a plain
-  // TIMESTAMP bind.
+  // `TIMESTAMP_NTZ` binds a timezone-free (wall-clock) timestamp. It is a real
+  // Spark type, bound natively on both the Thrift and kernel backends (requires
+  // a server that supports TIMESTAMP_NTZ; Spark 3.4+ / recent DBR).
   TIMESTAMP_NTZ = 'TIMESTAMP_NTZ',
+  // `TIMESTAMP_LTZ` is an alias for `TIMESTAMP`: Spark has no distinct
+  // TIMESTAMP_LTZ type — `TIMESTAMP` already carries local/instant (LTZ)
+  // semantics. `toSparkParameter` therefore binds it as `TIMESTAMP` on the wire
+  // (valid on both backends); it exists only as a self-documenting alias.
   TIMESTAMP_LTZ = 'TIMESTAMP_LTZ',
   FLOAT = 'FLOAT',
   DECIMAL = 'DECIMAL',
@@ -58,10 +59,16 @@ export class DBSQLParameter {
       return new TSparkParameter({ name }); // for NULL neither `type` nor `value` should be set
     }
 
+    // Map timezone-explicit timestamp aliases to their Spark wire type. Spark
+    // has no distinct TIMESTAMP_LTZ type (TIMESTAMP carries LTZ semantics), so
+    // bind it as TIMESTAMP — valid on both the Thrift and kernel backends.
+    // TIMESTAMP_NTZ is a real Spark type and is bound natively.
+    const wireType = this.type === DBSQLParameterType.TIMESTAMP_LTZ ? DBSQLParameterType.TIMESTAMP : this.type;
+
     if (typeof this.value === 'boolean') {
       return new TSparkParameter({
         name,
-        type: this.type ?? DBSQLParameterType.BOOLEAN,
+        type: wireType ?? DBSQLParameterType.BOOLEAN,
         value: new TSparkParameterValue({
           stringValue: this.value ? 'TRUE' : 'FALSE',
         }),
@@ -71,7 +78,7 @@ export class DBSQLParameter {
     if (typeof this.value === 'number') {
       return new TSparkParameter({
         name,
-        type: this.type ?? (Number.isInteger(this.value) ? DBSQLParameterType.INTEGER : DBSQLParameterType.DOUBLE),
+        type: wireType ?? (Number.isInteger(this.value) ? DBSQLParameterType.INTEGER : DBSQLParameterType.DOUBLE),
         value: new TSparkParameterValue({
           stringValue: Number(this.value).toString(),
         }),
@@ -81,7 +88,7 @@ export class DBSQLParameter {
     if (this.value instanceof Int64 || typeof this.value === 'bigint') {
       return new TSparkParameter({
         name,
-        type: this.type ?? DBSQLParameterType.BIGINT,
+        type: wireType ?? DBSQLParameterType.BIGINT,
         value: new TSparkParameterValue({
           stringValue: this.value.toString(),
         }),
@@ -91,7 +98,7 @@ export class DBSQLParameter {
     if (this.value instanceof Date) {
       return new TSparkParameter({
         name,
-        type: this.type ?? DBSQLParameterType.TIMESTAMP,
+        type: wireType ?? DBSQLParameterType.TIMESTAMP,
         value: new TSparkParameterValue({
           stringValue: this.value.toISOString(),
         }),
@@ -100,7 +107,7 @@ export class DBSQLParameter {
 
     return new TSparkParameter({
       name,
-      type: this.type ?? DBSQLParameterType.STRING,
+      type: wireType ?? DBSQLParameterType.STRING,
       value: new TSparkParameterValue({
         stringValue: this.value,
       }),

lib/sea/SeaSessionBackend.ts

@@ -35,6 +35,7 @@ import ParameterError from '../errors/ParameterError';
 import { LogLevel } from '../contracts/IDBSQLLogger';
 import { SeaConnection, SeaNativeExecuteOptions, SeaStatement } from './SeaNativeLoader';
 import { decodeNapiKernelError } from './SeaErrorMapping';
+import { numberToInt64 } from '../thrift-backend/ThriftSessionBackend';
 import SeaOperationBackend from './SeaOperationBackend';
 import { buildSeaPositionalParams, buildSeaNamedParams } from './SeaPositionalParams';
 import { seaServerInfoValue } from './SeaServerInfo';
@@ -121,7 +122,9 @@ export default class SeaSessionBackend implements ISessionBackend {
    * Per-statement options forwarded to the kernel `ExecuteOptions`:
    *   - `ordinalParameters` / `namedParameters` → bound params (mutually
    *     exclusive — the kernel binds one placeholder style per statement);
-   *   - `queryTimeout` → `queryTimeoutSecs` (SEA server wait timeout);
+   *   - `queryTimeout` → enforced client-side by the operation backend's poll
+   *     deadline (the kernel ignores `queryTimeoutSecs` on the async submit
+   *     path), NOT forwarded to the napi options;
    *   - `rowLimit` → `rowLimit` (SEA-only server-side row cap);
    *   - `queryTags` → serialised into the conf overlay's reserved
    *     `query_tags` key (the same wire shape Thrift's `serializeQueryTags`
@@ -176,7 +179,9 @@ export default class SeaSessionBackend implements ISessionBackend {
     return new SeaOperationBackend({
       asyncStatement: asyncStatement!,
       context: this.context,
-      queryTimeoutSecs: options.queryTimeout !== undefined ? Number(options.queryTimeout) : undefined,
+      // `queryTimeout` is typed `number | bigint | Int64`; `numberToInt64(...).toNumber()`
+      // coerces all three (a bare `Number(int64)` yields NaN — node-int64 has no valueOf).
+      queryTimeoutSecs: options.queryTimeout !== undefined ? numberToInt64(options.queryTimeout).toNumber() : undefined,
     });
   }
 

tests/unit/DBSQLParameter.test.ts

@@ -101,4 +101,27 @@ describe('DBSQLParameter', () => {
       expect(dbsqlParam.toSparkParameter()).to.deep.equal(expectedParam);
     }
   });
+
+  it('maps timezone-explicit timestamp types to valid Spark wire types', () => {
+    // TIMESTAMP_NTZ is a real Spark type → bound verbatim.
+    expect(
+      new DBSQLParameter({ type: DBSQLParameterType.TIMESTAMP_NTZ, value: '2024-01-15 10:30:00' }).toSparkParameter(),
+    ).to.deep.equal(
+      new TSparkParameter({
+        type: DBSQLParameterType.TIMESTAMP_NTZ,
+        value: new TSparkParameterValue({ stringValue: '2024-01-15 10:30:00' }),
+      }),
+    );
+    // TIMESTAMP_LTZ has no distinct Spark type → bound as TIMESTAMP (valid on
+    // both Thrift and kernel; the old verbatim 'TIMESTAMP_LTZ' was rejected by
+    // the Thrift server).
+    expect(
+      new DBSQLParameter({ type: DBSQLParameterType.TIMESTAMP_LTZ, value: '2024-01-15 10:30:00' }).toSparkParameter(),
+    ).to.deep.equal(
+      new TSparkParameter({
+        type: DBSQLParameterType.TIMESTAMP,
+        value: new TSparkParameterValue({ stringValue: '2024-01-15 10:30:00' }),
+      }),
+    );
+  });
 });

tests/unit/sea/execution.test.ts

@@ -14,6 +14,7 @@
 
 import { expect } from 'chai';
 import sinon from 'sinon';
+import Int64 from 'node-int64';
 import SeaBackend from '../../../lib/sea/SeaBackend';
 import SeaSessionBackend from '../../../lib/sea/SeaSessionBackend';
 import SeaOperationBackend from '../../../lib/sea/SeaOperationBackend';
@@ -526,6 +527,26 @@ describe('SeaSessionBackend', () => {
     expect((connection.lastOptions as { queryTimeoutSecs?: number } | undefined)?.queryTimeoutSecs).to.equal(undefined);
   });
 
+  it('coerces an Int64 queryTimeout into the client-side deadline (not NaN)', async function int64Timeout() {
+    // Regression: `Number(new Int64(...))` yields NaN (node-int64 has no valueOf),
+    // which would silently disable the deadline. The backend must coerce via
+    // numberToInt64(...).toNumber() so an Int64 queryTimeout still bounds the poll.
+    // eslint-disable-next-line no-invalid-this
+    this.timeout(5000);
+    const connection = new FakeNativeConnection();
+    connection.submitStatusValue = 'Running'; // never reaches a terminal state
+    const session = makeSession(connection);
+    const op = await session.executeStatement('SELECT 1', { queryTimeout: new Int64(1) });
+    let thrown: unknown;
+    try {
+      await op.waitUntilReady();
+    } catch (err) {
+      thrown = err;
+    }
+    expect(thrown, 'Int64(1) timeout must fire — NaN would poll forever').to.be.instanceOf(OperationStateError);
+    expect((thrown as OperationStateError).errorCode).to.equal(OperationStateErrorCode.Timeout);
+  });
+
   it('executeStatement forwards rowLimit', async () => {
     const connection = new FakeNativeConnection();
     const session = makeSession(connection);

tests/unit/sea/positionalParams.test.ts

@@ -91,15 +91,15 @@ describe('SeaPositionalParams.buildSeaPositionalParams', () => {
     ]);
   });
 
-  it('honours explicit TIMESTAMP_NTZ / TIMESTAMP_LTZ types (kernel param codec)', () => {
+  it('binds TIMESTAMP_NTZ natively and TIMESTAMP_LTZ as TIMESTAMP (Spark has no distinct LTZ type)', () => {
     expect(
       buildSeaPositionalParams([
         new DBSQLParameter({ type: DBSQLParameterType.TIMESTAMP_NTZ, value: '2024-01-15 10:30:00' }),
         new DBSQLParameter({ type: DBSQLParameterType.TIMESTAMP_LTZ, value: '2024-01-15 10:30:00' }),
       ]),
     ).to.deep.equal([
       { sqlType: 'TIMESTAMP_NTZ', value: '2024-01-15 10:30:00' },
-      { sqlType: 'TIMESTAMP_LTZ', value: '2024-01-15 10:30:00' },
+      { sqlType: 'TIMESTAMP', value: '2024-01-15 10:30:00' },
     ]);
   });
 });