Skip to content

[SEA-NodeJS] (3/8) Kernel ErrorCode → JS error-class mapping (M0 minimum) - #377

Closed
msrathore-db wants to merge 11 commits into
msrathore/sea-napi-bindingfrom
msrathore/sea-errors-logging
Closed

[SEA-NodeJS] (3/8) Kernel ErrorCode → JS error-class mapping (M0 minimum)#377
msrathore-db wants to merge 11 commits into
msrathore/sea-napi-bindingfrom
msrathore/sea-errors-logging

Conversation

@msrathore-db

@msrathore-dbmsrathore-db commented May 16, 2026

Copy link
Copy Markdown
Contributor

Stack

Linear stack of 8 PRs landing the M0 + M1 Phase 1 SEA NodeJS work. Merge in order from base ↑ to tip. The tip branch (msrathore/sea-auth-u2m, PR #383) is the single snapshot containing everything in flight — point your test or benchmark harness at it for an end-to-end check.

PosPRBranchScope
1/8#378sea-abstractionIBackend / ISessionBackend / IOperationBackend interfaces
2/8#380sea-napi-bindingTS loader + build script for the kernel-provided .node artifact
3/8#377sea-errors-loggingKernel ErrorCode → JS error-class mapping (M0 minimum)
4/8#379sea-authPAT auth via useSEA: true
5/8#382sea-executionexecuteStatement + openSession (sessionConfig, initialCatalog/Schema)
6/8#381sea-resultsCloudFetch + Inline Arrow result fetching
7/8#384sea-operationcancel / close / finished lifecycle + INTERVAL parity + napi-relocation acceptance (absorbed sea-integration content)
8/8#383sea-auth-u2m← TIPM1 Phase 1 OAuth M2M + U2M (5 review rounds, ZERO HIGH at close)

Companion kernel stack (databricks/databricks-sql-kernel): 8 PRs — root #26 (async-public-api) → #27#25#29#28#30#24#23 (tip).

Policy: new PRs always stack on the current tip. No sibling/parallel topology. No force-pushes on existing PRs unless absolutely necessary; if a PR's content is wrong, add a fix-up commit on top of the stack tip rather than rewriting history.


This PR is position 3/8.

Summary

SeaErrorMapping.ts decodes the kernel's __databricks_error__:<json> napi error envelope into typed JS errors (AuthenticationError, NetworkError, OperationStateError, etc.). M0-minimum mapping covers 13 kernel ErrorCode variants.

The decoder gets significantly hardened later in #383 (M1 OAuth) — namespace under error.kernelMetadata.*, sentinel-strip on corrupted envelopes, type-guards — but the M0 baseline is here.

Draft until you give the go for review.

Downstream fixes / reviewer note

Tracking

Co-authored-by: Isaac

…kend
Refactors DBSQLClient/Session/Operation to dispatch through three
backend interfaces. ThriftBackend (lib/thrift-backend/) contains the
relocated existing thrift logic. SeaBackend (lib/sea/) is a stub for
M0; the sea-napi-binding feature wires the real impl.
Public surface (lib/index.ts) unchanged.
No new dependencies. All existing tests pass.
Files:
- lib/contracts/IBackend.ts (new)
- lib/contracts/ISessionBackend.ts (new)
- lib/contracts/IOperationBackend.ts (new)
- lib/contracts/IDBSQLClient.ts (adds useSEA?: boolean to ConnectionOptions)
- lib/thrift-backend/ThriftBackend.ts (new)
- lib/thrift-backend/ThriftSessionBackend.ts (new)
- lib/thrift-backend/ThriftOperationBackend.ts (new)
- lib/sea/SeaBackend.ts (new, M0 stub)
- lib/DBSQLClient.ts (dispatch through IBackend; useSEA picks SeaBackend)
- lib/DBSQLSession.ts (facade over ISessionBackend; staging stays here)
- lib/DBSQLOperation.ts (facade over IOperationBackend; iterators/fetchAll stay here)
- tests/unit/DBSQLClient.test.ts (retarget internal state lookup through backend; pre-seed client.backend in tests that bypass connect())
- tests/unit/DBSQLOperation.test.ts (retarget internal state lookup through backend)
…nline type
Addresses code-bloat-watchdog findings from commit 0085928:
- Restores public-API JSDoc on DBSQLSession + DBSQLOperation methods
(was deleted as scope creep; contracts unchanged so docs still apply)
- Adds makeStubbedClient() helper to tests/unit/DBSQLClient.test.ts;
replaces 14× duplicated ThriftBackend pre-seed
- Imports WaitUntilReadyOptions instead of inline option types in
IOperationBackend + DBSQLOperation.waitUntilReady
@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@msrathore-db
msrathore-dbforce-pushed the msrathore/sea-errors-logging branch from f7cdb80 to 8ccc8e2CompareMay 17, 2026 13:22
@msrathore-db
msrathore-db changed the base branch from main to msrathore/sea-abstractionMay 17, 2026 13:22
@msrathore-db
msrathore-db changed the base branch from msrathore/sea-abstraction to msrathore/sea-napi-bindingMay 17, 2026 13:22
@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Round-N fixes from the 9-reviewer pre-review. Public IOperation/DBSQLOperation
surface preserved byte-identical; backend interfaces (IBackend / ISessionBackend
/ IOperationBackend) made fully neutral so both Thrift and SEA can implement
the same contract.
F1 — neutral DTOs at IOperationBackend with Thrift-shape preservation on the
public facade (adapter pattern):
- lib/contracts/OperationStatus.ts (new) — neutral OperationStatus + OperationState
enum mirroring databricks-sql-python's CommandState and kernel pyo3's
StatementStatus taxonomy.
- lib/contracts/ResultMetadata.ts (new) — neutral ResultMetadata + ResultFormat
enum mirroring the three TSparkRowSetType cases.
- IOperationBackend.status()/getResultMetadata() return the neutral DTOs.
- ThriftOperationBackend.status() adapts at the boundary via adaptOperationStatus
/ adaptResultMetadata; module-level helpers thriftStateToOperationState and
thriftRowSetTypeToResultFormat do the enum maps.
- ThriftOperationBackend exposes thriftStatusResponse() and
thriftResultMetadataResponse() as public Thrift-only accessors used by the
facade's zero-loss fast path (kept for internal state-machine + result-handler
dispatch as well).
- lib/utils/thriftWireSynthesis.ts (new) — synthesizeThriftStatus and
synthesizeThriftResultSetMetadata: convert neutral DTOs back to Thrift wire
shape for the non-Thrift backend path. Lossy on Thrift-only fields
(taskStatus, numModifiedRows, cacheLookupResult, etc.).
- DBSQLOperation.status() and getMetadata() preserved Thrift return shape:
Thrift backend path returns the real wire response (zero loss); non-Thrift
backend path synthesizes via the new helpers.
- DBSQLOperation.getResultMetadata() — new additive neutral accessor on
IOperation; DBSQLSession.handleStagingOperation uses it instead of the
deprecated Thrift-shaped getMetadata().
F2 — IBackend.connect() is now zero-arg. Backend reads everything it needs
from IClientContext / constructor; matches Python connector's pattern of
passing session_configuration via constructor not method-arg.
F3 — Restore the 'Server protocol version' debug log dropped by the original
PR-378 refactor. Re-added to ThriftSessionBackend.constructor with the
LogLevel.debug + IClientContext.getLogger() pattern; matches the pre-refactor
log site at main:lib/DBSQLSession.ts:175.
F4 + F11 + F14 — SeaBackend stub safety:
- close() is a no-op so DBSQLClient.close()'s state-clearing block can finish
even after a useSEA: true connect() failure.
- connect() and openSession() throw HiveDriverError instead of generic Error,
matching the rest of the codebase.
- connect(options: ConnectionOptions) and openSession(request: OpenSessionRequest)
declare their parameters (with @typescript-eslint/no-unused-vars disable)
so IDE autocomplete prompts the M1 SEA implementer.
F6 + F7 + F9 + F10 — JSDoc on backend interfaces:
- IBackend: connect/openSession/close docstrings; close() doc explicitly
states transport-layer cleanup is owned by DBSQLClient.
- ISessionBackend: copy IDBSQLSession's per-method one-liner JSDoc.
- IOperationBackend: doc hasResultSet (readonly external; mutates internally),
waitUntilReady (MUST throw OperationStateError on terminal non-success).
F8 — tests/unit/sea/SeaBackend.test.ts (new) locks in the stub contract:
connect() rejects HiveDriverError, openSession() rejects HiveDriverError,
close() resolves no-op. ~30 LOC.
F12 — Drop legacy { handle, ... } ctor branch from DBSQLOperation and
DBSQLSession:
- Facades accept only { backend, context }.
- DBSQLSession no longer imports ThriftSessionBackend at all.
- DBSQLOperation imports ThriftOperationBackend solely for the F1 typed
downcast (zero-loss Thrift fast path); this is a deliberate, scoped
coupling tied to the back-compat decision.
- tests/unit/.stubs/createSessionForTest.ts and createOperationForTest.ts
(new) wrap the legacy shape; all 48 + 54 test sites mechanically migrated.
F15 — ThriftOperationBackend.waitUntilReady uses imported WaitUntilReadyOptions
type instead of an anonymous inline shape.
F16 — useSEA flag moved out of public ConnectionOptions:
- Removed useSEA?: boolean from the exported lib/contracts/IDBSQLClient.ts
ConnectionOptions; no longer ships in the public .d.ts.
- lib/contracts/InternalConnectionOptions.ts (new) declares the flag as a
non-exported internal extension; DBSQLClient.connect() reads via a typed
cast. Mirrors Python's kwargs.get('use_sea', False) pattern at
databricks-sql-python/src/databricks/sql/session.py:111.
F17 — Missing return; after case 'timeout' in forwardConnectionEvent so a
future fifth case doesn't silently fall through. The trailing return; in
the last case triggers no-useless-return — quieted with a localized
eslint-disable-next-line + intent comment.
F5 — deferred per owner instruction (test-only as any cast tightening).
Verification:
- yarn lint clean (3 pre-existing warnings in tests/e2e/protocol_versions.test.ts).
- yarn build clean.
- tsc --noEmit -p tsconfig.json clean (apart from pre-existing
examples/tokenFederation/* import errors that exist on main).
- Runtime smoke test of SeaBackend stub + Thrift-wire synthesis round-trip
passes 5/5 assertions.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
@msrathore-db
msrathore-dbforce-pushed the msrathore/sea-napi-binding branch from 20231c8 to 2f1865bCompareMay 24, 2026 21:58
@msrathore-db
msrathore-dbforce-pushed the msrathore/sea-errors-logging branch from 8ccc8e2 to c9a8811CompareMay 24, 2026 21:58
@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Creates the napi-rs binding skeleton: Cargo.toml + lib.rs + module
stubs for database/connection/statement/result/error/logger. Captures
napi-rs tokio Handle via OnceCell in runtime.rs. Single working
#[napi] fn version() proves the binding loads + executes end-to-end
in Node.
Depends on krn-async-public-api branch (path dep on kernel).
Round 2 will add open/execute/fetch methods.
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…wired
Adds real async methods on the opaque wrappers backing M0:
- openSession (free function) with PAT → kernel Session
- Connection::execute_statement → kernel ExecutedStatement
- Statement::fetch_next_batch / schema / cancel / close → kernel ResultStream
- Arrow batches returned as IPC bytes (per Layer 2 design)
- Error mapping preserves kernel ErrorCode + SQLSTATE for TS layer
- All entry points wrapped in catch_unwind
End-to-end smoke test against pecotesting passes.
No new dependencies beyond arrow-{ipc,array,schema} + futures.
Uses kernel async public API (no block_on).
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…indings
Round 1 scaffold declared tracing + tracing-subscriber as deps but
never used them. Removed. Logger bridge will re-add in round 3.
Other findings from 6b3affd-2026-05-15.md reviewed:
- Finding 2 (Database::Drop unreachable in Round 1b) — obsoleted by
Round 2 (40d0b57): database.rs no longer declares a Database struct
or Drop impl; it is now an `open_session` free function.
- Finding 3 (empty Connection::Drop) — obsoleted by Round 2: the Drop
impl now spawns a real fire-and-forget close on the captured tokio
handle.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Per D-006 architectural decision (Python team's workspace pattern):
all language bindings (PyO3, napi-rs) now live as workspace siblings
in the kernel repo at databricks-sql-kernel/{pyo3,napi}/.
What this commit removes from the nodejs repo:
- native/sea/Cargo.toml (path dep relocated; package now at
databricks-sql-kernel/napi/Cargo.toml with path = "..")
- native/sea/build.rs
- native/sea/src/* (lib, runtime, database, connection, statement,
result, error, logger, util — all 9 files)
- native/sea/package.json (the @databricks/sea-native-linux-x64-gnu
sub-package moves to the kernel workspace too)
- native/sea/index.js (regenerated artifact)
What stays in nodejs:
- native/sea/index.d.ts — TS declarations consumed by lib/sea/ adapter
- native/sea/README.md (new) — explains the move; points readers at
databricks-sql-kernel/napi/
What's updated:
- package.json: `build:native` and `build:native:debug` scripts now
delegate to the kernel workspace via $DATABRICKS_SQL_KERNEL_REPO
(defaults to ../../databricks-sql-kernel-sea-WT/napi-binding for the
local dev worktree layout). Build copies index.node + index.d.ts
back into native/sea/ for the loader to find.
Why workspace co-location:
- Arrow version pinning lockstep — no silent IPC version drift
- path = ".." (clean) vs ../../../../databricks-sql-kernel-sea-WT/...
- Single CI: cargo build --workspace covers kernel + pyo3 + napi
- Kernel API changes that break either binding caught at PR-review time
- Future cgo binding for Go SEA slots in as another workspace member
This branch (sea-napi-binding) is now a thin consumer of the kernel
napi crate. The actual Rust code lives at krn-napi-binding HEAD on
the kernel repo (commit debe3d7).
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…generated
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Addresses PR #380 review findings C1, C2 (partial), C3, H1, H2, H3,
H4, H5, H6, H7, H8 (runtime guard), H9, M1, M2, M3, M4 (linguist),
M5, M6, M7, M8, L1, L2, L4.
- C1 lint: drop `.js` ext from native/sea require so eslint
import/extensions passes; gitignore the auto-generated index.js
and *.node artifacts; prettier-ignore the napi-rs auto-generated
index.d.ts / index.js.
- C2 publish: whitelist native/sea/index.{js,d.ts} in .npmignore;
declare @databricks/sea-native-linux-x64-gnu in optionalDependencies.
The kernel-side package-name rename (drop the linux-x64-gnu
prefix) is tracked separately as a cross-PR ask.
- C3 test wiring: move tests/native/version.test.ts ->
tests/unit/sea/, tests/native/e2e-smoke.test.ts -> tests/e2e/sea/;
both now picked up by the existing mocharc globs and run via the
existing `npm test` / `npm run e2e` jobs.
- H1 noise drop: version.test.ts now asserts one meaningful semver
check; three tautological assertions removed.
- H2 + H3 + H7 lazy load: rewrite SeaNativeLoader.ts on the
lib/utils/lz4.ts pattern. Lazy require behind getSeaNative();
capability-detection helper tryGetSeaNative(); structured error
messages that classify MODULE_NOT_FOUND vs ERR_DLOPEN_FAILED and
include platform/arch/Node-version + install hint.
- H4 supply chain: pin @napi-rs/cli to 2.18.4 in devDependencies;
build:native switches to `npx --no-install` so the pinned local
install is used (no per-build network fetch).
- H5 path: switch build:native default kernel path to the canonical
`../../databricks-sql-kernel/napi-binding` (the worktree-specific
`-sea-WT` suffix is gone).
- H6 CI safety: e2e suite hard-fails when CI=true and any required
env var is missing; dev machines still skip.
- H8 runtime guard: loader throws a structured error on Node <18
instead of attempting a dlopen that would fail mysteriously.
Driver's engines.node stays >=14 — Thrift consumers on older
Node continue to work.
- H9 + M1 + M2 types: tsconfig adds a `@sea-native` path alias to
native/sea/index.d.ts; loader imports the real Connection /
Statement / ConnectionOptions / ExecuteOptions / ArrowBatch /
ArrowSchema types and re-exports them. Tests use the re-exported
types — the inline-shape duplication across three files collapses.
- M3 README: rewrite native/sea/README.md to match kernel reality.
The napi crate is a standalone Cargo workspace (not a pyo3
sibling); explain the tls-rustls choice that the standalone
workspace exists to enable.
- M4 drift detection: mark native/sea/index.d.ts as
linguist-generated in .gitattributes so GitHub collapses it in
diffs and excludes from blame/language stats.
- M5 artifacts: gitignore native/sea/index.js, index.node,
index.*.node.
- M6 + M7 e2e coverage: decode the IPC payload via apache-arrow's
tableFromIPC and assert numRows + cell value (not just shape);
add drain-past-null idempotence and schema-before-fetch
coverage.
- L1 build scripts: collapse build:native + build:native:debug
into one script taking BUILD_PROFILE (defaults to --release).
- L2 / L4 / M8: drop the version() alias and the "Round 2+ will…"
comment debt; the loader now actually delivers the value the
prior JSDoc was only promising.
Verified on this branch: npm run lint clean (0 errors); npm run
prettier clean for PR-owned files (3 unrelated pre-existing
warnings on PR #378 territory); tsc --project tsconfig.build.json
clean; mocha on tests/unit/sea passes (4/4); mocha on
tests/e2e/sea passes against a live pecotesting warehouse (2/2,
IPC decode confirms SELECT 1 returns 1).
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
@msrathore-db
msrathore-dbforce-pushed the msrathore/sea-napi-binding branch from 6b70ce4 to 548a14bCompareMay 24, 2026 23:23
@msrathore-db
msrathore-dbforce-pushed the msrathore/sea-errors-logging branch from c9a8811 to 3dc4b3cCompareMay 24, 2026 23:31
msrathore-db added a commit that referenced this pull request May 29, 2026
… IOperationBackend (#378)
* sea-abstraction: introduce IBackend / ISessionBackend / IOperationBackend
Refactors DBSQLClient/Session/Operation to dispatch through three
backend interfaces. ThriftBackend (lib/thrift-backend/) contains the
relocated existing thrift logic. SeaBackend (lib/sea/) is a stub for
M0; the sea-napi-binding feature wires the real impl.
Public surface (lib/index.ts) unchanged.
No new dependencies. All existing tests pass.
Files:
- lib/contracts/IBackend.ts (new)
- lib/contracts/ISessionBackend.ts (new)
- lib/contracts/IOperationBackend.ts (new)
- lib/contracts/IDBSQLClient.ts (adds useSEA?: boolean to ConnectionOptions)
- lib/thrift-backend/ThriftBackend.ts (new)
- lib/thrift-backend/ThriftSessionBackend.ts (new)
- lib/thrift-backend/ThriftOperationBackend.ts (new)
- lib/sea/SeaBackend.ts (new, M0 stub)
- lib/DBSQLClient.ts (dispatch through IBackend; useSEA picks SeaBackend)
- lib/DBSQLSession.ts (facade over ISessionBackend; staging stays here)
- lib/DBSQLOperation.ts (facade over IOperationBackend; iterators/fetchAll stay here)
- tests/unit/DBSQLClient.test.ts (retarget internal state lookup through backend; pre-seed client.backend in tests that bypass connect())
- tests/unit/DBSQLOperation.test.ts (retarget internal state lookup through backend)
* sea-abstraction: cleanup — restore JSDoc, dedupe test pre-seed, fix inline type
Addresses code-bloat-watchdog findings from commit 0085928:
- Restores public-API JSDoc on DBSQLSession + DBSQLOperation methods
(was deleted as scope creep; contracts unchanged so docs still apply)
- Adds makeStubbedClient() helper to tests/unit/DBSQLClient.test.ts;
replaces 14× duplicated ThriftBackend pre-seed
- Imports WaitUntilReadyOptions instead of inline option types in
IOperationBackend + DBSQLOperation.waitUntilReady
* sea-abstraction: address full-review findings (F1-F17 except F5)
Round-N fixes from the 9-reviewer pre-review. Public IOperation/DBSQLOperation
surface preserved byte-identical; backend interfaces (IBackend / ISessionBackend
/ IOperationBackend) made fully neutral so both Thrift and SEA can implement
the same contract.
F1 — neutral DTOs at IOperationBackend with Thrift-shape preservation on the
public facade (adapter pattern):
- lib/contracts/OperationStatus.ts (new) — neutral OperationStatus + OperationState
enum mirroring databricks-sql-python's CommandState and kernel pyo3's
StatementStatus taxonomy.
- lib/contracts/ResultMetadata.ts (new) — neutral ResultMetadata + ResultFormat
enum mirroring the three TSparkRowSetType cases.
- IOperationBackend.status()/getResultMetadata() return the neutral DTOs.
- ThriftOperationBackend.status() adapts at the boundary via adaptOperationStatus
/ adaptResultMetadata; module-level helpers thriftStateToOperationState and
thriftRowSetTypeToResultFormat do the enum maps.
- ThriftOperationBackend exposes thriftStatusResponse() and
thriftResultMetadataResponse() as public Thrift-only accessors used by the
facade's zero-loss fast path (kept for internal state-machine + result-handler
dispatch as well).
- lib/utils/thriftWireSynthesis.ts (new) — synthesizeThriftStatus and
synthesizeThriftResultSetMetadata: convert neutral DTOs back to Thrift wire
shape for the non-Thrift backend path. Lossy on Thrift-only fields
(taskStatus, numModifiedRows, cacheLookupResult, etc.).
- DBSQLOperation.status() and getMetadata() preserved Thrift return shape:
Thrift backend path returns the real wire response (zero loss); non-Thrift
backend path synthesizes via the new helpers.
- DBSQLOperation.getResultMetadata() — new additive neutral accessor on
IOperation; DBSQLSession.handleStagingOperation uses it instead of the
deprecated Thrift-shaped getMetadata().
F2 — IBackend.connect() is now zero-arg. Backend reads everything it needs
from IClientContext / constructor; matches Python connector's pattern of
passing session_configuration via constructor not method-arg.
F3 — Restore the 'Server protocol version' debug log dropped by the original
PR-378 refactor. Re-added to ThriftSessionBackend.constructor with the
LogLevel.debug + IClientContext.getLogger() pattern; matches the pre-refactor
log site at main:lib/DBSQLSession.ts:175.
F4 + F11 + F14 — SeaBackend stub safety:
- close() is a no-op so DBSQLClient.close()'s state-clearing block can finish
even after a useSEA: true connect() failure.
- connect() and openSession() throw HiveDriverError instead of generic Error,
matching the rest of the codebase.
- connect(options: ConnectionOptions) and openSession(request: OpenSessionRequest)
declare their parameters (with @typescript-eslint/no-unused-vars disable)
so IDE autocomplete prompts the M1 SEA implementer.
F6 + F7 + F9 + F10 — JSDoc on backend interfaces:
- IBackend: connect/openSession/close docstrings; close() doc explicitly
states transport-layer cleanup is owned by DBSQLClient.
- ISessionBackend: copy IDBSQLSession's per-method one-liner JSDoc.
- IOperationBackend: doc hasResultSet (readonly external; mutates internally),
waitUntilReady (MUST throw OperationStateError on terminal non-success).
F8 — tests/unit/sea/SeaBackend.test.ts (new) locks in the stub contract:
connect() rejects HiveDriverError, openSession() rejects HiveDriverError,
close() resolves no-op. ~30 LOC.
F12 — Drop legacy { handle, ... } ctor branch from DBSQLOperation and
DBSQLSession:
- Facades accept only { backend, context }.
- DBSQLSession no longer imports ThriftSessionBackend at all.
- DBSQLOperation imports ThriftOperationBackend solely for the F1 typed
downcast (zero-loss Thrift fast path); this is a deliberate, scoped
coupling tied to the back-compat decision.
- tests/unit/.stubs/createSessionForTest.ts and createOperationForTest.ts
(new) wrap the legacy shape; all 48 + 54 test sites mechanically migrated.
F15 — ThriftOperationBackend.waitUntilReady uses imported WaitUntilReadyOptions
type instead of an anonymous inline shape.
F16 — useSEA flag moved out of public ConnectionOptions:
- Removed useSEA?: boolean from the exported lib/contracts/IDBSQLClient.ts
ConnectionOptions; no longer ships in the public .d.ts.
- lib/contracts/InternalConnectionOptions.ts (new) declares the flag as a
non-exported internal extension; DBSQLClient.connect() reads via a typed
cast. Mirrors Python's kwargs.get('use_sea', False) pattern at
databricks-sql-python/src/databricks/sql/session.py:111.
F17 — Missing return; after case 'timeout' in forwardConnectionEvent so a
future fifth case doesn't silently fall through. The trailing return; in
the last case triggers no-useless-return — quieted with a localized
eslint-disable-next-line + intent comment.
F5 — deferred per owner instruction (test-only as any cast tightening).
Verification:
- yarn lint clean (3 pre-existing warnings in tests/e2e/protocol_versions.test.ts).
- yarn build clean.
- tsc --noEmit -p tsconfig.json clean (apart from pre-existing
examples/tokenFederation/* import errors that exist on main).
- Runtime smoke test of SeaBackend stub + Thrift-wire synthesis round-trip
passes 5/5 assertions.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
* sea-abstraction: address PR #378 review-comment fixes (H1 / M1-M4 / L1-L10)
Addresses 15 review findings from the code-review-squad pass on PR #378.
L11 (backend kind field on the three interfaces) is deliberately deferred
to avoid a cross-stack cascade ripple while the downstream PRs are still
in flight.
H1 — fetchChunk lost mid-flight failIfClosed regression.
Add optional `isClosed?: () => boolean` to IOperationBackend.fetchChunk's
options bag. ThriftOperationBackend.fetchChunk probes it after the
setTimeout(0) macrotask yield and returns [] when set; the facade's
post-fetch failIfClosed then raises the user-visible OperationStateError.
Restores the guard that the refactor split across the facade/backend
boundary so a cancel/close arriving during the yield window no longer
runs the data RPC to completion needlessly.
M1 — neutralize WaitUntilReadyOptions callback shape.
Introduce IOperationBackendWaitOptions { callback?: (status:
OperationStatus) => unknown } on the backend interface. Facade keeps
the public Thrift-typed OperationStatusCallback and adapts at the
boundary by wrapping the user's callback with synthesizeThriftStatus.
ThriftOperationBackend.waitUntilReady consumes the neutral options and
passes adaptOperationStatus(response) to the callback.
M2 — synthesizeOkStatus maps OperationState to TStatusCode.
Add synthesizeStatusFromOperation that returns ERROR_STATUS for
Failed/Cancelled/Closed (carrying errorMessage + sqlState) and
SUCCESS_STATUS otherwise. Wire it into synthesizeThriftStatus so
legacy Status.assert(resp.status) sees the right code on non-Thrift
backends.
M3 — TelemetryEvent + DriverConfiguration carry a backend tag.
Add optional backend?: 'thrift' | 'sea' | 'kernel' on both interfaces
so dashboards can slice latency/error rate by backend without a
metrics-schema migration once non-Thrift emission goes live.
M4 — test coverage for the synthesize helpers + useSEA failure path.
New tests/unit/thrift-backend/wireSynthesis.test.ts covering all
OperationState/ResultFormat mappings, ERROR_STATUS carries
errorMessage/sqlState, hasResultSet round-trip, schema/arrowSchema/
lz4Compressed/isStagingOperation preservation, and the L3 throw on
unknown ResultFormat. New test in DBSQLClient.test.ts asserts that a
useSEA:true connect failure leaves this.backend === undefined and the
next openSession() surfaces "not connected" rather than the
SeaBackend's "not implemented" error.
L1 — forwardConnectionEvent normalizes payload to Error.
Replace `payload as Error` with `payload instanceof Error ? payload
: new Error(String(payload))` so a backend that emits a non-Error
through the cross-backend onConnectionEvent doesn't crash the
logger.log call.
L2 — DBSQLClient.connect publishes this.backend only on success.
Construct the backend locally, await connect() in a try/catch, run a
best-effort backend.close() (per IBackend.close()'s
safe-on-partial-init contract) and rethrow on failure. Only assign
this.backend after a clean connect so a failed connect surfaces
"DBSQLClient: not connected" on the next openSession.
L3 — resultFormatToThrift throws on unknown ResultFormat.
Replace the silent default fallback to COLUMN_BASED_SET with a
HiveDriverError. Prevents a future ResultFormat enum extension from
silently routing results through JsonResultHandler and surfacing
garbled rows.
L4 — DBSQLOperation.getMetadata carries @deprecated.
Adds the canonical TypeScript JSDoc tag so IDEs (strikethrough), tsc,
ESLint plugins, and agentic codegen pick up the soft deprecation in
favour of getResultMetadata.
L5 — numberToInt64 re-export carries @deprecated.
Re-export through a named const with a JSDoc block (rather than a
bare `export { ... } from`) so the @deprecated tag attaches to the
symbol consumers see in their IDE / .d.ts.
L6 — DBSQLSession.runBackend helper.
Collapse 11 duplicated `failIfClosed → backend.X → failIfClosed`
brackets into a single private runBackend<T>(fn) so the
open-flag-before-and-after contract has a name and can't be forgotten
in a new delegation method.
L7 — restore three why-comments deleted from DBSQLSession.
Staging-detection invariant in executeStatement, AWS-vs-Azure 404
difference on staging-remove, and the Content-Length-required note on
staging-upload. Verbatim from main; these document non-obvious
intentional behaviour the refactor inadvertently dropped.
L8 — hasResultSet becomes a method on IOperationBackend.
The value is state-dependent (the Thrift impl mutates the underlying
operation handle inside processOperationStatusResponse), so the
property+readonly+disclaimer-JSDoc pattern was misleading. Method
form makes the live-read semantics obvious to a fresh implementer.
3 facade call sites updated.
L9 — wireSynthesis moves under thrift-backend.
The file imports Thrift IDL types and produces Thrift-typed values;
it belongs next to ThriftOperationBackend, not in the neutral
lib/utils/ tree where it would creep into the dependency cone of
future backend-neutral helpers. Same reasoning that placed
numberToInt64 and getDirectResultsOptions under thrift-backend/.
L10 — interface-level downcast policy.
Add a JSDoc paragraph on IOperationBackend grandfathering the two
existing `instanceof ThriftOperationBackend` downcasts in
DBSQLOperation.status/getMetadata and prohibiting new ones. Future
zero-loss back-compat needs should extend the interface (or add an
optional method) rather than spawn a per-backend branch matrix.
Gates: yarn build (exit 0), yarn lint (0 errors, 3 pre-existing
warnings in tests/e2e/protocol_versions.test.ts), yarn test on touched
files (163 passing, +12 net new tests from M4 work; 2 failures pre-
existing on PR head unchanged: getSchema-directResults and the
LZ4-cloud-fetch flag — both flagged in the team-lead playbook as
known prior regressions).
Cascade implications for downstream PRs (#380#377#379#382#381#384#383): L8 converts hasResultSet from a property to a method,
M1 swaps WaitUntilReadyOptions for IOperationBackendWaitOptions on
the backend interface. Both are mechanical renames at downstream
backend impls when they rebase.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
* Fix SEA abstraction merge fallout
Restore Thrift compatibility paths needed by existing schema and result-handler tests after merging main telemetry changes.
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
* Restore Thrift result-handler compatibility hooks
Keep existing e2e-only inspection hooks available through the facade while the new backend abstraction owns result handling.
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
---------
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…DI seam, packaging, tests
Rebuilds native/sea against the merged kernel main (binding renamed
@databricks/sea-native -> @databricks/sql-kernel via kernel #82) and
addresses the /full-review findings on PR #380:
- C1: commit the napi-rs router (native/sea/index.js, un-ignored) + a
prepack assertion so the publish tarball can never ship without it.
Companion kernel fix (databricks-sql-kernel#93) corrects the base
package name so the router require paths resolve.
- C2: drop the @sea-native tsconfig path alias; use a relative import
so no unresolvable specifier leaks into the emitted .d.ts.
- C3/C11: error hints no longer name a 404-ing package; dlopen hint now
includes the underlying dlerror string + concrete remediation.
- C4: document M0 = linux-x64-gnu-only scope + npm scope-lock note.
- C5: SeaNativeBinding = typeof import('../../native/sea') (no drift).
- C6: SeaNativeLoader is now a class with an injectable load seam;
getSeaNative/tryGetSeaNative are thin process-global wrappers.
- C7: re-exports renamed Sea* to avoid colliding with Thrift types.
- C8: version.test.ts fails loud on the linux-x64 CI runner + shape
checks; new loader.test.ts covers the hint branches, Node gate,
shape check, and caching via the injected seam.
- C9: Node-version guard fails closed (NaN or < floor).
- C13: e2e smoke uses the shared tests/e2e/utils/config.ts creds.
- C10/C12 are resolved upstream in merged kernel main / deferred to M1.
index.d.ts regenerated from merged main: ExecuteOptions dropped
(catalog/schema/sessionConf are session-level on openSession),
close() awaits DeleteSession, schema() is sync, sessionId/statementId
getters added.
Verified: 11 unit tests; e2e SELECT 1 against a live warehouse, direct
and through mitmproxy (SEA REST: POST /sql/sessions -> POST
/sql/statements -> DELETE /sql/sessions/<id>).
Co-authored-by: Isaac
Single mapping function in lib/sea/SeaErrorMapping.ts converts the
napi-binding's surfaced kernel error (code+message+sqlstate) to the
appropriate existing JS error class. M0 minimum: PAT auth errors
land as AuthenticationError; cancel/timeout as OperationStateError;
network/internal as HiveDriverError. SQLSTATE preserved on the
error object via .sqlState property.
No new error classes. M1 may add nuance.
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
@msrathore-db
msrathore-dbforce-pushed the msrathore/sea-errors-logging branch from 3dc4b3c to 83015acCompareMay 31, 2026 00:02
@msrathore-db
msrathore-db marked this pull request as ready for review June 1, 2026 00:54
@msrathore-db
msrathore-dbforce-pushed the msrathore/sea-napi-binding branch 2 times, most recently from f95e3d4 to d6101aaCompareJune 1, 2026 05:46
@msrathore-db

Copy link
Copy Markdown
ContributorAuthor

Superseded — this layer is folded into the consolidated SEA foundation, now split into #409#410#411 (stacked on main).

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@msrathore-db