Skip to content

Commit c1d66dc

Browse files
committed
[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>
1 parent bb749d6 commit c1d66dc

2 files changed

Lines changed: 79 additions & 19 deletions

File tree

lib/sea/SeaSessionBackend.ts

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,19 @@ export interface SeaSessionBackendOptions {
6767
* needed — that pattern was removed when the napi binding moved these
6868
* onto `openSession` to match pyo3.
6969
*/
70+
71+
/**
72+
* Narrow the directResults union (`executeStatementDirect`'s
73+
* `Statement | AsyncStatement`) to the Running `AsyncStatement` arm. Only that
74+
* arm exposes `awaitResult`; the terminal `Statement` (Completed arm) does not.
75+
* Mirrors the kernel `DirectStatement::{Completed, Running}` discriminant, which
76+
* the opaque napi classes can't carry on the wire — the `awaitResult` probe is
77+
* the load-bearing feature-detect (see databricks-sql-kernel#140).
78+
*/
79+
function isSeaAsyncStatement(x: SeaStatement | SeaNativeAsyncStatement): x is SeaNativeAsyncStatement {
80+
return typeof (x as SeaNativeAsyncStatement).awaitResult === 'function';
81+
}
82+
7083
export default class SeaSessionBackend implements ISessionBackend {
7184
private readonly connection: SeaConnection;
7285

@@ -164,13 +177,16 @@ export default class SeaSessionBackend implements ISessionBackend {
164177
// JSDoc in `IDBSQLSession` for the cross-backend contract.
165178
//
166179
// - 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.
180+
// `executeStatementDirect` (the directResults model): the kernel sends
181+
// ExecuteStatement with the server's inline wait and returns WITHOUT
182+
// polling past it — a terminal `Statement` (result inline) for a fast
183+
// query, or a still-running `AsyncStatement` (poll/cancel handle) for a
184+
// slow one. The handle is server-owned, so a long query stays cancellable
185+
// 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.
174190
//
175191
// - `runAsync: true` — ASYNC. Submit (`wait_timeout=0s`): the server
176192
// returns a pending `AsyncStatement` immediately while the query runs;
@@ -193,7 +209,7 @@ export default class SeaSessionBackend implements ISessionBackend {
193209
// release (no client-side drive-to-terminal). Fire-and-forget DDL/DML
194210
// commits because the server runs it inline during the POST.
195211
const execOptions = this.buildExecuteOptions(options, queryTimeoutSecs);
196-
let direct;
212+
let direct: SeaStatement | SeaNativeAsyncStatement;
197213
try {
198214
direct =
199215
execOptions === undefined
@@ -202,17 +218,22 @@ export default class SeaSessionBackend implements ISessionBackend {
202218
} catch (err) {
203219
throw this.logAndMapError('executeStatement', err);
204220
}
205-
// Feature-detect the arm: only `AsyncStatement` (the Running arm) exposes
206-
// `awaitResult`; the terminal `Statement` (Completed arm) does not.
207-
const asAsync = direct as unknown as SeaNativeAsyncStatement;
208-
if (direct !== null && typeof asAsync.awaitResult === 'function') {
209-
return new SeaOperationBackend({ asyncStatement: asAsync, context: this.context, queryTimeoutSecs });
221+
// The kernel contract (`Promise<Statement | AsyncStatement>`) never yields
222+
// null/undefined; reject a non-handle up front so a contract violation
223+
// surfaces as a mapped driver error here rather than an opaque `TypeError`
224+
// deferred to first fetch/close.
225+
if (direct === null || direct === undefined) {
226+
throw this.logAndMapError(
227+
'executeStatement',
228+
new HiveDriverError('SEA executeStatementDirect returned no statement handle'),
229+
);
230+
}
231+
// Feature-detect the arm via a type guard: only the Running `AsyncStatement`
232+
// exposes `awaitResult`; the terminal `Statement` (Completed arm) does not.
233+
if (isSeaAsyncStatement(direct)) {
234+
return new SeaOperationBackend({ asyncStatement: direct, context: this.context, queryTimeoutSecs });
210235
}
211-
return new SeaOperationBackend({
212-
statement: direct as unknown as SeaStatement,
213-
context: this.context,
214-
queryTimeoutSecs,
215-
});
236+
return new SeaOperationBackend({ statement: direct, context: this.context, queryTimeoutSecs });
216237
}
217238

218239
// Async path: do NOT forward `queryTimeoutSecs` (the kernel ignores it on

tests/unit/sea/execution.test.ts

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,45 @@ describe('SeaSessionBackend', () => {
567567
expect(op.id).to.be.a('string').and.have.length.greaterThan(0);
568568
});
569569

570+
it('executeStatement (sync default) routes a still-running query through the AsyncStatement arm', async () => {
571+
// The directResults Running arm — a query that did NOT finish within the
572+
// server inline wait comes back as an AsyncStatement (poll/cancel handle).
573+
// This is the branch the whole PR exists to add (Node mid-run cancel).
574+
const connection = new FakeNativeConnection();
575+
connection.directReturnsRunning = true;
576+
const session = makeSession(connection);
577+
const op = await session.executeStatement('SELECT slow', {});
578+
expect(op).to.be.instanceOf(SeaOperationBackend);
579+
// The Running arm was taken: an AsyncStatement was constructed + wired
580+
// (not the terminal `statement` arm).
581+
expect(connection.lastAsyncStatement, 'AsyncStatement (Running) arm should be taken').to.not.equal(undefined);
582+
// Driving the op polls the async handle's status() — the polling arm.
583+
await op.waitUntilReady();
584+
expect(connection.lastAsyncStatement!.statusCalls, 'async handle polled via status()').to.be.greaterThan(0);
585+
});
586+
587+
it('executeStatement (sync default) AsyncStatement arm: op.cancel() reaches the running statement', async () => {
588+
// The point of directResults on single-threaded Node: the returned op holds
589+
// a handle to the still-running statement, so op.cancel() can abort it.
590+
const connection = new FakeNativeConnection();
591+
connection.directReturnsRunning = true;
592+
const session = makeSession(connection);
593+
const op = await session.executeStatement('SELECT slow', {});
594+
await op.cancel();
595+
expect(connection.lastAsyncStatement!.cancelled, 'cancel reaches the running statement').to.equal(true);
596+
});
597+
598+
it('executeStatement (sync default) routes a fast query through the terminal Statement arm', async () => {
599+
// Contrast: a query that finished within the inline wait comes back as a
600+
// terminal Statement (result inline) — no AsyncStatement is created.
601+
const connection = new FakeNativeConnection(); // directReturnsRunning = false (default)
602+
const session = makeSession(connection);
603+
const op = await session.executeStatement('SELECT 1', {});
604+
expect(connection.lastAsyncStatement, 'no AsyncStatement arm for a terminal query').to.equal(undefined);
605+
await op.cancel();
606+
expect(connection.statementToReturn.cancelled, 'cancel reaches the terminal statement').to.equal(true);
607+
});
608+
570609
it('executeStatement forwards ordinalParameters as napi positionalParams', async () => {
571610
const connection = new FakeNativeConnection();
572611
const session = makeSession(connection);
@@ -696,7 +735,7 @@ describe('SeaSessionBackend', () => {
696735
const envelope = `__databricks_error__:${JSON.stringify({ code: 'SqlError', message: 'SUBMIT_BOOM' })}`;
697736
for (const opts of [{}, { runAsync: true }]) {
698737
const connection = new FakeNativeConnection();
699-
connection.throwOnExecute = new Error(envelope); // fails executeStatementCancellable / submitStatement
738+
connection.throwOnExecute = new Error(envelope); // fails executeStatementDirect / submitStatement
700739
const session = makeSession(connection);
701740
let thrown: unknown;
702741
try {

0 commit comments

Comments
 (0)