[SEA-NodeJS] Kernel backend: connection/statement options, TLS & configurable sync/async execution#416
Open
msrathore-db wants to merge 8 commits into
Open
[SEA-NodeJS] Kernel backend: connection/statement options, TLS & configurable sync/async execution#416msrathore-db wants to merge 8 commits into
msrathore-db wants to merge 8 commits into
Conversation
Wire the SEA connection-level and per-statement option surfaces onto the merged-kernel napi binding (thin forwarding — the kernel owns the behaviour): Connection options (SeaAuth.buildSeaConnectionOptions): - `maxConnections` → kernel pool size, validated as a positive integer within the napi u32 range. - TLS: `checkServerCertificate` (secure-by-default — omit to keep the kernel's verify-on default; `false` opts into insecure) and `customCaCert` (PEM string or Buffer; strings are PEM-sanity-checked and normalised to a Buffer before the FFI boundary), via the new `buildSeaTlsOptions`. - `intervalsAsString: true` is always set so SEA interval/duration columns render as strings — a byte-compatible drop-in for the Thrift backend. `complexTypesAsJson` is intentionally left at the kernel default (native Arrow), which already decodes identically to Thrift via the shared converter. Statement options (SeaSessionBackend.executeStatement, via buildExecuteOptions): - `queryTimeout` → `queryTimeoutSecs`; `rowLimit` → `rowLimit` (SEA-only cap). - `queryTags` serialised JS-side (reusing Thrift's `serializeQueryTags`) into the reserved `query_tags` conf key, merged with any explicit `statementConf` — the napi `queryTags` field can't carry null-valued tags, and the kernel rejects setting both. `queryTags` / `queryTimeout` are no longer rejected. - Still rejected (genuinely unsupported on SEA): `useCloudFetch`, `useLZ4Compression`, `stagingAllowedLocalPath`. `rowLimit` / `statementConf` added to the public `ExecuteStatementOptions`; SEA-only knobs (`maxConnections` / `checkServerCertificate` / `customCaCert`) added to the internal `InternalConnectionOptions`. Validated against a live warehouse: secure-by-default connect, maxConnections, checkServerCertificate, rowLimit (caps rows), queryTimeout, queryTags, statementConf, and non-PEM customCaCert rejection. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Switch the SEA query path from the blocking `executeStatement` to the kernel's async `submitStatement`, matching the Thrift backend's always-async (`runAsync: true`) model. `submitStatement` returns immediately with a pending `AsyncStatement` (kernel `wait_timeout=0s`) while the query runs server-side. SeaOperationBackend becomes dual-mode (exactly one of): - `asyncStatement` (query path): `waitUntilReady()` polls `status()` to a terminal state on a 100ms cadence (matching Thrift), firing the progress callback each tick. Polling `status()` rather than blocking on `awaitResult()` keeps `cancel()` responsive — a blocking awaitResult would hold the kernel statement mutex for the whole query and queue cancel behind it. On Succeeded it materialises the result handle (first fetch is free); on Failed it drives `awaitResult()` to surface the kernel's typed SQL-error envelope; on a server-side Cancelled/Closed/Unknown it throws a clear error. `status()` reports the real Pending/Running/Succeeded state. - `statement` (metadata path): the kernel `list*`/`get*` statement is already terminal, so `waitUntilReady()` stays the one-shot completion tick. The fetch pipeline is shared: `awaitResult()`'s `AsyncResultHandle` and the metadata `Statement` expose the same `fetchNextBatch()` / `schema()` surface, so `SeaResultsProvider` → `ArrowResultConverter` → `ResultSlicer` consume either interchangeably via a single memoised fetch handle. cancel()/close() route through a `lifecycleHandle` abstraction over whichever handle backs the op. Re-exports the kernel `AsyncStatement` / `AsyncResultHandle` types from `SeaNativeLoader`. Validated against a live warehouse: async fetchAll correctness, multi-row drain (5000 rows), long-running aggregate (count over 20M), kernel SQL-error surfacing, and cancellation mid-execution. PR1's params/metadata/getInfo all still pass through the new async path. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
… code-style) The CI "Check code style" step runs `prettier . --check` (whole repo); these files were committed without prettier formatting. Formatting-only. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Code-review #413 (81/100). Validated each against the code + a live warehouse: - F1 (HIGH): the async poll loop threw plain HiveDriverError for server-driven Cancelled/Closed/Unknown. The DBSQLOperation facade only mirrors its cancelled/closed flags when `err instanceof OperationStateError` (and OperationStateError extends HiveDriverError, not the reverse), so a server-side cancel/close/admin-kill left the facade desynced. Now throws OperationStateError(Canceled/Closed/Unknown) — matching the Thrift backend. The Failed branch still surfaces the kernel SQL-error envelope via awaitResult. - F2 (MED): the server-Cancelled test asserted only instanceOf(HiveDriverError), which passes for both the correct and incorrect type — it couldn't catch F1. Now asserts instanceOf(OperationStateError) + errorCode, plus a new Closed test. - F3 (MED): queryTimeout was forwarded to submitStatement but the kernel ignores queryTimeoutSecs on submit (always wait_timeout=0s), so the documented public option was a silent no-op, and the poll loop had no client-side deadline (a stalled Running statement polled forever). Now enforced client-side: the poll loop tracks a deadline, best-effort cancels the statement on expiry, and throws OperationStateError(Timeout) — matching Thrift's server TIMEDOUT outcome. Stopped forwarding the ignored queryTimeoutSecs to the napi options. Validated live: a 2s timeout interrupts a slow cross-join with TIMEOUT. - F4 (LOW): customCaCert PEM string check now requires the END marker too (a truncated/headerless cert no longer passes), consistent with the Buffer path. - F5 (LOW): SeaAuth reads the SEA-only fields (checkServerCertificate / customCaCert / maxConnections) through `InternalConnectionOptions` instead of ad-hoc inline casts, so a typo'd key fails to compile. - F6 (LOW): corrected the poll-loop comment — the prior text justified polling by an incorrect "blocking awaitResult holds the mutex and queues cancel" claim; cancel() is documented lock-free. The real rationale (real-time status to the progress callback + cancel observed between ticks) is now stated. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Consolidates the last net-new bit of the superseded #408: two SEA-path DBSQLParameterType variants for binding timezone-explicit timestamps. The type name flows through the existing param codec (toSparkParameter → sqlType), which the kernel accepts — validated live (SELECT ? with TIMESTAMP_NTZ/LTZ returns the bound values). On the Thrift backend they degrade to a plain TIMESTAMP bind. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…ryTimeout coercion Code-review #413 (gopalldb). Two P1s: - TIMESTAMP_LTZ was sent verbatim on the wire, but Spark has no distinct TIMESTAMP_LTZ type (TIMESTAMP already carries LTZ semantics) — so a Thrift caller got an opaque server bind error, and the enum comment falsely claimed NTZ/LTZ "degrade to a plain TIMESTAMP bind" (there was no such logic). `toSparkParameter` now maps TIMESTAMP_LTZ → `TIMESTAMP` (valid on both Thrift and kernel); TIMESTAMP_NTZ stays native (a real Spark type). Comment corrected. Added DBSQLParameter tests for both wire types (the Thrift behaviour the review flagged as untested) and updated the kernel positional-params test. - queryTimeout (`number | bigint | Int64`) was coerced with `Number(...)`, which yields NaN for an Int64 (node-int64 has no valueOf) → the client-side deadline was silently disabled for Int64 inputs. Now uses `numberToInt64(...).toNumber()`, matching the Thrift backend. Added a regression test that an `Int64(1)` queryTimeout actually fires the deadline (OperationStateError(Timeout)) rather than polling forever. (P1 "queryTimeout silently dropped on submit" and the unbounded-poll note were already resolved earlier by the client-side deadline fix; doc comment updated to match. P2 polarity/Date-NTZ items noted for the public-surface follow-up.) Validated live: NTZ binds natively and LTZ binds as TIMESTAMP on the kernel path. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Collaborator
|
🟡 P1
🟢 P2
|
Make the SEA execution path user-configurable between sync and async,
toggled by the EXISTING `runAsync` option — no new public field, exactly
mirroring the Thrift backend's `runAsync` distinction. Default is SYNC
(`runAsync: false`): faster, and with the kernel sync canceller fully
cancellable mid-compute.
SeaSessionBackend.executeStatement now branches on `options.runAsync`:
- false/undefined (DEFAULT) -> Connection.executeStatementCancellable:
the kernel blocks on execute() (poll-to-terminal server-side), driven
lazily in the operation backend's result(). `queryTimeoutSecs` IS
forwarded (the kernel execute() honours it).
- true -> Connection.submitStatement (submit + poll), unchanged.
`queryTimeoutSecs` stays client-side (kernel ignores it on submit).
SeaOperationBackend gains a third dual-mode handle kind,
`cancellableExecution`, alongside `asyncStatement` and `statement`:
- waitUntilReadyCancellable drives result() to the terminal Statement
(memoised as the fetch handle + close target);
- the lifecycle handle is a composite: cancel() routes to the detached
canceller (lock-free, interrupts a running result() mid-COMPUTE and
is a no-op once terminal); close() routes to the resolved statement;
- a cancel-induced result() rejection maps to OperationStateError(
Canceled) so the DBSQLOperation facade mirrors its cancelled flag,
matching the Thrift path.
Public API, result shape, schema (TTableSchema), and error classes are
identical across both modes and to Thrift — the only observable
difference is lifecycle timing (when executeStatement resolves).
Built against databricks-sql-kernel napi
639e19ef97decc1c5aa2365c0b3a229c1ccd5b58 (executeStatementCancellable /
CancellableExecution); KERNEL_REV bumped to match. Refreshed the
committed binding surface (native/sea/index.{js,d.ts}).
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
On the sync (cancellable) execute path the operation id was a client UUID, because the server statement_id isn't known at construction — the kernel publishes it mid-`result()` once the initial execute round-trip returns. That left a cancelled/closed sync op untraceable against server/kernel logs (the async path already had the id from `submit`). `id` now prefers the server statement_id once known (captured from the resolved `Statement`, then the live canceller slot), falling back to the construction-time UUID until then. Updated the fake to model the real null-until-resolved `statementId` and assert op.id flips from UUID → server id after the execute completes. Validated live: SELECT 1 op.id is a UUID before fetch and the real `01f1…` statement id after. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Base
main. Recreation of #413 (which was mistakenly based on the #412 branch and merged there, never reachingmain), now extended with a configurable sync/async execution path.What's here
Connection / statement options + async (the original #413): TLS (
checkServerCertificatesecure-by-default +customCaCert),maxConnections,rowLimit,statementConf,queryTags, bound params (incl.TIMESTAMP_NTZ;TIMESTAMP_LTZ→TIMESTAMP), and the async submit/poll path. All earlier #413 review fixes carried over (server-driven Cancelled/Closed/Unknown →OperationStateError;queryTimeoutenforced client-side;Int64queryTimeout coercion; etc.).Configurable sync/async execution (new), toggled by the existing
runAsyncoption — no new public field:runAsyncfalse/undefined) → kernelConnection.executeStatementCancellable(blocking direct-results). Faster, and now cancellable. Matches the python connector (cursor.execute()→stmt.execute()).runAsync: true→submitStatement(submit + poll), unchanged.IOperation,fetchAll/fetchChunk/getSchema/getOperationStatus/cancel/close/hasMoreRows, result shape,TTableSchema, error classes). Only lifecycle timing differs — exactly Thrift'srunAsyncdistinction.Sync mid-compute cancellation:
cancel()on a sync op fires the detached kernelStatementCanceller(via #122's napi surface) so a still-running blocking execute is cancelled server-side; cancel-induced rejection maps toOperationStateError(Canceled). Mid-download cancel already works via the CloudFetch token.op.idnow surfaces the serverstatement_idon the sync path once the execute round-trip publishes it (was a client UUID), so cancelled/closed sync ops are traceable in server/kernel logs — parity with the async path.Verification
Known caveats (flagged for reviewers)
cancel()before the initial execute POST returns is a server-side no-op (the kernel publishes thestatement_idonly when that POST returns); the client still unblocks withOperationStateError(Canceled). Kernel-core property shared with pyo3; tracked as a kernel follow-up.InvalidArgument(the async path returnsCancelled); node normalises it toOperationStateError(Canceled)via the client-side cancel flag. Kernel follow-up.This pull request and its description were written by Isaac.