Skip to content

Commit 980ac66

Browse files
committed
fix(sea): address review findings + bump KERNEL_REV to kernel main; prettier
Review findings (GopalDB / code-review agent), each validated: 1. DBSQLParameter DATE off-by-one in non-UTC timezones — `toISOString()` converts to UTC before slicing, so a local-constructed Date shifts a day in offset zones. Use local calendar accessors (getFullYear/getMonth/getDate). Validated end-to-end on a live SEA warehouse under TZ=Australia/Sydney: a `new Date(2024,2,14)` now round-trips as 2024-03-14 (was 2024-03-13). Regression test sabotages toISOString so it guards in any timezone (incl. the UTC CI runner). 2. SeaOperationBackend.status() (sync path) could block indefinitely — it gated Succeeded on `fetchHandlePromise` *existing*, but that is assigned synchronously when getFetchHandle() is first called while result() may still be pending; status() would then report Succeeded early AND await the pending result() via readRichStatusFields(). Gate on `blockingStatement` (set only after result() resolves) instead. 3. Async-path progress callback omitted the rich-status fields on the terminal Succeeded tick (the sync path includes them) — parity break for DML numModifiedRows. Merge the rich fields into the Succeeded callback in waitUntilReadyAsync. Also: - KERNEL_REV -> 9c2e2378 (kernel main, #144 merged). Binding contract (native/sea/index.d.ts) already matched the bumped surface. - prettier --write (lint fix) + new regression tests for findings 1-3. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
1 parent 86ee26b commit 980ac66

6 files changed

Lines changed: 110 additions & 8 deletions

File tree

KERNEL_REV

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
75bfa526cf70c6c37d565e3cbb19c6d922714174
1+
9c2e2378f9a0bcee7d2750371392c07cac38fc3d

lib/DBSQLParameter.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,17 @@ export class DBSQLParameter {
137137
name,
138138
type: wireType ?? DBSQLParameterType.TIMESTAMP,
139139
value: new TSparkParameterValue({
140-
stringValue: isDateType ? this.value.toISOString().slice(0, 10) : this.value.toISOString(),
140+
// For DATE, project the *calendar* date using local-time accessors
141+
// rather than `toISOString().slice(0, 10)`. `toISOString()` first
142+
// converts to UTC, so a `new Date(2024, 2, 14)` constructed in a
143+
// positive-offset zone (e.g. UTC+10, internal `2024-03-13T14:00Z`)
144+
// would yield "2024-03-13" — off by one. Users reason about a DATE
145+
// as the wall-calendar date they constructed, so extract that.
146+
stringValue: isDateType
147+
? `${this.value.getFullYear()}-${String(this.value.getMonth() + 1).padStart(2, '0')}-${String(
148+
this.value.getDate(),
149+
).padStart(2, '0')}`
150+
: this.value.toISOString(),
141151
}),
142152
});
143153
}

lib/sea/SeaOperationBackend.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,15 @@ export default class SeaOperationBackend implements IOperationBackend {
406406
// server-side; there is no per-status RPC to query while it runs. Report
407407
// Running until `result()` has materialised the terminal statement, then
408408
// Succeeded — mirroring the kernel's blocking-then-terminal lifecycle.
409-
if (!this.fetchHandlePromise) {
409+
//
410+
// Gate on `blockingStatement`, NOT on `fetchHandlePromise`: the latter is
411+
// assigned synchronously when `getFetchHandle()` is first called (e.g. by
412+
// a concurrent fetch or `waitUntilReady`), while `result()` may still be
413+
// pending — `blockingStatement` is only set inside the resolve handler.
414+
// Using the promise's *existence* as a completion proxy would make a
415+
// concurrent `status()` poll both report Succeeded early AND block on the
416+
// pending `result()` via `readRichStatusFields()`.
417+
if (!this.blockingStatement) {
410418
return { state: OperationState.Running, hasResultSet: true };
411419
}
412420
// The blocking `result()` has resolved a terminal `Statement` — surface
@@ -587,8 +595,16 @@ export default class SeaOperationBackend implements IOperationBackend {
587595
const state = statusStringToOperationState(await this.asyncStatement!.status());
588596

589597
if (options?.callback) {
598+
// On the terminal Succeeded tick, carry the rich status fields
599+
// (numModifiedRows / displayMessage / diagnosticInfo / errorDetailsJson)
600+
// so the async path's progress callback matches the sync path
601+
// (`waitUntilReadyCancellable`). The kernel has populated the
602+
// AsyncStatement's accessors off the just-completed terminal poll, and
603+
// the read is memoised + does not force result materialisation.
604+
// eslint-disable-next-line no-await-in-loop
605+
const richFields = state === OperationState.Succeeded ? await this.readRichStatusFields() : {};
590606
// eslint-disable-next-line no-await-in-loop
591-
await Promise.resolve(options.callback({ state, hasResultSet: true }));
607+
await Promise.resolve(options.callback({ state, hasResultSet: true, ...richFields }));
592608
}
593609

594610
switch (state) {

tests/unit/DBSQLParameter.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,4 +168,22 @@ describe('DBSQLParameter', () => {
168168
}),
169169
);
170170
});
171+
172+
it('binds a typed DATE from local calendar accessors, never via toISOString (no UTC off-by-one)', () => {
173+
// A `Date` constructed from local components must bind the wall-calendar
174+
// date the user intended, regardless of the process timezone.
175+
// `toISOString()` converts to UTC first, so in a positive-offset zone a
176+
// local-midnight `new Date(2024, 2, 14)` (internally 2024-03-13T..Z) would
177+
// bind "2024-03-13" — off by one. Sabotage `toISOString` to PROVE the DATE
178+
// path doesn't use it: this guards the regression in any timezone, including
179+
// the UTC CI runner where the two formulations would otherwise agree.
180+
const localDate = new Date(2024, 2, 14);
181+
(localDate as unknown as { toISOString: () => string }).toISOString = () => '1999-12-31T00:00:00.000Z';
182+
expect(new DBSQLParameter({ type: DBSQLParameterType.DATE, value: localDate }).toSparkParameter()).to.deep.equal(
183+
new TSparkParameter({
184+
type: DBSQLParameterType.DATE,
185+
value: new TSparkParameterValue({ stringValue: '2024-03-14' }),
186+
}),
187+
);
188+
});
171189
});

tests/unit/sea/connectionOptions.test.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,10 @@ describe('SeaAuth HTTP options (buildSeaHttpOptions)', () => {
288288
];
289289
for (const [label, customHeaders] of injections) {
290290
it(`throws HiveDriverError on ${label}`, () => {
291-
expect(() => buildSeaHttpOptions(opts({ customHeaders }))).to.throw(HiveDriverError, /forbidden control character/);
291+
expect(() => buildSeaHttpOptions(opts({ customHeaders }))).to.throw(
292+
HiveDriverError,
293+
/forbidden control character/,
294+
);
292295
});
293296
}
294297

@@ -301,9 +304,10 @@ describe('SeaAuth HTTP options (buildSeaHttpOptions)', () => {
301304
it('validates a reserved header before dropping it (injection via Authorization is still rejected)', () => {
302305
// Reserved-name drop must not let a CR/LF-laced reserved header slip past
303306
// validation — validate first, then drop.
304-
expect(() =>
305-
buildSeaHttpOptions(opts({ customHeaders: { Authorization: 'Bearer x\r\nInjected: 1' } })),
306-
).to.throw(HiveDriverError, /forbidden control character/);
307+
expect(() => buildSeaHttpOptions(opts({ customHeaders: { Authorization: 'Bearer x\r\nInjected: 1' } }))).to.throw(
308+
HiveDriverError,
309+
/forbidden control character/,
310+
);
307311
});
308312
});
309313
});

tests/unit/sea/execution.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,16 @@ class FakeCancellableExecution {
232232
this.pendingResolve = undefined;
233233
}
234234
}
235+
236+
// Resolve a parked (blocked) result() with the terminal statement, modelling
237+
// the server-side blocking execute finally completing.
238+
public release(): void {
239+
if (this.pendingResolve) {
240+
this.pendingResolve(this.resultHandle);
241+
this.pendingResolve = undefined;
242+
this.pendingReject = undefined;
243+
}
244+
}
235245
}
236246

237247
class FakeNativeConnection implements SeaConnection {
@@ -1044,6 +1054,26 @@ describe('SeaOperationBackend — async (submitStatement) path', () => {
10441054
expect(states).to.deep.equal([OperationState.Pending, OperationState.Running, OperationState.Succeeded]);
10451055
});
10461056

1057+
it('progress callback carries rich-status fields on the terminal Succeeded tick (parity with the sync path)', async () => {
1058+
// Regression guard: the async poll loop must merge the rich-status fields
1059+
// into the callback on the Succeeded tick, matching waitUntilReadyCancellable.
1060+
// Previously the async path fired only { state, hasResultSet } even when
1061+
// Succeeded, so a DML's numModifiedRows never reached a progress callback.
1062+
const stmt = new FakeAsyncStatement(['Running', 'Succeeded']);
1063+
stmt.rich = { numModifiedRows: 13, displayMessage: 'UPDATE 0 13', diagnosticInfo: null, errorDetailsJson: null };
1064+
const op = makeAsyncOp(stmt);
1065+
const ticks: Array<{ state: OperationState; numModifiedRows?: number | null; displayMessage?: string | null }> = [];
1066+
await op.waitUntilReady({ callback: (s) => ticks.push(s) });
1067+
1068+
const succeeded = ticks.find((t) => t.state === OperationState.Succeeded);
1069+
expect(succeeded, 'a Succeeded tick fired').to.not.equal(undefined);
1070+
expect(succeeded?.numModifiedRows).to.equal(13);
1071+
expect(succeeded?.displayMessage).to.equal('UPDATE 0 13');
1072+
// Non-terminal ticks must NOT carry rich fields (only populated when terminal).
1073+
const running = ticks.find((t) => t.state === OperationState.Running);
1074+
expect(running?.numModifiedRows).to.equal(undefined);
1075+
});
1076+
10471077
it('waitUntilReady() surfaces the kernel error envelope on a Failed statement', async () => {
10481078
const stmt = new FakeAsyncStatement('Failed');
10491079
// The kernel rejects awaitResult() with a sentinel-framed structured error;
@@ -1195,6 +1225,30 @@ describe('SeaOperationBackend — sync (executeStatementCancellable) path', () =
11951225
expect((await op.status(false)).state).to.equal(OperationState.Succeeded);
11961226
});
11971227

1228+
it('status() stays Running and does NOT block while a concurrent result() is still in flight', async () => {
1229+
// Regression guard: status() must gate Succeeded on the *resolved* statement
1230+
// (blockingStatement), not on the mere existence of fetchHandlePromise.
1231+
// Once a concurrent path (here, waitUntilReady) has dispatched result(),
1232+
// fetchHandlePromise is set but still pending; the old `!fetchHandlePromise`
1233+
// gate would (a) report Succeeded early and (b) block on the pending
1234+
// result() via readRichStatusFields() -> getFetchHandle().
1235+
const exec = new FakeCancellableExecution();
1236+
exec.block = true; // result() parks until released
1237+
const op = makeSyncOp(exec);
1238+
const waitPromise = op.waitUntilReady(); // dispatches result() (parks); fetchHandlePromise now pending
1239+
await Promise.resolve(); // let result() get dispatched + parked
1240+
1241+
const status = (await Promise.race([
1242+
op.status(false),
1243+
new Promise((_, reject) => setTimeout(() => reject(new Error('status() blocked on the pending result()')), 2000)),
1244+
])) as { state: OperationState };
1245+
expect(status.state).to.equal(OperationState.Running);
1246+
1247+
exec.release(); // blocking execute completes
1248+
await waitPromise;
1249+
expect((await op.status(false)).state).to.equal(OperationState.Succeeded);
1250+
});
1251+
11981252
it('waitUntilReady() drives result() to the terminal statement and fires the callback once', async () => {
11991253
const exec = new FakeCancellableExecution();
12001254
const op = makeSyncOp(exec);

0 commit comments

Comments
 (0)