Skip to content

Commit 29cc499

Browse files
committed
feat(sea): surface operation-status fields (numModifiedRows, displayMessage, diagnosticInfo, errorDetailsJson)
Ports the async rich-status work (was #422) onto the consolidated branch: the napi Statement.status() fields the kernel already exposes are now surfaced through getOperationStatus instead of a flat Succeeded (M1 item). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
1 parent a43e20c commit 29cc499

4 files changed

Lines changed: 197 additions & 20 deletions

File tree

lib/contracts/OperationStatus.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,4 +53,33 @@ export interface OperationStatus {
5353
* to `WaitUntilReadyOptions.callback` for the consumer to interpret.
5454
*/
5555
progressUpdateResponse?: unknown;
56+
57+
/**
58+
* Number of rows modified by a DML statement (UPDATE / INSERT / DELETE /
59+
* MERGE). `undefined`/`null` for SELECT and on backends/warehouses that do
60+
* not surface the counter. Mirrors Thrift's
61+
* `TGetOperationStatusResp.numModifiedRows`.
62+
*/
63+
numModifiedRows?: number | null;
64+
65+
/**
66+
* Server-supplied user-facing message, when the backend exposes one. Mirrors
67+
* Thrift's `TGetOperationStatusResp.displayMessage`. May contain SQL
68+
* fragments or parameter values — treat as potentially sensitive.
69+
*/
70+
displayMessage?: string | null;
71+
72+
/**
73+
* Server-supplied diagnostic detail (multi-line operator / stack context),
74+
* when available. Mirrors Thrift's `TGetOperationStatusResp.diagnosticInfo`.
75+
* For support surfaces, not user-facing.
76+
*/
77+
diagnosticInfo?: string | null;
78+
79+
/**
80+
* Server-supplied JSON blob with extended error details, when available.
81+
* Mirrors Thrift's `TGetOperationStatusResp.errorDetailsJson`. Pass-through
82+
* string — callers parse with `JSON.parse` if they need structured access.
83+
*/
84+
errorDetailsJson?: string | null;
5685
}

lib/sea/SeaOperationBackend.ts

Lines changed: 129 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,33 @@ export type SeaOperationStatement = SeaStatementHandle & Partial<SeaStatement>;
8585
*/
8686
type SeaFetchHandle = Pick<SeaStatement, 'fetchNextBatch' | 'schema'>;
8787

88+
/**
89+
* The rich operation-status surface the kernel exposes on a terminal sync
90+
* `Statement` (`numModifiedRows` / `displayMessage` / `diagnosticInfo` /
91+
* `errorDetailsJson`). These accessors live ONLY on the blocking `Statement`
92+
* (metadata path + sync `runAsync:false` path once `result()` resolves) — the
93+
* async `AsyncStatement` / `AsyncResultHandle` do not expose them — so the
94+
* reader below is best-effort and returns an empty record when the handle
95+
* predates this surface or the operation never resolved to a `Statement`.
96+
*/
97+
type SeaStatusFieldsHandle = Pick<
98+
SeaStatement,
99+
'numModifiedRows' | 'displayMessage' | 'diagnosticInfo' | 'errorDetailsJson'
100+
>;
101+
102+
/**
103+
* The rich operation-status fields, as the kernel returns them (each `null`
104+
* when the server didn't supply it — e.g. `numModifiedRows` is null for a
105+
* SELECT). Carried onto the neutral `OperationStatus` and ultimately into the
106+
* Thrift `TGetOperationStatusResp` so SEA reports parity with the Thrift path.
107+
*/
108+
interface SeaRichStatusFields {
109+
numModifiedRows: number | null;
110+
displayMessage: string | null;
111+
diagnosticInfo: string | null;
112+
errorDetailsJson: string | null;
113+
}
114+
88115
/** Poll cadence for the async `status()` loop — matches the Thrift backend's 100ms. */
89116
const STATUS_POLL_INTERVAL_MS = 100;
90117

@@ -377,7 +404,9 @@ export default class SeaOperationBackend implements IOperationBackend {
377404
if (this.asyncStatement) {
378405
// Async query path: report the real kernel state (single
379406
// GetStatementStatus RPC — no polling here; `waitUntilReady` owns the
380-
// poll loop).
407+
// poll loop). The rich status fields (`numModifiedRows` etc.) live on the
408+
// terminal sync `Statement`, which the async path never produces, so they
409+
// stay undefined here.
381410
const state = statusStringToOperationState(await this.asyncStatement.status());
382411
return { state, hasResultSet: true };
383412
}
@@ -386,11 +415,16 @@ export default class SeaOperationBackend implements IOperationBackend {
386415
// server-side; there is no per-status RPC to query while it runs. Report
387416
// Running until `result()` has materialised the terminal statement, then
388417
// Succeeded — mirroring the kernel's blocking-then-terminal lifecycle.
389-
const state = this.fetchHandlePromise ? OperationState.Succeeded : OperationState.Running;
390-
return { state, hasResultSet: true };
418+
if (!this.fetchHandlePromise) {
419+
return { state: OperationState.Running, hasResultSet: true };
420+
}
421+
// The blocking `result()` has resolved a terminal `Statement` — surface
422+
// its rich status fields alongside the Succeeded state.
423+
return { state: OperationState.Succeeded, hasResultSet: true, ...(await this.readRichStatusFields()) };
391424
}
392-
// Metadata path: the kernel statement is already terminal.
393-
return { state: OperationState.Succeeded, hasResultSet: true };
425+
// Metadata path: the kernel statement is already terminal — read its rich
426+
// fields too (they are `null` for metadata results, by design).
427+
return { state: OperationState.Succeeded, hasResultSet: true, ...(await this.readRichStatusFields()) };
394428
}
395429

396430
public async waitUntilReady(options?: IOperationBackendWaitOptions): Promise<void> {
@@ -402,8 +436,11 @@ export default class SeaOperationBackend implements IOperationBackend {
402436
}
403437
// Metadata path: the kernel statement has already resolved, so there is
404438
// nothing to poll. seaFinished fires the progress callback once with a
405-
// synthesised completion tick, matching the Thrift path's final tick.
406-
return seaFinished(this.lifecycle, options);
439+
// synthesised completion tick, matching the Thrift path's final tick. The
440+
// rich-field reader is passed lazily so it only runs when a callback is
441+
// wired (metadata statements report all-null, but the surface stays
442+
// consistent with the query paths).
443+
return seaFinished(this.lifecycle, options, () => this.readRichStatusFields());
407444
}
408445

409446
public async cancel(): Promise<Status> {
@@ -418,6 +455,85 @@ export default class SeaOperationBackend implements IOperationBackend {
418455
// Internals.
419456
// ---------------------------------------------------------------------------
420457

458+
/**
459+
* Read the kernel's rich operation-status fields (`numModifiedRows` /
460+
* `displayMessage` / `diagnosticInfo` / `errorDetailsJson`) off the terminal
461+
* sync `Statement`. These accessors live only on the blocking `Statement`
462+
* (metadata path, or the sync `runAsync:false` path once `result()` has
463+
* resolved) — not on the async `AsyncStatement` / `AsyncResultHandle` — so:
464+
*
465+
* - on the async path we have no `Statement`, so we return all-null;
466+
* - on the sync path we await `getFetchHandle()` first, which both drives
467+
* `result()` to completion and stores the resolved `Statement` on
468+
* `blockingStatement` (the handle that backs the accessors);
469+
* - if the (older) binding predates these accessors we degrade to all-null
470+
* rather than throwing — `getOperationStatus()` must never fail just
471+
* because the rich fields are unavailable.
472+
*
473+
* Errors from the individual accessors are swallowed to null: a failed
474+
* status-field read must not turn a successful operation's status query into
475+
* a throw. The fields are best-effort metadata, not the operation outcome.
476+
*/
477+
private async readRichStatusFields(): Promise<SeaRichStatusFields> {
478+
const empty: SeaRichStatusFields = {
479+
numModifiedRows: null,
480+
displayMessage: null,
481+
diagnosticInfo: null,
482+
errorDetailsJson: null,
483+
};
484+
485+
// The async path never produces a terminal sync `Statement`, so there is
486+
// nothing to read these off of.
487+
if (this.asyncStatement && !this.cancellableExecution) {
488+
return empty;
489+
}
490+
491+
// Ensure the sync path's blocking `result()` has resolved and stored the
492+
// terminal `Statement` on `blockingStatement` (no-op on the metadata path,
493+
// where `blockingStatement` was set at construction).
494+
if (this.cancellableExecution) {
495+
try {
496+
await this.getFetchHandle();
497+
} catch {
498+
// The operation failed/cancelled — its outcome surfaces through the
499+
// wait/fetch path; status-field reads have nothing to add.
500+
return empty;
501+
}
502+
}
503+
504+
const handle = this.blockingStatement as Partial<SeaStatusFieldsHandle> | undefined;
505+
if (!handle || typeof handle.numModifiedRows !== 'function') {
506+
// No resolved statement, or a binding that predates the rich-field
507+
// accessors — degrade to all-null.
508+
return empty;
509+
}
510+
const richHandle = handle as SeaStatusFieldsHandle;
511+
512+
const readOrNull = async <T>(read: () => Promise<T | null>): Promise<T | null> => {
513+
try {
514+
return await read();
515+
} catch (err) {
516+
this.context
517+
.getLogger()
518+
.log(
519+
LogLevel.debug,
520+
`SEA status-field read failed for operation ${this._id}; reporting null. Cause: ` +
521+
`${err instanceof Error ? err.message : String(err)}`,
522+
);
523+
return null;
524+
}
525+
};
526+
527+
const [numModifiedRows, displayMessage, diagnosticInfo, errorDetailsJson] = await Promise.all([
528+
readOrNull(() => richHandle.numModifiedRows()),
529+
readOrNull(() => richHandle.displayMessage()),
530+
readOrNull(() => richHandle.diagnosticInfo()),
531+
readOrNull(() => richHandle.errorDetailsJson()),
532+
]);
533+
534+
return { numModifiedRows, displayMessage, diagnosticInfo, errorDetailsJson };
535+
}
536+
421537
/**
422538
* Poll the kernel `AsyncStatement` to a terminal state on a fixed 100ms
423539
* cadence, mirroring the Thrift backend's `waitUntilReady` loop. We poll
@@ -547,9 +663,13 @@ export default class SeaOperationBackend implements IOperationBackend {
547663
// `getFetchHandle()` drives `result()` and memoises the resolved Statement
548664
// (also stored on `blockingStatement` so `close()` can reach it).
549665
await this.getFetchHandle();
550-
// Single completion tick, matching the metadata path.
666+
// Single completion tick, matching the metadata path — carrying the rich
667+
// status fields (numModifiedRows etc.) read off the now-terminal Statement.
551668
if (options?.callback) {
552-
await Promise.resolve(options.callback({ state: OperationState.Succeeded, hasResultSet: true }));
669+
const richFields = await this.readRichStatusFields();
670+
await Promise.resolve(
671+
options.callback({ state: OperationState.Succeeded, hasResultSet: true, ...richFields }),
672+
);
553673
}
554674
}
555675

lib/sea/SeaOperationLifecycle.ts

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -185,18 +185,21 @@ export async function seaClose(
185185
}
186186

187187
/**
188-
* Synthesize a neutral {@link OperationStatus} reporting the "finished"
189-
* state. `IOperationBackend.waitUntilReady` is backend-neutral surface — its
188+
* Synthesize an {@link OperationStatus} reporting the "finished" state.
189+
* `IOperationBackend.waitUntilReady` is backend-neutral surface — its
190190
* `callback` receives an {@link OperationStatus}, not a Thrift wire struct
191191
* (the public Thrift-shaped `OperationStatusCallback` is adapted at the
192-
* `DBSQLOperation` facade boundary). For M0 we report `Succeeded`. Richer
193-
* fields (`numModifiedRows`, `progressUpdateResponse`, `errorMessage`) defer
194-
* to M1 per the operation feature plan.
192+
* `DBSQLOperation` facade boundary). We report `Succeeded`, and merge in any
193+
* rich status fields (`numModifiedRows` / `displayMessage` / `diagnosticInfo`
194+
* / `errorDetailsJson`) the backend resolved off the terminal kernel
195+
* statement, so a `finished({callback})` consumer sees the same surface as a
196+
* subsequent `getOperationStatus()` call.
195197
*/
196-
function synthesizeFinishedStatus(): OperationStatus {
198+
function synthesizeFinishedStatus(extra?: Partial<OperationStatus>): OperationStatus {
197199
return {
198200
state: OperationState.Succeeded,
199201
hasResultSet: true,
202+
...extra,
200203
};
201204
}
202205

@@ -227,13 +230,19 @@ export async function seaFinished(
227230
progress?: boolean;
228231
callback?: (status: OperationStatus) => unknown;
229232
},
233+
// Rich status fields the backend read off the terminal statement, merged into
234+
// the synthesised completion tick so callback consumers see them. Lazy (a
235+
// thunk) so the (potentially RPC-backed) read only happens when a callback is
236+
// actually wired.
237+
richFields?: () => Promise<Partial<OperationStatus>>,
230238
): Promise<void> {
231239
if (state.isCancelled || state.isClosed) {
232240
return;
233241
}
234242

235243
if (options?.callback) {
236-
const response = synthesizeFinishedStatus();
244+
const extra = richFields ? await richFields() : undefined;
245+
const response = synthesizeFinishedStatus(extra);
237246
// Await the callback in case it returns a promise — matches the
238247
// Thrift code path at `lib/DBSQLOperation.ts:348-351`.
239248
await Promise.resolve(options.callback(response));

lib/thrift-backend/wireSynthesis.ts

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import Int64 from 'node-int64';
12
import {
23
TGetOperationStatusResp,
34
TGetResultSetMetadataResp,
@@ -77,10 +78,17 @@ function resultFormatToThrift(format: ResultFormat): TSparkRowSetType {
7778
* `OperationStatus` DTO. Used by `DBSQLOperation.status()` when running
7879
* against a non-Thrift backend (e.g. SEA) so the public API stays Thrift-shaped.
7980
*
80-
* Lossy by design: Thrift-only fields not carried by `OperationStatus`
81-
* (`taskStatus`, `numModifiedRows`, `operationStarted`, `operationCompleted`,
82-
* `displayMessage`, `diagnosticInfo`) are left undefined. Consumers that
83-
* read those fields will see `undefined` on non-Thrift backends.
81+
* Carries the rich status fields when the backend supplies them
82+
* (`numModifiedRows`, `displayMessage`, `diagnosticInfo`, `errorDetailsJson`)
83+
* — the SEA backend reads these off the terminal kernel statement, so DML
84+
* operations report `numModifiedRows` at parity with the Thrift path.
85+
* `numModifiedRows` is re-boxed as a Thrift `Int64` (`node-int64`) to match the
86+
* wire shape the Thrift deserializer produces, so consumers can read it
87+
* uniformly across backends.
88+
*
89+
* Still lossy for Thrift-only fields not carried by `OperationStatus`
90+
* (`taskStatus`, `operationStarted`, `operationCompleted`), which are left
91+
* undefined.
8492
*/
8593
export function synthesizeThriftStatus(status: OperationStatus): TGetOperationStatusResp {
8694
return {
@@ -90,6 +98,17 @@ export function synthesizeThriftStatus(status: OperationStatus): TGetOperationSt
9098
errorMessage: status.errorMessage,
9199
hasResultSet: status.hasResultSet,
92100
progressUpdateResponse: status.progressUpdateResponse as TGetOperationStatusResp['progressUpdateResponse'],
101+
// Rich status fields: only present on backends that surface them (SEA on a
102+
// terminal sync statement). `null` (server didn't supply) maps to
103+
// `undefined` so the synthesized response matches the Thrift path, where an
104+
// absent field is simply not set.
105+
numModifiedRows:
106+
status.numModifiedRows === undefined || status.numModifiedRows === null
107+
? undefined
108+
: new Int64(status.numModifiedRows),
109+
displayMessage: status.displayMessage ?? undefined,
110+
diagnosticInfo: status.diagnosticInfo ?? undefined,
111+
errorDetailsJson: status.errorDetailsJson ?? undefined,
93112
} as TGetOperationStatusResp;
94113
}
95114

0 commit comments

Comments
 (0)