[SEA-NodeJS] (8/8) M1 Phase 1 — OAuth M2M + OAuth U2M (5 review rounds, ZERO HIGH at close)#383
Closed
msrathore-db wants to merge 36 commits into
Closed
[SEA-NodeJS] (8/8) M1 Phase 1 — OAuth M2M + OAuth U2M (5 review rounds, ZERO HIGH at close)#383msrathore-db wants to merge 36 commits into
msrathore-db wants to merge 36 commits into
Conversation
…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
|
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 ( |
msrathore-db
force-pushed
the
msrathore/sea-auth-u2m
branch
from
e9131ae to
e7c979d
Compare
msrathore-db
changed the base branch from
msrathore/sea-results
to
msrathore/sea-integration
|
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 ( |
msrathore-db
changed the base branch from
msrathore/sea-integration
to
msrathore/sea-operation
This was referenced May 17, 2026
msrathore-db
changed the title
This was referenced May 17, 2026
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
force-pushed
the
msrathore/sea-operation
branch
from
9b057f0 to
1a9ddd9
Compare
msrathore-db
force-pushed
the
msrathore/sea-auth-u2m
branch
from
3917148 to
0eb08b8
Compare
|
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 ( |
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
force-pushed
the
msrathore/sea-operation
branch
from
1a9ddd9 to
1f914fd
Compare
…integration shape
Reconciles the sea-auth-u2m commits onto sea-integration (the linear-
stack target) by combining the post-integration SeaBackend / SeaSession-
Backend / SeaNativeLoader / test shapes with the OAuth M2M+U2M behaviors
introduced across the 5 review rounds on sea-auth-u2m.
Behaviors preserved from sea-auth-u2m:
- decodeNapiKernelError on openSession / executeStatement / close
(kernelMetadata namespace, sentinel-stripping, AuthenticationError
raising for kernel-side `Unauthenticated`)
- buildSeaConnectionOptions: id-keyed M2M-vs-U2M selector, blank-or-
reserved-literal credential rejection, defense-in-depth U2M-with-id
- AuthenticationError this.name constructor (LE4-1)
- discriminated SeaNativeConnectionOptions union (Pat | OAuthM2m |
OAuthU2m) at the napi-binding seam
Shape kept from sea-integration:
- SeaBackend constructor signature `{ context, nativeBinding? }`
(DBSQLClient.ts:241 call-site stays compiling)
- SeaSessionBackend as a separate module (was inline in sea-auth-u2m)
- SeaSessionBackendOptions: { connection, context, defaults?, id? }
- SeaSessionBackend session ids via uuidv4() (auth-only counter scheme
superseded; OAuth tests updated)
- post-integration SeaNativeLoader exports (SeaExecuteOptions,
SeaArrow{Batch,Schema}, SeaNative{Statement,Connection}) carry through
Test reconciliations:
- new SeaBackend(binding) → new SeaBackend({ nativeBinding: binding })
across 14 OAuth-test call-sites
- SeaBackendOptions.context made optional (constructor already
downcasts undefined; runtime callers always supply via DBSQLClient)
- session-id regex from /^sea-session-\d+$/ to UUIDv4
- _helpers/fakeBinding.ts openSession return cast to SeaNativeConnection
- execution.test.ts: the "rejects databricks-oauth (M0 PAT-only)" test
flips to "rejects unsupported auth modes (non-PAT, non-OAuth)" —
databricks-oauth is now the U2M happy path
- execution.test.ts: openSession round-trip asserts new authMode:'Pat'
field on the discriminated union
Skipped commit:
- 37156db (Cargo.toml path-dep flip) became empty after sea-integration's
napi-source relocation — the native crate is no longer at
native/sea/Cargo.toml, it's in the kernel workspace.
Verification (in /tmp/dry-run-nodejs):
- tsc --project tsconfig.build.json: clean
- SEA unit subset: 144/144 passing (87 sea-auth-u2m + 57 sea-integration)
- M2M e2e: 2/2 passing (happy-path 652ms + bad-secret AuthenticationError
233ms)
This is a dry-run-only commit. Do not push or force-push the real
sea-auth-u2m branch from this work; the real branch stays at e9131ae
until owner approves the rebase. Branch:
`dryrun/sea-auth-u2m-on-integration-fresh` lives in /tmp/dry-run-nodejs.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…rwarding to openSession The napi binding moved initialCatalog/initialSchema/sessionConfig from ExecuteOptions onto ConnectionOptions (matching pyo3) because the kernel does not read StatementSpec::statement_conf — they were silently no-op'd in the per-statement path. Adapter follows. - SeaAuth.ts: extend SeaNativeConnectionOptions with optional catalog / schema / sessionConf (intersection with each auth-mode variant). New SeaSessionDefaults interface for the shared shape. - SeaBackend.ts::openSession: fold OpenSessionRequest.initialCatalog / initialSchema / configuration into the napi options before the openSession call. Drop the SeaSessionBackend.defaults forwarding. - SeaNativeLoader.ts: drop SeaExecuteOptions; executeStatement now takes only the SQL. - SeaSessionBackend.ts: drop SeaSessionDefaults and defaults field; drop per-statement overlay logic. useCloudFetch becomes a no-op on the SEA path (kernel hardcodes disposition to INLINE_OR_EXTERNAL_LINKS; ResultConfig exposure is M1 work). - tests: replace per-statement-forwarding assertions with openSession-arg assertions. 23/23 SEA execution tests pass (143/143 across the SEA suite). Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Rebased onto the updated sea-operation (carrying the #379 review fixes up the stack). Test-fake refresh: - execution.test.ts: keep the sessionId getter; executeStatement(sql) only (the session-level options migration already on this branch dropped the per-statement options). - _helpers/fakeBinding.ts: cast Connection/Statement through the binding's member types instead of bare Function. - OAuth e2e tests (auth-m2m / auth-u2m): cast the connect literal as ConnectionOptions & InternalConnectionOptions for the useSEA opt-in. Known follow-ups (not test fakes; tracked separately): SeaBackend passes OAuth options (authMode/oauthClientId) that the merged-kernel binding's ConnectionOptions does not yet expose — the kernel's OAuth napi surface must land in main first; and the SeaOperationBackend neutral-type conformance (status/getResultMetadata) is the sea-results follow-up. Co-authored-by: Isaac
msrathore-db
force-pushed
the
msrathore/sea-auth-u2m
branch
from
43893da to
cea1925
Compare
|
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 ( |
Finishes the IOperationBackend neutral-type WIP that blocked the SEA
driver from compiling against the merged abstraction:
- SeaOperationBackend.status() returns the neutral OperationStatus
(OperationState.{Cancelled,Closed,Succeeded}) instead of a Thrift
TGetOperationStatusResp; getResultMetadata() returns the neutral
ResultMetadata (schema + ResultFormat.ArrowBased + arrowSchema) instead
of TGetResultSetMetadataResp. The IOperationBackend contract is neutral;
the Thrift backend adapts at its own boundary.
- ArrowResultConverter only consumes `schema`, so its ctor param is
narrowed from TGetResultSetMetadataResp to `{ schema?: TTableSchema }`
— now satisfied by BOTH the Thrift resp and the neutral ResultMetadata
(DRY: no per-backend adapter at the call site).
- SeaBackend casts the SeaAuth options union to the binding's openSession
param at the single boundary (authMode string-literal vs napi const enum
— same runtime values; cast localized per SeaAuth's const-enum note).
Validated locally end-to-end: the built DBSQLClient on the SEA backend
runs `SELECT 1, current_catalog()` → correct rows, and the databricks-
driver-test Node comparator (PR #281) runs thrift-vs-SEA with conn1
(Thrift) and conn2 (useSEA) both opening + querying OK.
Co-authored-by: Isaac
This was referenced May 31, 2026
gopalldb
reviewed
Jun 1, 2026
| * creation. Mirror that here so the adapter doesn't promise a | ||
| * capability the binding can't honour. | ||
| */ | ||
| export interface SeaSessionDefaults { |
Collaborator
There was a problem hiding this comment.
why are we using Sea wording here? Can't we use Kernel instead?
gopalldb
reviewed
Jun 1, 2026
| SeaNativeBinding, | ||
| SeaNativeConnection, | ||
| } from './SeaNativeLoader'; | ||
| import { mapKernelErrorToJsError, KernelErrorShape } from './SeaErrorMapping'; |
Collaborator
There was a problem hiding this comment.
can't we say KernelBackend instead? Sea is internal details of kernel. Tomorrow Kernel can talk to more backends also if needed.
Collaborator
|
🟠 P1 (Important — should fix before merge)
🟡 P2 (Minor)
|
#380 (napi binding) and #378 (backend abstraction) are squash-merged on main, and main's abstraction evolved past this branch's early fork (neutral IOperationBackendWaitOptions, hasResultSet() method, isClosed seam, dataProvider, telemetry mappers, relocated wireSynthesis). Resolution: - Facade / abstraction / thrift / test-stubs -> main (authoritative, evolved): DBSQLOperation, DBSQLSession, DBSQLClient, IOperationBackend, ThriftOperationBackend, createOperationForTest, createSessionForTest. - SeaBackend -> branch (real impl; main only had the NOT_IMPLEMENTED stub). - native/sea/* -> branch (full kernel binding surface the SEA code needs). - Loader files (SeaNativeLoader, loader.test, version.test) -> main (final CI fixes: injectable node-version seam, prettier, version gating). - package.json optionalDeps -> main (lz4 + @napi-rs/cli optional; dropped the unpublished sql-kernel pin); @napi-rs/cli removed from devDeps. SEA implementers still need conformance to main's neutral abstraction (follow-up commit). Co-authored-by: Isaac
…lint/format debt
Post-merge reconciliation so the consolidated foundation builds, tests, lints
and formats cleanly against current main:
Conformance to main's evolved IOperationBackend / loader:
- SeaOperationBackend: hasResultSet() is now a method (was a getter);
waitUntilReady() accepts neutral IOperationBackendWaitOptions; SeaStatement
import (loader renamed Sea*Native* -> Sea*).
- SeaOperationLifecycle: seaFinished / synthesizeFinishedStatus emit a neutral
OperationStatus (the facade adapts the public Thrift callback at its boundary).
- SeaBackend / SeaSessionBackend: SeaConnection / SeaStatement loader names.
- DBSQLClient: pass IClientContext into the real SeaBackend (main constructed
the old stub with no args).
Tests:
- Drop main's obsolete SeaBackend stub test (real backend is covered by
auth-pat/m2m/u2m + execution); update operation-lifecycle assertions to the
neutral OperationStatus shape; ArrowResultConverter / compatibility drop the
excess `status` field now that the converter takes neutral { schema? };
refresh fake bindings (structural cast covers the AuthMode const enum).
Build / lint / format debt (pre-existing on the branch):
- Pin flatbuffers to 23.5.26 to match apache-arrow@13's nested copy (25.x broke
SeaArrowIpcDurationFix typing on a clean install).
- .eslintrc: honor `_`-prefixed unused args, allow hoisted-function use, allow
`continue` (parser idiom) — patterns the SEA code already relies on.
- prettier-format the SEA source/tests; drop a dead mocha-plugin disable.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
|
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 ( |
This was referenced Jun 1, 2026
Contributor
Author
msrathore-db
added a commit
that referenced
this pull request
Jun 1, 2026
Third of three stacked PRs (base: [2/3] execution + results). Completes the
SEA foundation:
- ArrowResultConverter: INTERVAL parity. Formats Arrow Interval[YearMonth] /
Interval[DayTime] and Duration (rewritten to Int64 by SeaArrowIpcDurationFix)
into the canonical Thrift strings ("Y-M" / "D HH:mm:ss.fffffffff"), byte-
identical to the Thrift path. Threads the Arrow field through convertArrowTypes
so the duration-unit metadata is available at value-conversion time.
- Exhaustive operation-lifecycle coverage: seaCancel / seaClose / seaFinished
idempotency, flag-set-before-await ordering (cancel-mid-fetch), kernel-error
mapping, and the neutral OperationStatus callback shape.
- SeaIntervalParity tests build real Arrow IPC batches via flatbuffers and
assert the formatted strings.
With this, SEA reaches M0 parity with Thrift (connect/auth → execute →
fetch → operation lifecycle → INTERVAL types). Replaces the single 8/8 PR #383.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
added a commit
that referenced
this pull request
Jun 1, 2026
* feat(sea): SEA connect + auth (PAT + OAuth M2M/U2M) [1/3] First of three stacked PRs splitting the SEA foundation (was the single 8/8 PR #383). This PR establishes a SEA-backed connection and session: - SeaBackend: connect() validates auth + captures the napi ConnectionOptions; openSession() folds catalog/schema/sessionConf and opens a kernel session. - SeaAuth: PAT + OAuth M2M + OAuth U2M validation/routing (mirrors the DBSQLClient auth-validation pattern; slash-prepended httpPath via prependSlash). - SeaErrorMapping: kernel ErrorCode → JS error-class mapping. - SeaSessionBackend: session open/close. executeStatement + metadata methods throw a clear deferred error — wired in [2/3] SEA execution + results. - DBSQLClient: route `useSEA: true` to the real SeaBackend (with IClientContext). - native/sea: the napi-rs binding surface (.d.ts + router); the .node stays gitignored (CI does not build it, loader/version tests skip when absent). Tests: PAT / M2M / U2M / edge-case auth suites, kernel error mapping, and the DBSQLClient SEA-routing + partial-init guard. Drops the obsolete stub SeaBackend.test (real backend is covered by the auth suites). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * fix(sea): address review on #409 (packaging, auth-flow docs, context) - P0 packaging: the napi router's per-platform npm names were garbled — the M0 triple was baked into the prefix for every platform (@databricks/sea-native-linux-x64-gnu-<triple>, and the doubled ...-gnu-linux-x64-gnu). Corrected to the canonical @databricks/sql-kernel-<triple> (matches the loader hint + native/sea/README). Added native-packaging.test to lock the naming. The per-platform packages are unpublished, so they remain undeclared in optionalDependencies (npm ci can't resolve an unpublished pin); README documents the M0 build:native load path. - Moved @napi-rs/cli out of optionalDependencies (a build tool should not land in consumer installs); build:native now fetches it on demand via npx --yes. - P1 auth-flow: documented honestly that SEA's OAuth flow selection DIVERGES from Thrift — Thrift keys off the secret (DBSQLClient.ts:216), SEA keys off oauthClientId presence — including the two real gaps (id+no-secret throws; U2M has no custom client id). Fixed the stale DBSQLClient.ts:143 ref. - P1 stale ref: SeaErrorMapping pointed at DBSQLOperation.ts:209; the Canceled switch is now ~:374. Updated. - P2 context: SeaBackendOptions.context is now required (was an `as IClientContext` downcast of undefined → latent NPE). Tests pass a shared makeFakeContext(). - P2 prependSlash: dropped the dead lib/utils/prependSlash.ts (nothing imported it; SeaAuth and DBSQLClient each keep their own inline copy). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> --------- Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
added a commit
that referenced
this pull request
Jun 1, 2026
Third of three stacked PRs (base: [2/3] execution + results). Completes the
SEA foundation:
- ArrowResultConverter: INTERVAL parity. Formats Arrow Interval[YearMonth] /
Interval[DayTime] and Duration (rewritten to Int64 by SeaArrowIpcDurationFix)
into the canonical Thrift strings ("Y-M" / "D HH:mm:ss.fffffffff"), byte-
identical to the Thrift path. Threads the Arrow field through convertArrowTypes
so the duration-unit metadata is available at value-conversion time.
- Exhaustive operation-lifecycle coverage: seaCancel / seaClose / seaFinished
idempotency, flag-set-before-await ordering (cancel-mid-fetch), kernel-error
mapping, and the neutral OperationStatus callback shape.
- SeaIntervalParity tests build real Arrow IPC batches via flatbuffers and
assert the formatted strings.
With this, SEA reaches M0 parity with Thrift (connect/auth → execute →
fetch → operation lifecycle → INTERVAL types). Replaces the single 8/8 PR #383.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
added a commit
that referenced
this pull request
Jun 1, 2026
Third of three stacked PRs (base: [2/3] execution + results). Completes the
SEA foundation:
- ArrowResultConverter: INTERVAL parity. Formats Arrow Interval[YearMonth] /
Interval[DayTime] and Duration (rewritten to Int64 by SeaArrowIpcDurationFix)
into the canonical Thrift strings ("Y-M" / "D HH:mm:ss.fffffffff"), byte-
identical to the Thrift path. Threads the Arrow field through convertArrowTypes
so the duration-unit metadata is available at value-conversion time.
- Exhaustive operation-lifecycle coverage: seaCancel / seaClose / seaFinished
idempotency, flag-set-before-await ordering (cancel-mid-fetch), kernel-error
mapping, and the neutral OperationStatus callback shape.
- SeaIntervalParity tests build real Arrow IPC batches via flatbuffers and
assert the formatted strings.
With this, SEA reaches M0 parity with Thrift (connect/auth → execute →
fetch → operation lifecycle → INTERVAL types). Replaces the single 8/8 PR #383.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
added a commit
that referenced
this pull request
Jun 2, 2026
Third of three stacked PRs (base: [2/3] execution + results). Completes the
SEA foundation:
- ArrowResultConverter: INTERVAL parity. Formats Arrow Interval[YearMonth] /
Interval[DayTime] and Duration (rewritten to Int64 by SeaArrowIpcDurationFix)
into the canonical Thrift strings ("Y-M" / "D HH:mm:ss.fffffffff"), byte-
identical to the Thrift path. Threads the Arrow field through convertArrowTypes
so the duration-unit metadata is available at value-conversion time.
- Exhaustive operation-lifecycle coverage: seaCancel / seaClose / seaFinished
idempotency, flag-set-before-await ordering (cancel-mid-fetch), kernel-error
mapping, and the neutral OperationStatus callback shape.
- SeaIntervalParity tests build real Arrow IPC batches via flatbuffers and
assert the formatted strings.
With this, SEA reaches M0 parity with Thrift (connect/auth → execute →
fetch → operation lifecycle → INTERVAL types). Replaces the single 8/8 PR #383.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
added a commit
that referenced
this pull request
Jun 2, 2026
Third of three stacked PRs (base: [2/3] execution + results). Completes the
SEA foundation:
- ArrowResultConverter: INTERVAL parity. Formats Arrow Interval[YearMonth] /
Interval[DayTime] and Duration (rewritten to Int64 by SeaArrowIpcDurationFix)
into the canonical Thrift strings ("Y-M" / "D HH:mm:ss.fffffffff"), byte-
identical to the Thrift path. Threads the Arrow field through convertArrowTypes
so the duration-unit metadata is available at value-conversion time.
- Exhaustive operation-lifecycle coverage: seaCancel / seaClose / seaFinished
idempotency, flag-set-before-await ordering (cancel-mid-fetch), kernel-error
mapping, and the neutral OperationStatus callback shape.
- SeaIntervalParity tests build real Arrow IPC batches via flatbuffers and
assert the formatted strings.
With this, SEA reaches M0 parity with Thrift (connect/auth → execute →
fetch → operation lifecycle → INTERVAL types). Replaces the single 8/8 PR #383.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
added a commit
that referenced
this pull request
Jun 2, 2026
Third of three stacked PRs (base: [2/3] execution + results). Completes the
SEA foundation:
- ArrowResultConverter: INTERVAL parity. Formats Arrow Interval[YearMonth] /
Interval[DayTime] and Duration (rewritten to Int64 by SeaArrowIpcDurationFix)
into the canonical Thrift strings ("Y-M" / "D HH:mm:ss.fffffffff"), byte-
identical to the Thrift path. Threads the Arrow field through convertArrowTypes
so the duration-unit metadata is available at value-conversion time.
- Exhaustive operation-lifecycle coverage: seaCancel / seaClose / seaFinished
idempotency, flag-set-before-await ordering (cancel-mid-fetch), kernel-error
mapping, and the neutral OperationStatus callback shape.
- SeaIntervalParity tests build real Arrow IPC batches via flatbuffers and
assert the formatted strings.
With this, SEA reaches M0 parity with Thrift (connect/auth → execute →
fetch → operation lifecycle → INTERVAL types). Replaces the single 8/8 PR #383.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
added a commit
that referenced
this pull request
Jun 2, 2026
…411) * feat(sea): INTERVAL type parity + operation-lifecycle depth [3/3] Third of three stacked PRs (base: [2/3] execution + results). Completes the SEA foundation: - ArrowResultConverter: INTERVAL parity. Formats Arrow Interval[YearMonth] / Interval[DayTime] and Duration (rewritten to Int64 by SeaArrowIpcDurationFix) into the canonical Thrift strings ("Y-M" / "D HH:mm:ss.fffffffff"), byte- identical to the Thrift path. Threads the Arrow field through convertArrowTypes so the duration-unit metadata is available at value-conversion time. - Exhaustive operation-lifecycle coverage: seaCancel / seaClose / seaFinished idempotency, flag-set-before-await ordering (cancel-mid-fetch), kernel-error mapping, and the neutral OperationStatus callback shape. - SeaIntervalParity tests build real Arrow IPC batches via flatbuffers and assert the formatted strings. With this, SEA reaches M0 parity with Thrift (connect/auth → execute → fetch → operation lifecycle → INTERVAL types). Replaces the single 8/8 PR #383. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * fix(sea): address #411 review — exhaustive interval switch, docs, coverage Validated every interval edge case (null, multi-row, negative sub-year, sibling-survives) against a live pecotesting warehouse first — all byte-identical to Thrift. The findings were layering/dead-code/coverage, not runtime bugs. - F1: corrected the DURATION_UNIT_METADATA_KEY doc — the interval/duration branches are SEA-gated by construction (Thrift maps INTERVAL → ArrowString and never reaches them), NOT "reused by thrift" as the old comment claimed. - F3/F6/F10: formatArrowInterval now handles YEAR_MONTH only (typed `Interval` + `IntervalUnit.YEAR_MONTH`, no magic `=== 0`) and THROWS on any other unit. The old non-exhaustive default silently misread MONTH_DAY_NANO/undefined as [days,ms]; and the native Interval[DayTime] branch was dead+broken (the kernel emits DAY-TIME as Duration, handled separately) — removed it. - F2: exported the metadata key + added a test pinning it equal to the SEA-side declaration (guards against a silent rename drift). - F8: dropped the dead `_unit` param from formatDayTimeFromTotal. - F9: removed the dead duration check in the BigNum branch (rewritten Int64s arrive as raw bigint) and fixed the false "Int32Array instanceof Uint8Array" comment. - F7: added a YEAR-MONTH sub-year-negative unit test (-1 month → "-0-1"). - F4: added live e2e for null INTERVAL → null and multi-row batches. - F5: e2e before() now probes getSeaNative() and skips (not errors) when the binding is absent; dropped the flaky wall-clock latency assertions (assert behavior — cancel resolved, callback fired once). Deferred (low/informational, noted): F11 (consolidate test makeContext helpers), F12 (tighten instanceOf assertions), F13 (per-operation interval-representation breadcrumb — a per-value log would be spam). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> * fix(sea): address #411 review — fail-loud duration unit, interval test gaps Addresses the P2 review comment on #411 (the interval/duration layer) and cascades the #410 fixes underneath (this branch was rebased onto the updated #410 tip). Validated against a live warehouse (interval-duration + interval-edge e2e) and unit tests. - ArrowResultConverter.toNanoseconds: throw on an unrecognized Arrow duration unit instead of silently defaulting to NANOSECOND. The four units are exactly what SeaArrowIpcDurationFix stamps, so an unknown unit means the two sides drifted — fail loud, matching formatArrowInterval. - SeaIntervalParity tests: add DAY-TIME via Duration(MILLISECOND) and Duration(SECOND) (only MICROSECOND/NANOSECOND were exercised), and a test asserting the converter throws (HiveDriverError) on a native non-YEAR_MONTH Arrow Interval rather than misreading it. - interval-duration-e2e: flip the stale "raw Int64 on this layer" assertion to expect the formatted thrift string "1 02:03:04.000000000". That test was written on #410 (pre-formatter) and explicitly noted "#411 flips this to the formatted string" — now that #411's formatter is wired, the value is the byte-identical thrift DAY-TIME string. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com> --------- Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
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.
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.sea-abstractionsea-napi-binding.nodeartifactsea-errors-loggingErrorCode→ JS error-class mapping (M0 minimum)sea-authuseSEA: truesea-executionexecuteStatement+openSession(sessionConfig, initialCatalog/Schema)sea-resultssea-operationsea-auth-u2m← TIPCompanion 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 8/8 — TIP OF STACK. Contains everything in the M0 + M1 Phase 1 OAuth work. Single snapshot branch for end-to-end testing + benchmarking.
Summary
M1 Phase 1 OAuth M2M + OAuth U2M. Built on the cumulative M0 linear stack (#378–#384).
Size note (1902 LOC, over the 800 cap) — review commit-by-commit
This PR is large because it carries 5 rounds of adversarial + language review as accumulated commits. The commits ARE the audit trail; squashing or splitting would lose the round-by-round review record. Reviewers should walk it commit-by-commit:
f1ddad1— Round 1 base: OAuth M2M + U2M wiring throughSeaBackend→ napi → kernele244dec— Round 1 M2M review fixup: sharedfakeBindinghelper, doc + loader cleanup3809749— Round 1 review: HIGH error-mapping wiring + 7 mediums3aa6e04— Round 2 fixup: wrap close() indecodeNapiKernelError, raise on U2M+id, case-insensitive validation, preserve all envelope fieldsa317c10— Round 3 fixup: NF-N1 namespace kernel metadata, dedupe predicate, type-guard envelope, treat blank secret as U2M2e57346— Rewire M2M e2e to AAD SP on pecotesting HTTP_PATH2d344901— Round 4 fixup: restore M2M-with-bad-secret class, strip envelope sentinel, trim RetryError docd311718— Round 5 fixup: JSDoc selector contract, defense-in-depth test, message mutation safety, class-pin simplificatione7c979d— Post-integration rebase reconciliation: API-shear fix bridging OAuth code to post-M0SeaBackendOptions/IClientContextRound 5 closed ZERO HIGH findings from two independent fresh reviewers (devils-advocate + language-expert). Full review trail in
decisions.md:D-014..D-019.Verification at TIP
AuthenticationError)Companion kernel PR
#28 kernel-oauth-error-class reclassifies OAuth token-exchange failures as
Error::unauthenticated()— required for the bad-secret e2e to raiseAuthenticationError.Draft until you give the go for review.
Downstream fixes / reviewer note
native/sea/index.d.tsis present in this branch and the OAuth native typings were carried into feat(sea): expose maxConnections + lock complex types as Arrow default #392/feat(sea): queryTags through executeStatement + fetchAll regression lock #393 so reviewers should not re-flag the missing-binding issue there.Tracking
Co-authored-by: Isaac