Skip to content

Commit 8f1b268

Browse files
committed
[SEA-NodeJS] Make queryTimeout a no-op on SEA (stop abusing wait_timeout)
`queryTimeout` was mapped to the kernel's `wait_timeout` (via `query_timeout_secs` → `wait_timeout={N}s` + CANCEL). That is semantically wrong: `wait_timeout` is the server's inline-HOLD window (how long the POST blocks for results, capped 5–50s), NOT a statement-execution timeout. Mapping queryTimeout there silently caps it at 50s, rejects <5s with HTTP 400, and conflates the inline wait with execution. Per the option's own JSDoc, `queryTimeout` is "effective only with Compute clusters; for SQL Warehouses use the STATEMENT_TIMEOUT configuration" — and SEA targets SQL Warehouses. So make it a clean no-op on the SEA backend: not forwarded to the kernel (sync directResults path no longer sends `query_timeout_secs`), and not applied as a client-side deadline (async path). Drop the now-dead `buildExecuteOptions` param and the unused `numberToInt64` import. Verified e2e: `queryTimeout: 5` on a ~35s query previously cancelled at ~5s; it now runs to completion (no-op). directResults CREATE/close/cancel unaffected. Tests: sync + async paths now assert queryTimeout is NOT forwarded; removed the obsolete Int64 client-side-deadline test. SEA suite 262 passing; eslint clean. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
1 parent d29e675 commit 8f1b268

2 files changed

Lines changed: 34 additions & 67 deletions

File tree

lib/sea/SeaSessionBackend.ts

Lines changed: 28 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ import ParameterError from '../errors/ParameterError';
3535
import { LogLevel } from '../contracts/IDBSQLLogger';
3636
import { SeaConnection, SeaNativeExecuteOptions, SeaStatement, SeaNativeAsyncStatement } from './SeaNativeLoader';
3737
import { decodeNapiKernelError } from './SeaErrorMapping';
38-
import { numberToInt64 } from '../thrift-backend/ThriftSessionBackend';
3938
import SeaOperationBackend from './SeaOperationBackend';
4039
import { buildSeaPositionalParams, buildSeaNamedParams } from './SeaPositionalParams';
4140
import { seaServerInfoValue } from './SeaServerInfo';
@@ -135,9 +134,9 @@ export default class SeaSessionBackend implements ISessionBackend {
135134
* Per-statement options forwarded to the kernel `ExecuteOptions`:
136135
* - `ordinalParameters` / `namedParameters` → bound params (mutually
137136
* exclusive — the kernel binds one placeholder style per statement);
138-
* - `queryTimeout` → enforced client-side by the operation backend's poll
139-
* deadline (the kernel ignores `queryTimeoutSecs` on the async submit
140-
* path), NOT forwarded to the napi options;
137+
* - `queryTimeout` → NO-OP on SEA (SQL Warehouses use `STATEMENT_TIMEOUT`);
138+
* never forwarded to the kernel and never applied as a client-side
139+
* deadline — see the note in `executeStatement`;
141140
* - `rowLimit` → `rowLimit` (SEA-only server-side row cap);
142141
* - `queryTags` → serialised into the conf overlay's reserved
143142
* `query_tags` key (the same wire shape Thrift's `serializeQueryTags`
@@ -183,20 +182,25 @@ export default class SeaSessionBackend implements ISessionBackend {
183182
// query, or a still-running `AsyncStatement` (poll/cancel handle) for a
184183
// slow one. The handle is server-owned, so a long query stays cancellable
185184
// via `op.cancel()` and `close()` is a clean release (no client-side
186-
// drive-to-terminal). `queryTimeoutSecs` IS honoured here (forwarded to
187-
// the napi options below) — note the kernel sends it as `wait_timeout=Ns`
188-
// + CANCEL, so a timeout shorter than the query cancels it (eager error)
189-
// rather than returning the `Running` handle.
185+
// drive-to-terminal). `queryTimeout` is a no-op here (see the note
186+
// below) — it is NOT mapped to the server `wait_timeout`.
190187
//
191188
// - `runAsync: true` — ASYNC. Submit (`wait_timeout=0s`): the server
192189
// returns a pending `AsyncStatement` immediately while the query runs;
193190
// the backend polls `status()` to terminal in `waitUntilReady()` and
194-
// materialises results via `awaitResult()`. `queryTimeoutSecs` is
195-
// ignored by the kernel on submit, so it is enforced client-side by the
196-
// operation backend's poll-loop deadline instead.
191+
// materialises results via `awaitResult()`. `queryTimeout` is a no-op
192+
// here too.
197193
const runAsync = options.runAsync ?? false;
198-
const queryTimeoutSecs =
199-
options.queryTimeout !== undefined ? numberToInt64(options.queryTimeout).toNumber() : undefined;
194+
// `queryTimeout` is a NO-OP on the SEA (kernel) backend. It is the JDBC
195+
// `setQueryTimeout` knob which — per the option's JSDoc — is effective only
196+
// on Compute clusters; SQL Warehouses (what SEA targets) use the
197+
// `STATEMENT_TIMEOUT` SQL/session conf instead. We deliberately do NOT map it
198+
// to the SEA `wait_timeout` field: `wait_timeout` is the server's inline-hold
199+
// window (the time the POST blocks for results, capped 5–50s), NOT a
200+
// statement-execution timeout — mapping it there silently caps the timeout at
201+
// 50s, rejects <5s with HTTP 400, and conflates the inline wait with
202+
// execution. So `queryTimeout` is neither forwarded to the kernel nor used as
203+
// a client-side deadline.
200204

201205
if (!runAsync) {
202206
// DEFAULT — directResults (the Thrift/JDBC model). The kernel's
@@ -208,7 +212,7 @@ export default class SeaSessionBackend implements ISessionBackend {
208212
// query stays cancellable via `op.cancel()` and `close()` is a clean
209213
// release (no client-side drive-to-terminal). Fire-and-forget DDL/DML
210214
// commits because the server runs it inline during the POST.
211-
const execOptions = this.buildExecuteOptions(options, queryTimeoutSecs);
215+
const execOptions = this.buildExecuteOptions(options);
212216
let direct: SeaStatement | SeaNativeAsyncStatement;
213217
try {
214218
direct =
@@ -231,13 +235,13 @@ export default class SeaSessionBackend implements ISessionBackend {
231235
// Feature-detect the arm via a type guard: only the Running `AsyncStatement`
232236
// exposes `awaitResult`; the terminal `Statement` (Completed arm) does not.
233237
if (isSeaAsyncStatement(direct)) {
234-
return new SeaOperationBackend({ asyncStatement: direct, context: this.context, queryTimeoutSecs });
238+
return new SeaOperationBackend({ asyncStatement: direct, context: this.context });
235239
}
236-
return new SeaOperationBackend({ statement: direct, context: this.context, queryTimeoutSecs });
240+
return new SeaOperationBackend({ statement: direct, context: this.context });
237241
}
238242

239-
// Async path: do NOT forward `queryTimeoutSecs` (the kernel ignores it on
240-
// submit — `wait_timeout=0s`); it is enforced client-side by the poll loop.
243+
// Async path: submit (`wait_timeout=0s`) returns a pending handle the
244+
// backend polls. (`queryTimeout` is a no-op on SEA — see the note above.)
241245
const execOptions = this.buildExecuteOptions(options);
242246
let asyncStatement;
243247
try {
@@ -248,16 +252,11 @@ export default class SeaSessionBackend implements ISessionBackend {
248252
} catch (err) {
249253
throw this.logAndMapError('executeStatement', err);
250254
}
251-
// `queryTimeout` is enforced client-side by the operation backend's poll
252-
// loop: the kernel ignores `queryTimeoutSecs` on the async submit path
253-
// (`submitStatement` always sends `wait_timeout=0s`), so we do NOT forward
254-
// it to the napi options — passing it there would be a silent no-op.
255+
// `queryTimeout` is a no-op on SEA (see the note at the top of this method);
256+
// not forwarded to the kernel and not applied as a client-side deadline.
255257
return new SeaOperationBackend({
256258
asyncStatement: asyncStatement!,
257259
context: this.context,
258-
// `queryTimeout` is typed `number | bigint | Int64`; `numberToInt64(...).toNumber()`
259-
// coerces all three (a bare `Number(int64)` yields NaN — node-int64 has no valueOf).
260-
queryTimeoutSecs,
261260
});
262261
}
263262

@@ -266,10 +265,7 @@ export default class SeaSessionBackend implements ISessionBackend {
266265
* `ExecuteOptions`, returning `undefined` when nothing is set so the
267266
* no-options call shape (`executeStatement(sql)`) is preserved.
268267
*/
269-
private buildExecuteOptions(
270-
options: ExecuteStatementOptions,
271-
queryTimeoutSecs?: number,
272-
): SeaNativeExecuteOptions | undefined {
268+
private buildExecuteOptions(options: ExecuteStatementOptions): SeaNativeExecuteOptions | undefined {
273269
// Positional (`?`) and named (`:name`) parameters are mutually exclusive —
274270
// the kernel binds one placeholder style per statement. Use the SAME error
275271
// type and message as the Thrift backend (`ThriftSessionBackend`) so a
@@ -287,14 +283,9 @@ export default class SeaSessionBackend implements ISessionBackend {
287283
if (namedParams !== undefined) {
288284
execOptions.namedParams = namedParams;
289285
}
290-
// `queryTimeoutSecs` is forwarded only on the SYNC path (the caller passes
291-
// it in): the kernel `execute()` consults it as the server statement
292-
// timeout. On the async submit path the caller omits it (the kernel ignores
293-
// it under `wait_timeout=0s`), so it is enforced client-side by the
294-
// operation backend's poll-loop deadline instead (see executeStatement).
295-
if (queryTimeoutSecs !== undefined) {
296-
execOptions.queryTimeoutSecs = queryTimeoutSecs;
297-
}
286+
// NB: `queryTimeout` is intentionally NOT forwarded — it is a no-op on SEA
287+
// (SQL Warehouses use `STATEMENT_TIMEOUT`; mapping it to `wait_timeout` would
288+
// abuse the inline-hold window). See the note in `executeStatement`.
298289
if (options.rowLimit !== undefined) {
299290
execOptions.rowLimit = Number(options.rowLimit);
300291
}

tests/unit/sea/execution.test.ts

Lines changed: 6 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414

1515
import { expect } from 'chai';
1616
import sinon from 'sinon';
17-
import Int64 from 'node-int64';
1817
import SeaBackend from '../../../lib/sea/SeaBackend';
1918
import SeaSessionBackend from '../../../lib/sea/SeaSessionBackend';
2019
import SeaOperationBackend from '../../../lib/sea/SeaOperationBackend';
@@ -650,46 +649,23 @@ describe('SeaSessionBackend', () => {
650649
expect((thrown as Error).message).to.equal('Driver does not support both ordinal and named parameters.');
651650
});
652651

653-
it('executeStatement (sync default) DOES forward queryTimeout to the napi options', async () => {
652+
it('executeStatement (sync default) does NOT forward queryTimeout — no-op on SEA', async () => {
654653
const connection = new FakeNativeConnection();
655654
const session = makeSession(connection);
656655
await session.executeStatement('SELECT 1', { queryTimeout: 30 });
657-
// Sync path: the kernel `execute()` honours queryTimeoutSecs (server
658-
// statement timeout), so the backend forwards it onto the napi options.
659-
expect((connection.lastOptions as { queryTimeoutSecs?: number } | undefined)?.queryTimeoutSecs).to.equal(30);
656+
// queryTimeout is a no-op on SEA (SQL Warehouses use STATEMENT_TIMEOUT). It
657+
// must NOT be mapped to the kernel's `wait_timeout` (the inline-hold window),
658+
// so nothing is forwarded onto the napi options.
659+
expect((connection.lastOptions as { queryTimeoutSecs?: number } | undefined)?.queryTimeoutSecs).to.equal(undefined);
660660
});
661661

662-
it('executeStatement (runAsync: true) does NOT forward queryTimeout to submit (kernel ignores it; enforced client-side)', async () => {
662+
it('executeStatement (runAsync: true) does NOT forward queryTimeout — no-op on SEA', async () => {
663663
const connection = new FakeNativeConnection();
664664
const session = makeSession(connection);
665665
await session.executeStatement('SELECT 1', { queryTimeout: 30, runAsync: true });
666-
// Async submit path: the kernel ignores queryTimeoutSecs under
667-
// `wait_timeout=0s`, so it's enforced client-side by the poll deadline
668-
// instead — never forwarded to the napi options.
669666
expect((connection.lastOptions as { queryTimeoutSecs?: number } | undefined)?.queryTimeoutSecs).to.equal(undefined);
670667
});
671668

672-
it('coerces an Int64 queryTimeout into the client-side deadline on the async path (not NaN)', async function int64Timeout() {
673-
// Regression: `Number(new Int64(...))` yields NaN (node-int64 has no valueOf),
674-
// which would silently disable the deadline. The backend must coerce via
675-
// numberToInt64(...).toNumber() so an Int64 queryTimeout still bounds the poll.
676-
// Exercised on the async path, where the client-side poll deadline applies.
677-
// eslint-disable-next-line no-invalid-this
678-
this.timeout(5000);
679-
const connection = new FakeNativeConnection();
680-
connection.submitStatusValue = 'Running'; // never reaches a terminal state
681-
const session = makeSession(connection);
682-
const op = await session.executeStatement('SELECT 1', { queryTimeout: new Int64(1), runAsync: true });
683-
let thrown: unknown;
684-
try {
685-
await op.waitUntilReady();
686-
} catch (err) {
687-
thrown = err;
688-
}
689-
expect(thrown, 'Int64(1) timeout must fire — NaN would poll forever').to.be.instanceOf(OperationStateError);
690-
expect((thrown as OperationStateError).errorCode).to.equal(OperationStateErrorCode.Timeout);
691-
});
692-
693669
it('executeStatement forwards rowLimit', async () => {
694670
const connection = new FakeNativeConnection();
695671
const session = makeSession(connection);

0 commit comments

Comments
 (0)