Skip to content

Commit bc4571d

Browse files
authored
[SEA-NodeJS] Sync execute via directResults (executeStatementDirect): fix CREATE, drop close-drives, keep cancel (#426)
* [SEA-NodeJS] Sync execute via directResults (executeStatementDirect) The default sync path (`runAsync: false`) now calls the kernel's additive `Connection.executeStatementDirect` instead of `executeStatementCancellable`. The kernel runs ExecuteStatement with a bounded server inline wait and returns WITHOUT polling past it; the session feature-detects the arm via `awaitResult`: a fast query comes back as a terminal `Statement` (result inline) → wrapped with the operation backend's `statement` arm; a slow one as an `AsyncStatement` → the `asyncStatement` arm. Because the returned handle always corresponds to a server-owned statement: - fire-and-forget CREATE/INSERT commits (server runs it inline in the POST); - `close()` is a clean release, never a drive-to-terminal (no close-drives); - a long query stays cancellable via `op.cancel()` (~150-300ms), Thrift parity; - errors surface at `executeStatement`, matching Thrift / Python use_kernel. Requires the kernel's additive directResults execute (databricks/databricks-sql-kernel#140). `execute()` on the kernel is unchanged, so Python `use_kernel` needs no change. Regenerated napi types add `executeStatementDirect`. Validated e2e (pecotesting): CREATE fire-and-forget commits, 100k read, error at execute, mid-run cancel, close() cheap (~120ms) on an abandoned long query. Unit: SEA suite 260 passing; eslint clean. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * [SEA-NodeJS] Address review: test the AsyncStatement arm; type-guard; null + stale comments - F1: add coverage for the directResults Running (AsyncStatement) arm — the branch the PR exists to add. Three tests via the fake's `directReturnsRunning`: (a) a still-running query routes through the AsyncStatement arm and is driven via `status()`/`waitUntilReady()`; (b) `op.cancel()` reaches the running statement's `cancel()`; (c) contrast — a fast query routes through the terminal `Statement` arm and cancel reaches it there. Previously `directReturnsRunning` was never set, so the async arm had zero coverage. - F4: replace the `as unknown as` double-casts with a cast-free user-defined type guard `isSeaAsyncStatement` (the napi `Statement`/`AsyncStatement` are the exact alias types, so no laundering is needed). Idiomatic, narrows both arms. - F5: reject a null/undefined `direct` up front via `logAndMapError` (HiveDriverError) instead of the inconsistent `direct !== null` guard that let a null fall through to the `{statement}` arm and defer an opaque TypeError. - F3: update the stale `runAsync` selector comment (still described `executeStatementCancellable` + blocking `result()`) to directResults; fix the stale `executeStatementCancellable` comment in the test. - Document the `queryTimeout`→`wait_timeout=Ns`+CANCEL interaction (a timeout shorter than the query cancels it rather than returning the Running handle). SEA unit suite 263 passing (3 new); eslint clean. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * [SEA-NodeJS] Regenerate napi .d.ts JSDoc to match corrected kernel direct-path docs executeStatementDirect JSDoc now reflects: no wait_timeout field (server default inline wait + auto-close), the awaitResult feature-detect contract, and the queryTimeout->CANCEL interaction. Doc-only; no API surface change. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * [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> * [SEA-NodeJS] Remove the dead queryTimeout deadline plumbing (kernel field gone) Follow-on to making queryTimeout a no-op on SEA: now that the kernel's `query_timeout_secs` field is removed (databricks/databricks-sql-kernel#140) and SeaSessionBackend no longer passes a timeout, the SeaOperationBackend client-side deadline is dead code. Remove it: - `SeaOperationBackend`: drop the `queryTimeoutSecs` option, the `queryTimeoutMs` field, and the async poll-loop deadline enforcement (the `OperationStateError(Timeout)` + best-effort-cancel branch). - Regenerated napi `.d.ts`: `ExecuteOptions` no longer carries `queryTimeoutSecs`. - Tests: remove the obsolete client-side-deadline test; drop the `queryTimeoutSecs` args from the `makeAsyncOp` / `makeSyncOp` helpers. - Comment cleanups in SeaNativeLoader. queryTimeout remains a no-op on SEA (SQL Warehouses use STATEMENT_TIMEOUT). SEA unit suite 261 passing; eslint clean; e2e directResults (CREATE/close/cancel) unaffected. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * style: prettier-format SeaOperationBackend + execution.test after queryTimeout removal CI runs `prettier . --check` (not just eslint); the deadline-plumbing removal left two files needing a prettier reflow. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> --------- Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
1 parent f4d1675 commit bc4571d

5 files changed

Lines changed: 184 additions & 192 deletions

File tree

lib/sea/SeaNativeLoader.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ export type SeaStatement = NativeStatement;
5656
// Per-statement execution options and bound-parameter inputs are kernel
5757
// concerns: the napi binding generates the canonical shapes (`positionalParams`
5858
// / `namedParams` as `TypedValueInput` / `NamedTypedValueInput`, plus
59-
// `rowLimit`, `queryTimeoutSecs`, `statementConf`, `queryTags`). We re-export
59+
// `rowLimit`, `statementConf`, `queryTags`). We re-export
6060
// rather than re-declare so the driver-side param codec can never drift from
6161
// the kernel contract.
6262
export type SeaNativeExecuteOptions = NativeExecuteOptions;

lib/sea/SeaOperationBackend.ts

Lines changed: 4 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -140,15 +140,6 @@ export interface SeaOperationBackendOptions {
140140
* handle exposes one, else a fresh UUIDv4.
141141
*/
142142
id?: string;
143-
/**
144-
* Client-side query timeout in whole seconds (the public `queryTimeout`).
145-
* The kernel ignores `queryTimeoutSecs` on the async submit path
146-
* (`submitStatement` always sends `wait_timeout=0s`), so the JS poll loop
147-
* enforces it as a deadline — on expiry it best-effort cancels the statement
148-
* and throws `OperationStateError(Timeout)`, matching the Thrift path's
149-
* server-side TIMEDOUT outcome. Omitted ⇒ no client-side deadline.
150-
*/
151-
queryTimeoutSecs?: number;
152143
}
153144

154145
export default class SeaOperationBackend implements IOperationBackend {
@@ -190,18 +181,7 @@ export default class SeaOperationBackend implements IOperationBackend {
190181
// already-terminal statement. Drives both fetch and result-metadata.
191182
private fetchHandlePromise?: Promise<SeaFetchHandle>;
192183

193-
// Client-side query-timeout deadline in ms (the public `queryTimeout`),
194-
// undefined when unset. Enforced in the async poll loop.
195-
private readonly queryTimeoutMs?: number;
196-
197-
constructor({
198-
asyncStatement,
199-
statement,
200-
cancellableExecution,
201-
context,
202-
id,
203-
queryTimeoutSecs,
204-
}: SeaOperationBackendOptions) {
184+
constructor({ asyncStatement, statement, cancellableExecution, context, id }: SeaOperationBackendOptions) {
205185
// Exactly one of the three handle kinds must be supplied.
206186
const providedCount =
207187
(asyncStatement !== undefined ? 1 : 0) +
@@ -232,7 +212,6 @@ export default class SeaOperationBackend implements IOperationBackend {
232212
this.context = context;
233213
this._id =
234214
id ?? asyncStatement?.statementId ?? statement?.statementId ?? cancellableExecution?.statementId ?? uuidv4();
235-
this.queryTimeoutMs = queryTimeoutSecs !== undefined && queryTimeoutSecs > 0 ? queryTimeoutSecs * 1000 : undefined;
236215
}
237216

238217
public get id(): string {
@@ -441,9 +420,6 @@ export default class SeaOperationBackend implements IOperationBackend {
441420
if (this.fetchHandlePromise) {
442421
return;
443422
}
444-
// Client-side timeout deadline: the kernel ignores queryTimeoutSecs on the
445-
// async submit path, so we enforce the public `queryTimeout` here.
446-
const deadline = this.queryTimeoutMs !== undefined ? Date.now() + this.queryTimeoutMs : undefined;
447423
for (;;) {
448424
// A JS-initiated cancel/close short-circuits before the next poll.
449425
failIfNotActive(this.lifecycle);
@@ -494,30 +470,8 @@ export default class SeaOperationBackend implements IOperationBackend {
494470
throw new OperationStateError(OperationStateErrorCode.Unknown);
495471
}
496472

497-
// Still Pending/Running — enforce the client-side timeout before sleeping.
498-
if (deadline !== undefined && Date.now() >= deadline) {
499-
// Best-effort server-side cancel so the statement doesn't keep running
500-
// after we stop waiting; never mask the timeout with a cancel failure,
501-
// but warn-log a failed cancel so a still-running server statement is
502-
// diagnosable (mirrors the fetch-error cleanup path).
503-
// eslint-disable-next-line no-await-in-loop
504-
await this.cancel().catch((cancelErr) => {
505-
const cause = cancelErr instanceof Error ? cancelErr.message : String(cancelErr);
506-
this.context
507-
.getLogger()
508-
.log(
509-
LogLevel.warn,
510-
`SEA query-timeout cleanup: cancel() failed for operation ${this._id}; the server-side ` +
511-
`statement may keep running until the session is closed. Cause: ${cause}`,
512-
);
513-
});
514-
// Release the statement handle too (cancel stops the work; close frees
515-
// the handle) — best-effort, mirroring the other terminal branches.
516-
// eslint-disable-next-line no-await-in-loop
517-
await this.bestEffortClose();
518-
throw new OperationStateError(OperationStateErrorCode.Timeout);
519-
}
520-
473+
// Still Pending/Running — sleep before the next poll. (There is no
474+
// client-side query-timeout deadline: `queryTimeout` is a no-op on SEA.)
521475
// eslint-disable-next-line no-await-in-loop
522476
await delay(STATUS_POLL_INTERVAL_MS);
523477
}
@@ -526,7 +480,7 @@ export default class SeaOperationBackend implements IOperationBackend {
526480
/**
527481
* Sync (`runAsync: false`) execute path. Drives the blocking
528482
* `CancellableExecution.result()` to a terminal `Statement` (the kernel polls
529-
* to completion server-side, honouring `queryTimeoutSecs` on this path). The
483+
* to completion server-side). The
530484
* await is interruptible: a JS-initiated `cancel()` fires the detached
531485
* canceller, the server flips the statement terminal, and the parked
532486
* `result()` rejects with `Cancelled` — which we map to the typed

lib/sea/SeaSessionBackend.ts

Lines changed: 75 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,8 @@ import InfoValue from '../dto/InfoValue';
3333
import HiveDriverError from '../errors/HiveDriverError';
3434
import ParameterError from '../errors/ParameterError';
3535
import { LogLevel } from '../contracts/IDBSQLLogger';
36-
import { SeaConnection, SeaNativeExecuteOptions, SeaStatement } from './SeaNativeLoader';
36+
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';
@@ -67,6 +66,19 @@ export interface SeaSessionBackendOptions {
6766
* needed — that pattern was removed when the napi binding moved these
6867
* onto `openSession` to match pyo3.
6968
*/
69+
70+
/**
71+
* Narrow the directResults union (`executeStatementDirect`'s
72+
* `Statement | AsyncStatement`) to the Running `AsyncStatement` arm. Only that
73+
* arm exposes `awaitResult`; the terminal `Statement` (Completed arm) does not.
74+
* Mirrors the kernel `DirectStatement::{Completed, Running}` discriminant, which
75+
* the opaque napi classes can't carry on the wire — the `awaitResult` probe is
76+
* the load-bearing feature-detect (see databricks-sql-kernel#140).
77+
*/
78+
function isSeaAsyncStatement(x: SeaStatement | SeaNativeAsyncStatement): x is SeaNativeAsyncStatement {
79+
return typeof (x as SeaNativeAsyncStatement).awaitResult === 'function';
80+
}
81+
7082
export default class SeaSessionBackend implements ISessionBackend {
7183
private readonly connection: SeaConnection;
7284

@@ -122,9 +134,9 @@ export default class SeaSessionBackend implements ISessionBackend {
122134
* Per-statement options forwarded to the kernel `ExecuteOptions`:
123135
* - `ordinalParameters` / `namedParameters` → bound params (mutually
124136
* exclusive — the kernel binds one placeholder style per statement);
125-
* - `queryTimeout` → enforced client-side by the operation backend's poll
126-
* deadline (the kernel ignores `queryTimeoutSecs` on the async submit
127-
* 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`;
128140
* - `rowLimit` → `rowLimit` (SEA-only server-side row cap);
129141
* - `queryTags` → serialised into the conf overlay's reserved
130142
* `query_tags` key (the same wire shape Thrift's `serializeQueryTags`
@@ -164,49 +176,72 @@ export default class SeaSessionBackend implements ISessionBackend {
164176
// JSDoc in `IDBSQLSession` for the cross-backend contract.
165177
//
166178
// - DEFAULT (`runAsync` false/undefined) — SYNC. Route through
167-
// `executeStatementCancellable`: the kernel blocks on `execute()`
168-
// (server-side direct-results / poll-to-terminal), which is faster and,
169-
// with the napi sync canceller, fully cancellable mid-COMPUTE. The
170-
// blocking drive runs in the operation backend's `result()` (inside
171-
// `waitUntilReady`, which the facade invokes lazily at first fetch).
172-
// `queryTimeoutSecs` IS honoured on this path (forwarded to the napi
173-
// options below) since the kernel `execute()` consults it.
179+
// `executeStatementDirect` (the directResults model): the kernel sends
180+
// ExecuteStatement with the server's inline wait and returns WITHOUT
181+
// polling past it — a terminal `Statement` (result inline) for a fast
182+
// query, or a still-running `AsyncStatement` (poll/cancel handle) for a
183+
// slow one. The handle is server-owned, so a long query stays cancellable
184+
// via `op.cancel()` and `close()` is a clean release (no client-side
185+
// drive-to-terminal). `queryTimeout` is a no-op here (see the note
186+
// below) — it is NOT mapped to the server `wait_timeout`.
174187
//
175188
// - `runAsync: true` — ASYNC. Submit (`wait_timeout=0s`): the server
176189
// returns a pending `AsyncStatement` immediately while the query runs;
177190
// the backend polls `status()` to terminal in `waitUntilReady()` and
178-
// materialises results via `awaitResult()`. `queryTimeoutSecs` is
179-
// ignored by the kernel on submit, so it is enforced client-side by the
180-
// operation backend's poll-loop deadline instead.
191+
// materialises results via `awaitResult()`. `queryTimeout` is a no-op
192+
// here too.
181193
const runAsync = options.runAsync ?? false;
182-
const queryTimeoutSecs =
183-
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.
184204

185205
if (!runAsync) {
186-
// Sync path: forward `queryTimeoutSecs` to the napi options — the kernel
187-
// `execute()` honours it (server statement timeout).
188-
const execOptions = this.buildExecuteOptions(options, queryTimeoutSecs);
189-
let cancellableExecution;
206+
// DEFAULT — directResults (the Thrift/JDBC model). The kernel's
207+
// `executeStatementDirect` runs ExecuteStatement with a bounded server
208+
// inline wait and returns WITHOUT polling past it:
209+
// - a terminal `Statement` (result inline) for a fast query, or
210+
// - an `AsyncStatement` (a poll/cancel handle) for a slow one.
211+
// Either way the handle is tied to a server-owned statement, so a long
212+
// query stays cancellable via `op.cancel()` and `close()` is a clean
213+
// release (no client-side drive-to-terminal). Fire-and-forget DDL/DML
214+
// commits because the server runs it inline during the POST.
215+
const execOptions = this.buildExecuteOptions(options);
216+
let direct: SeaStatement | SeaNativeAsyncStatement;
190217
try {
191-
cancellableExecution =
218+
direct =
192219
execOptions === undefined
193-
? await this.connection.executeStatementCancellable(statement)
194-
: await this.connection.executeStatementCancellable(statement, execOptions);
220+
? await this.connection.executeStatementDirect(statement)
221+
: await this.connection.executeStatementDirect(statement, execOptions);
195222
} catch (err) {
196223
throw this.logAndMapError('executeStatement', err);
197224
}
198-
return new SeaOperationBackend({
199-
cancellableExecution: cancellableExecution!,
200-
context: this.context,
201-
// The kernel honours `queryTimeoutSecs` on the sync `execute` path, so
202-
// it is forwarded via the napi options (see `buildExecuteOptions`); the
203-
// backend also keeps it as a deadline guard for parity with async.
204-
queryTimeoutSecs,
205-
});
225+
// The kernel contract (`Promise<Statement | AsyncStatement>`) never yields
226+
// null/undefined; reject a non-handle up front so a contract violation
227+
// surfaces as a mapped driver error here rather than an opaque `TypeError`
228+
// deferred to first fetch/close.
229+
if (direct === null || direct === undefined) {
230+
throw this.logAndMapError(
231+
'executeStatement',
232+
new HiveDriverError('SEA executeStatementDirect returned no statement handle'),
233+
);
234+
}
235+
// Feature-detect the arm via a type guard: only the Running `AsyncStatement`
236+
// exposes `awaitResult`; the terminal `Statement` (Completed arm) does not.
237+
if (isSeaAsyncStatement(direct)) {
238+
return new SeaOperationBackend({ asyncStatement: direct, context: this.context });
239+
}
240+
return new SeaOperationBackend({ statement: direct, context: this.context });
206241
}
207242

208-
// Async path: do NOT forward `queryTimeoutSecs` (the kernel ignores it on
209-
// 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.)
210245
const execOptions = this.buildExecuteOptions(options);
211246
let asyncStatement;
212247
try {
@@ -217,16 +252,11 @@ export default class SeaSessionBackend implements ISessionBackend {
217252
} catch (err) {
218253
throw this.logAndMapError('executeStatement', err);
219254
}
220-
// `queryTimeout` is enforced client-side by the operation backend's poll
221-
// loop: the kernel ignores `queryTimeoutSecs` on the async submit path
222-
// (`submitStatement` always sends `wait_timeout=0s`), so we do NOT forward
223-
// 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.
224257
return new SeaOperationBackend({
225258
asyncStatement: asyncStatement!,
226259
context: this.context,
227-
// `queryTimeout` is typed `number | bigint | Int64`; `numberToInt64(...).toNumber()`
228-
// coerces all three (a bare `Number(int64)` yields NaN — node-int64 has no valueOf).
229-
queryTimeoutSecs,
230260
});
231261
}
232262

@@ -235,10 +265,7 @@ export default class SeaSessionBackend implements ISessionBackend {
235265
* `ExecuteOptions`, returning `undefined` when nothing is set so the
236266
* no-options call shape (`executeStatement(sql)`) is preserved.
237267
*/
238-
private buildExecuteOptions(
239-
options: ExecuteStatementOptions,
240-
queryTimeoutSecs?: number,
241-
): SeaNativeExecuteOptions | undefined {
268+
private buildExecuteOptions(options: ExecuteStatementOptions): SeaNativeExecuteOptions | undefined {
242269
// Positional (`?`) and named (`:name`) parameters are mutually exclusive —
243270
// the kernel binds one placeholder style per statement. Use the SAME error
244271
// type and message as the Thrift backend (`ThriftSessionBackend`) so a
@@ -256,14 +283,9 @@ export default class SeaSessionBackend implements ISessionBackend {
256283
if (namedParams !== undefined) {
257284
execOptions.namedParams = namedParams;
258285
}
259-
// `queryTimeoutSecs` is forwarded only on the SYNC path (the caller passes
260-
// it in): the kernel `execute()` consults it as the server statement
261-
// timeout. On the async submit path the caller omits it (the kernel ignores
262-
// it under `wait_timeout=0s`), so it is enforced client-side by the
263-
// operation backend's poll-loop deadline instead (see executeStatement).
264-
if (queryTimeoutSecs !== undefined) {
265-
execOptions.queryTimeoutSecs = queryTimeoutSecs;
266-
}
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`.
267289
if (options.rowLimit !== undefined) {
268290
execOptions.rowLimit = Number(options.rowLimit);
269291
}

0 commit comments

Comments
 (0)