fix(sea): address #411 review — exhaustive interval switch, docs, coverage

Validated every interval edge case (null, multi-row, negative sub-year,
sibling-survives) against a live pecotesting warehouse first — all byte-identical
to Thrift. The findings were layering/dead-code/coverage, not runtime bugs.

- F1: corrected the DURATION_UNIT_METADATA_KEY doc — the interval/duration
  branches are SEA-gated by construction (Thrift maps INTERVAL → ArrowString and
  never reaches them), NOT "reused by thrift" as the old comment claimed.
- F3/F6/F10: formatArrowInterval now handles YEAR_MONTH only (typed `Interval` +
  `IntervalUnit.YEAR_MONTH`, no magic `=== 0`) and THROWS on any other unit. The
  old non-exhaustive default silently misread MONTH_DAY_NANO/undefined as
  [days,ms]; and the native Interval[DayTime] branch was dead+broken (the kernel
  emits DAY-TIME as Duration, handled separately) — removed it.
- F2: exported the metadata key + added a test pinning it equal to the SEA-side
  declaration (guards against a silent rename drift).
- F8: dropped the dead `_unit` param from formatDayTimeFromTotal.
- F9: removed the dead duration check in the BigNum branch (rewritten Int64s
  arrive as raw bigint) and fixed the false "Int32Array instanceof Uint8Array"
  comment.
- F7: added a YEAR-MONTH sub-year-negative unit test (-1 month → "-0-1").
- F4: added live e2e for null INTERVAL → null and multi-row batches.
- F5: e2e before() now probes getSeaNative() and skips (not errors) when the
  binding is absent; dropped the flaky wall-clock latency assertions (assert
  behavior — cancel resolved, callback fired once).

Deferred (low/informational, noted): F11 (consolidate test makeContext helpers),
F12 (tighten instanceOf assertions), F13 (per-operation interval-representation
breadcrumb — a per-value log would be spam).

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

Author: msrathore-db

lib/result/ArrowResultConverter.ts

@@ -6,6 +6,8 @@ import {
   TypeMap,
   DataType,
   Type,
+  Interval,
+  IntervalUnit,
   StructRow,
   MapRow,
   Vector,
@@ -15,6 +17,7 @@ import {
 } from 'apache-arrow';
 import { TTableSchema, TColumnDesc } from '../../thrift/TCLIService_types';
 import IClientContext from '../contracts/IClientContext';
+import HiveDriverError from '../errors/HiveDriverError';
 import IResultsProvider, { ResultsProviderFetchNextOptions } from './IResultsProvider';
 import { ArrowBatch, getSchemaColumns, convertThriftValue } from './utils';
 
@@ -24,55 +27,51 @@ type ArrowSchema = Schema<TypeMap>;
 type ArrowSchemaField = Field<DataType<Type, TypeMap>>;
 
 /**
- * Metadata key carrying the original Arrow `Duration` time unit on
- * fields that were rewritten to `Int64` by the SEA IPC pre-processor
- * (`lib/sea/SeaArrowIpcDurationFix.ts`). We re-declare the constant
- * here (rather than importing it) so the converter has no compile-time
- * dependency on the SEA module — it's reused unchanged by the
- * thrift-path which has no SEA awareness.
+ * Metadata key carrying the original Arrow `Duration` time unit on fields
+ * rewritten to `Int64` by the SEA IPC pre-processor
+ * (`lib/sea/SeaArrowIpcDurationFix.ts`). Re-declared here (rather than
+ * imported) to keep this generic `lib/result` converter free of a
+ * compile-time dependency on `lib/sea`.
+ *
+ * **SEA-gated by construction — NOT shared with Thrift.** This key (and the
+ * `DataType.isInterval` / duration branches below) only ever execute on the
+ * SEA path. The Thrift backend sets `intervalTypesAsArrow: false` and maps
+ * both INTERVAL `TTypeId`s to `ArrowString` (`lib/result/utils.ts`), so the
+ * server pre-formats intervals to strings and this logic is never reached.
+ * `export`ed so `SeaIntervalParity.test` can pin it equal to the SEA-side
+ * declaration and catch a rename/typo that would silently no-op the consumer.
  */
-const DURATION_UNIT_METADATA_KEY = 'databricks.arrow.duration_unit';
+export const DURATION_UNIT_METADATA_KEY = 'databricks.arrow.duration_unit';
 const ZERO_BIGINT = BigInt(0);
 const NS_PER_MICRO = BigInt(1_000);
 const NS_PER_MILLI = BigInt(1_000_000);
 const NS_PER_SEC = BigInt(1_000_000_000);
-const MS_PER_DAY = BigInt(86_400_000);
 const NS_PER_MIN = NS_PER_SEC * BigInt(60);
 const NS_PER_HOUR = NS_PER_MIN * BigInt(60);
 const NS_PER_DAY = NS_PER_HOUR * BigInt(24);
 
 /**
- * Format an Arrow `Interval[YearMonth]` or `Interval[DayTime]` value
- * into the canonical thrift string the JDBC/ODBC server emits:
- *   YEAR-MONTH → `"Y-M"`         (e.g. 1 year 2 months  → `"1-2"`)
- *   DAY-TIME   → `"D HH:mm:ss.fffffffff"`
- *                                (e.g. 1 day 02:03:04   → `"1 02:03:04.000000000"`)
+ * Format a native Arrow `Interval[YearMonth]` value into the canonical thrift
+ * string `"Y-M"` (e.g. 1 year 2 months → `"1-2"`, -1 month → `"-0-1"`).
  *
- * Arrow surfaces these as `Int32Array(2)` via the `GetVisitor`
- * (`apache-arrow/visitor/get.js:177-185`):
- *   YEAR-MONTH: `[years, months]` (years/months derived from a single
- *               int32 holding total months)
- *   DAY-TIME:   `[days, milliseconds]` (legacy two-int32 form)
+ * Arrow surfaces YEAR-MONTH as an `Int32Array(2)` `[years, months]` via the
+ * `GetVisitor` (years/months derived from a single int32 of total months).
  *
- * Negative intervals: the FULL interval is emitted with a leading `-`
- * (Spark convention), and individual fields are unsigned. We mirror
- * Spark's display.
+ * **Only YEAR_MONTH reaches here.** The kernel emits INTERVAL DAY-TIME as an
+ * Arrow `Duration` (rewritten to `Int64`), handled by
+ * `formatDurationToIntervalDayTime` — never as a native `Interval[DayTime]`.
+ * Any other unit (DAY_TIME / MONTH_DAY_NANO / undefined) is therefore
+ * unexpected; we throw rather than silently misread the value as `[days, ms]`
+ * and emit a confidently-wrong string (the old non-exhaustive default).
  */
-function formatArrowInterval(value: any, valueType: any): string {
-  // `value` is an Int32Array of length 2.
-  const a = Number(value[0]);
-  const b = Number(value[1]);
-  // unit 0 = YEAR_MONTH, unit 1 = DAY_TIME, unit 2 = MONTH_DAY_NANO
-  const unit = valueType?.unit;
-  if (unit === 0) {
-    return formatYearMonth(a, b);
+function formatArrowInterval(value: Int32Array, valueType: Interval): string {
+  if (valueType?.unit !== IntervalUnit.YEAR_MONTH) {
+    throw new HiveDriverError(
+      `SEA result converter: unsupported Arrow Interval unit ${valueType?.unit}. The kernel emits only ` +
+        `YEAR_MONTH as a native Arrow Interval (DAY-TIME arrives as Duration); MONTH_DAY_NANO is unsupported.`,
+    );
   }
-  // DAY_TIME: a = days, b = milliseconds (within the day, can be ≥0 or <0)
-  // We re-normalise: total milliseconds = a * 86_400_000 + b, then split into
-  // days, hours, minutes, seconds, nanoseconds (nanoseconds is always 0
-  // because the legacy IntervalDayTime carries only millisecond precision).
-  const totalMs = BigInt(a) * MS_PER_DAY + BigInt(b);
-  return formatDayTimeFromTotal(totalMs * NS_PER_MILLI /* → ns */, 'NANOSECOND');
+  return formatYearMonth(Number(value[0]), Number(value[1]));
 }
 
 /**
@@ -107,7 +106,7 @@ function formatYearMonth(years: number, months: number): string {
 function formatDurationToIntervalDayTime(value: bigint | number, unit: string): string {
   const bi = typeof value === 'bigint' ? value : BigInt(value);
   const nanos = toNanoseconds(bi, unit);
-  return formatDayTimeFromTotal(nanos, unit);
+  return formatDayTimeFromTotal(nanos);
 }
 
 /**
@@ -137,13 +136,9 @@ function toNanoseconds(value: bigint, unit: string): bigint {
  * Always emits 9 fractional digits to match the thrift driver's wire
  * format (`"1 02:03:04.000000000"` — 9 digits regardless of the
  * server-side storage precision). Negative values get a single
- * leading `-`.
- *
- * The `unit` parameter is currently unused for formatting (the value
- * is already in nanoseconds by the time we get here) but is retained
- * for future use if a unit-aware precision is ever needed.
+ * leading `-`. The caller has already scaled to nanoseconds.
  */
-function formatDayTimeFromTotal(totalNanos: bigint, _unit: string): string {
+function formatDayTimeFromTotal(totalNanos: bigint): string {
   const sign = totalNanos < ZERO_BIGINT ? '-' : '';
   const abs = totalNanos < ZERO_BIGINT ? -totalNanos : totalNanos;
 
@@ -369,23 +364,17 @@ export default class ArrowResultConverter implements IResultsProvider<Array<any>
       if (DataType.isDecimal(valueType)) {
         return Number(result) / 10 ** valueType.scale;
       }
-      // Duration columns rewritten to Int64 — detect via metadata.
-      const durationUnit = field?.metadata.get(DURATION_UNIT_METADATA_KEY);
-      if (durationUnit) {
-        return formatDurationToIntervalDayTime(result, durationUnit);
-      }
+      // A rewritten Duration Int64 surfaces as a raw `bigint`, not a BigNum
+      // wrapper, so it is handled in the bigint branch below — not here.
       return result;
     }
 
-    // Convert binary data to Buffer
+    // Convert binary data to Buffer.
     if (value instanceof Uint8Array) {
-      // INTERVAL DAY-TIME / YEAR-MONTH that apache-arrow surfaced as
-      // an Int32Array (size 2). `Uint8Array.isInstanceOf` is true for
-      // every TypedArray subclass, so we have to check the parent type
-      // first. The `DataType.isInterval` branch above already handles
-      // the case where Arrow knew the field was an interval — this
-      // fallback covers schemas where the interval surfaced as bare
-      // bytes (defensive; not exercised in M0).
+      // Note: Arrow `Int32Array` / `BigInt64Array` are NOT `instanceof
+      // Uint8Array` (they are sibling TypedArrays), so an interval value never
+      // reaches this branch — intervals are handled by the `isInterval` /
+      // bigint branches above. This is purely the binary-column → Buffer path.
       return Buffer.from(value);
     }
 

tests/e2e/sea/interval-edge-e2e.test.ts

@@ -0,0 +1,88 @@
+// Copyright (c) 2026 Databricks, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/* eslint-disable no-console */
+
+import { expect } from 'chai';
+import { DBSQLClient } from '../../../lib';
+import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient';
+import { InternalConnectionOptions } from '../../../lib/contracts/InternalConnectionOptions';
+import { getSeaNative } from '../../../lib/sea/SeaNativeLoader';
+
+// INTERVAL edge cases the unit suite can't easily build (null, multi-row).
+// Verified byte-identical to the Thrift backend against a live warehouse.
+// Requires the pecotesting secrets AND the native binding — skips otherwise.
+
+interface PecoSecrets {
+  host: string;
+  path: string;
+  token: string;
+}
+
+function readSecrets(): PecoSecrets | null {
+  const host = process.env.DATABRICKS_PECOTESTING_SERVER_HOSTNAME;
+  const path = process.env.DATABRICKS_PECOTESTING_HTTP_PATH;
+  const token = process.env.DATABRICKS_PECOTESTING_TOKEN_PERSONAL;
+  if (!host || !path || !token) return null;
+  return { host, path, token };
+}
+
+async function seaValues(sql: string, secrets: PecoSecrets): Promise<unknown[]> {
+  const client = new DBSQLClient();
+  await client.connect({ ...secrets, useSEA: true } as ConnectionOptions & InternalConnectionOptions);
+  try {
+    const session = await client.openSession();
+    const op = await session.executeStatement(sql);
+    const rows = (await op.fetchAll()) as Array<Record<string, unknown>>;
+    await op.close();
+    await session.close();
+    return rows.map((r) => r.v);
+  } finally {
+    await client.close();
+  }
+}
+
+describe('SEA INTERVAL edge cases end-to-end', function suite() {
+  this.timeout(120_000);
+
+  const secrets = readSecrets();
+
+  before(function gate() {
+    // eslint-disable-next-line no-invalid-this
+    const self = this;
+    if (!secrets) {
+      self.skip();
+      return;
+    }
+    // Skip (not error) when the native binding isn't built/installed.
+    try {
+      getSeaNative();
+    } catch {
+      self.skip();
+    }
+  });
+
+  it('NULL INTERVAL DAY-TIME → null', async () => {
+    const values = await seaValues('SELECT CAST(NULL AS INTERVAL DAY TO SECOND) AS v', secrets as PecoSecrets);
+    expect(values).to.deep.equal([null]);
+  });
+
+  it('multi-row INTERVAL DAY-TIME batch formats every row', async () => {
+    const values = await seaValues(
+      "SELECT * FROM VALUES (INTERVAL '1' DAY), (INTERVAL '2 03:00:00' DAY TO SECOND), (INTERVAL '0' DAY) AS t(v)",
+      secrets as PecoSecrets,
+    );
+    expect(values).to.deep.equal(['1 00:00:00.000000000', '2 03:00:00.000000000', '0 00:00:00.000000000']);
+  });
+});

tests/e2e/sea/operation-lifecycle-e2e.test.ts

@@ -101,13 +101,23 @@ describe('SEA operation lifecycle — end-to-end', function suite() {
   const token = process.env.DATABRICKS_PECOTESTING_TOKEN_PERSONAL || process.env.E2E_ACCESS_TOKEN;
 
   before(function gate() {
+    // eslint-disable-next-line no-invalid-this
+    const self = this;
     if (!hostName || !httpPath || !token) {
-      // eslint-disable-next-line no-invalid-this
-      this.skip();
+      self.skip();
+      return;
+    }
+    // Creds present but the native binding not built/installed (e.g. a CI
+    // runner without the .node) must SKIP, not error: probe getSeaNative()
+    // here so every test isn't an uncaught-throw at its first call.
+    try {
+      getSeaNative();
+    } catch {
+      self.skip();
     }
   });
 
-  it('cancel() succeeds against a live SEA statement and is fast', async () => {
+  it('cancel() succeeds against a live SEA statement', async () => {
     const binding = getSeaNative() as unknown as NativeBinding;
 
     const connection = await binding.openSession({
@@ -130,12 +140,9 @@ describe('SEA operation lifecycle — end-to-end', function suite() {
         context: makeContext(),
       });
 
-      const t0 = Date.now();
+      // Assert behavior (cancel resolves with success), not wall-clock latency
+      // — a hard ms budget against a live warehouse is flaky on slow networks.
       const status = await op.cancel();
-      const elapsed = Date.now() - t0;
-
-      // Cancel must complete within 200ms.
-      expect(elapsed).to.be.lessThan(200, `cancel latency ${elapsed}ms exceeds 200ms budget`);
       expect(status.isSuccess).to.equal(true);
     } finally {
       // Bypass `op.close()` here because we want to verify cancel
@@ -170,10 +177,7 @@ describe('SEA operation lifecycle — end-to-end', function suite() {
         context: makeContext(),
       });
 
-      const t0 = Date.now();
       await op.cancel();
-      const elapsed = Date.now() - t0;
-      expect(elapsed).to.be.lessThan(200, `cancel latency ${elapsed}ms exceeds 200ms budget`);
 
       // After cancel, fetchChunk must throw the cancellation error
       // (regardless of whether the underlying fetch implementation
@@ -247,17 +251,15 @@ describe('SEA operation lifecycle — end-to-end', function suite() {
       });
 
       let ticks = 0;
-      const t0 = Date.now();
       await op.waitUntilReady({
         callback: () => {
           ticks += 1;
         },
       });
-      const elapsed = Date.now() - t0;
 
-      // M0 finished() is a no-op — must resolve in <50ms.
-      expect(elapsed).to.be.lessThan(50);
-      // Progress callback fires exactly once.
+      // M0 finished() is a no-op that resolves immediately and fires the
+      // progress callback exactly once. Assert the behavior, not a wall-clock
+      // budget (flaky against a live warehouse).
       expect(ticks).to.equal(1);
     } finally {
       if (statement !== null) {

tests/unit/sea/SeaIntervalParity.test.ts

@@ -61,6 +61,8 @@ import { TimeUnit as FbTimeUnit } from 'apache-arrow/fb/time-unit';
 
 import SeaOperationBackend from '../../../lib/sea/SeaOperationBackend';
 import ClientContextStub from '../.stubs/ClientContextStub';
+import { DURATION_UNIT_METADATA_KEY as CONVERTER_DURATION_KEY } from '../../../lib/result/ArrowResultConverter';
+import { DURATION_UNIT_METADATA_KEY as REWRITER_DURATION_KEY } from '../../../lib/sea/SeaArrowIpcDurationFix';
 
 // ---------------------------------------------------------------------------
 // Test helpers.
@@ -363,4 +365,28 @@ describe('SeaOperationBackend — INTERVAL parity with thrift', () => {
     expect(rows).to.have.length(1);
     expect((rows[0] as any).iv).to.equal('1 00:00:00.000000000');
   });
+
+  it('YEAR-MONTH negative sub-year (-1 month) → "-0-1"', async () => {
+    // |total| < 12 negatives are the trickiest formatYearMonth case (years
+    // truncates to 0, the sign must still lead). Verified live byte-identical
+    // to thrift.
+    const fields = [withTypeName(new Field('iv', new Interval(IntervalUnit.YEAR_MONTH), true), 'INTERVAL')];
+    const schema = new Schema(fields);
+    const schemaIpc = ipcSchemaOnly(schema);
+    const dataIpc = ipcFromColumns(schema, { iv: Int32Array.from([-1]) });
+
+    const stub = new StatementStub(schemaIpc, [dataIpc]);
+    const backend = new SeaOperationBackend({ statement: stub, context: new ClientContextStub() });
+    const rows = await backend.fetchChunk({ limit: 100 });
+    expect(rows).to.have.length(1);
+    expect((rows[0] as any).iv).to.equal('-0-1');
+  });
+
+  it('the duration_unit metadata key is identical in the rewriter and the converter', () => {
+    // Both modules declare the key independently (the converter avoids a
+    // compile-time dependency on lib/sea). Pin them equal so a rename/typo in
+    // one doesn't silently turn the converter's duration branch into a no-op.
+    expect(CONVERTER_DURATION_KEY).to.equal(REWRITER_DURATION_KEY);
+    expect(CONVERTER_DURATION_KEY).to.equal('databricks.arrow.duration_unit');
+  });
 });