Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,9 @@ Dockerfile* text
#
.gitattributes export-ignore
.gitignore export-ignore

# napi-rs auto-generates this file from the kernel's `napi-binding/napi/`
# crate; regenerated by `npm run build:native`. Tell git/GitHub it's
# machine-generated so it collapses in diffs and is excluded from
# blame and language stats.
native/sea/index.d.ts linguist-generated=true
7 changes: 7 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,10 @@ coverage_unit
dist
*.DS_Store
lib/version.ts

# SEA native binding — copied/generated from kernel workspace by `npm run build:native`.
# The committed contract is `native/sea/index.d.ts` (TypeScript declarations).
# Everything else under native/sea/ is a build artifact and must not be committed.
native/sea/index.js
native/sea/index.node
native/sea/index.*.node
7 changes: 7 additions & 0 deletions .npmignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
!dist/**/*
!thrift/**/*

# SEA napi-rs router shim + TypeScript declarations. The router (index.js)
# selects the per-platform `.node` artifact from `@databricks/sea-native-*`
# optionalDependencies (populated when the kernel CI publishes them);
# the .d.ts is the consumer-facing type contract.
!native/sea/index.js
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High — release-engineering landmine that crashes every consumer the moment PR 3/8+ wires SEA in.

Verified at HEAD 548a14b8:

  • git ls-tree native/sea/ shows ONLY README.md and index.d.ts — no index.js.
  • .gitignore:17 ignores native/sea/index.js (it's regenerated by npm run build:native).
  • .npmignore:10 allow-lists !native/sea/index.js to ship in the tarball.
  • package.json scripts has no prepack or prepublishOnly hook that runs build:native.
  • The loader at lib/sea/SeaNativeLoader.ts:82 does require('../../native/sea') — Node's resolver looks for index.js in that directory.

Result: any npm publish from a workspace that didn't manually run npm run build:native first ships a tarball missing the napi-rs router. Consumers get MODULE_NOT_FOUND with a misleading hint that tells them to install the optional dep (which is already installed) instead of pointing at the missing router shim.

PR #380 doesn't crash anything today because nothing calls getSeaNative(). The moment PR 3/8 wires SeaBackend.connect() to the loader, every install bricks.

  • Suggested fix: either (a) commit native/sea/index.js (analogous to the committed index.d.ts — the file is small and stable), or (b) add a prepack script that runs build:native gated on DATABRICKS_RELEASE_BUILD=1, with an explicit [ -f native/sea/index.js ] || (echo "Missing native router; run npm run build:native first" && exit 1) assertion that fails the pack if it's still missing.

Found by: security, ops, maintainability, devils-advocate (4 reviewers).


Posted by code-review-squad · /full-review

!native/sea/index.d.ts

!LICENSE
!NOTICE
!package.json
Expand Down
6 changes: 6 additions & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,9 @@ coverage
dist
thrift
package-lock.json

# Generated by napi-rs from the kernel's `napi-binding/napi/` crate;
# regenerated by `npm run build:native`. Format follows napi-rs's
# defaults (no semicolons), not this repo's prettier config.
native/sea/index.d.ts
native/sea/index.js
117 changes: 117 additions & 0 deletions lib/sea/SeaNativeLoader.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
// Copyright (c) 2026 Databricks, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/**
* Lazy loader for the SEA (Statement Execution API) native binding.
*
* Mirrors the load-failure-tolerant pattern of `lib/utils/lz4.ts`: the
* `.node` artifact ships via per-platform optional dependencies
* (`@databricks/sea-native-<triple>`), so its absence must not crash
* a Thrift-only consumer of the driver. Callers that actually need
* SEA invoke `getSeaNative()`, which throws a structured error if
* the binding could not be loaded.
*/

import type {
Connection as NativeConnection,
Statement as NativeStatement,
ConnectionOptions,
ExecuteOptions,
ArrowBatch,
ArrowSchema,
} from '@sea-native';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — name collisions will surface as IDE autocomplete ambiguity and silent field-shape bugs once SEA is wired in.

Lines 34-37 re-export:

export type { ConnectionOptions, ExecuteOptions, ArrowBatch, ArrowSchema };
export type Connection = NativeConnection;
export type Statement = NativeStatement;

Collisions with existing public types:

  • ConnectionOptions (SEA: hostName/httpPath/token) vs ConnectionOptions from lib/contracts/IDBSQLClient.ts (Thrift: host/path/token + AuthOptions union). The token field is shared → silent shape-mismatch bugs when an IDE auto-imports the wrong one.
  • ArrowBatch (SEA: {ipcBytes: Buffer}) vs ArrowBatch from lib/result/utils.ts (Thrift: {batches: Buffer[]; rowCount: number}).

With 0 callers today this is harmless. The moment SEA is wired into DBSQLClient and these types are imported anywhere outside lib/sea/, the bare names become ambiguous in autocomplete and in code review.

  • Suggested fix: rename the SEA-side exports now, before any production consumer imports them:
export type { ConnectionOptions as SeaConnectionOptions, ExecuteOptions as SeaExecuteOptions, ArrowBatch as SeaArrowBatch, ArrowSchema as SeaArrowSchema };
export type SeaConnection = NativeConnection;
export type SeaStatement = NativeStatement;

The kernel-generated .d.ts can keep the napi-rs default names (it's the kernel's contract). The TS-side wrapper is where disambiguation should happen, and doing it before any consumer imports the bare names is a one-way door.

Found by: agent-compat, architecture, devils-advocate, language, maintainability (5 reviewers).


Posted by code-review-squad · /full-review

export type { ConnectionOptions, ExecuteOptions, ArrowBatch, ArrowSchema };
export type Connection = NativeConnection;
export type Statement = NativeStatement;

export interface SeaNativeBinding {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — single-source-of-truth violation.

Lines 39-44 declare:

export interface SeaNativeBinding {
  version(): string;
  openSession(options: ConnectionOptions): Promise<NativeConnection>;
  Connection: typeof NativeConnection;
  Statement: typeof NativeStatement;
}

This is a manual subset of what native/sea/index.d.ts already declares. When the kernel adds a new free function (e.g. cancelStatement, future async APIs), the .d.ts regenerates but this interface stays stale until someone notices. The unchecked cast at line 82 (require('../../native/sea') as SeaNativeBinding) hides the drift.

  • Suggested fix: derive the type from the generated module instead of redeclaring it:
import type * as SeaNative from '../../native/sea';
export type SeaNativeBinding = typeof SeaNative;

Then the cast at line 82 stays automatically correct, and the README's claim that "the .d.ts is the contract" is enforced by the type system.

Found by: architecture.


Posted by code-review-squad · /full-review

version(): string;
openSession(options: ConnectionOptions): Promise<NativeConnection>;
Connection: typeof NativeConnection;
Statement: typeof NativeStatement;
}

const MIN_NODE_MAJOR = 18;

function detectNodeMajor(): number {
// `process.version` is `vX.Y.Z`; parseInt stops at the first non-digit.
return parseInt(process.version.slice(1), 10);
}

function loadFailureHint(err: NodeJS.ErrnoException): string {
const platform = `${process.platform}-${process.arch}`;
const installHint = `Install the matching optional dependency (e.g. @databricks/sea-native-${platform}).`;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — every customer who hits this error is sent on a wild-goose chase.

Verified: line 55 builds platform = '${process.platform}-${process.arch}'. On linux/x64 that produces linux-x64; on darwin/arm64 it produces darwin-arm64. The error message then says:

Install the matching optional dependency (e.g. @databricks/sea-native-linux-x64).

But the actual published package (per package.json:95) is @databricks/sea-native-linux-x64-**gnu**. The hint's recommended npm install will 404.

Same issue cross-platform:

  • linux/x64 → suggests @databricks/sea-native-linux-x64; actual is -linux-x64-gnu or -linux-x64-musl

  • linux/arm64 → suggests @databricks/sea-native-linux-arm64; actual is -linux-arm64-gnu

  • win32/x64 → suggests @databricks/sea-native-win32-x64; actual is -win32-x64-msvc

  • Suggested fix: either compute the napi-rs triple correctly (mirror the router's platform→triple table — easier said than done) or drop the package-name example: "Install the matching @databricks/sea-native-* optional dependency for your platform; see the README for the supported triple list."

Found by: ops, agent-compat.


Posted by code-review-squad · /full-review

if (err.code === 'MODULE_NOT_FOUND') {
return `SEA native binding not installed for platform ${platform} on Node ${process.version}. ${installHint}`;
}
if (err.code === 'ERR_DLOPEN_FAILED') {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — debuggability.

When dlopen fails (binding installed but won't load), real-world causes include musl-vs-glibc, Node ABI version mismatch, architecture mismatch (x64 binary on arm64 via Rosetta). The current hint says "likely a libc or Node ABI mismatch" without:

  • Including the underlying err.message (which carries the actual dlerror string, e.g. GLIBC_2.32 not found).
  • Suggesting concrete remediation (rm -rf node_modules, install musl variant, etc.).

Suggested fix:

return `SEA native binding present but failed to dlopen on platform ${platform} / Node ${process.version}: ${err.message}. Common causes: glibc/musl mismatch (e.g., Alpine Linux), Node ABI mismatch (try \`rm -rf node_modules && npm install\`), or architecture mismatch.`;

Found by: ops, devils-advocate.


Posted by code-review-squad · /full-review

return `SEA native binding present but failed to dlopen on platform ${platform} / Node ${process.version} — likely a libc or Node ABI mismatch. The binding requires Node >=${MIN_NODE_MAJOR}.`;
}
return `SEA native binding failed to load on platform ${platform} / Node ${process.version}: ${err.message}`;
}

let cached: SeaNativeBinding | null | undefined;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — turns every SeaBackend unit test into an integration test once SEA is wired in.

cached and cachedError are module-level let bindings (lines 65-66). getSeaNative() / tryGetSeaNative() are free functions that close over them. There's no constructor, no DI seam, no exported reset hook.

Consequence for PR 3/8: when SeaBackend.connect() stops throwing NOT_IMPLEMENTED and starts calling getSeaNative(), any unit test that touches SeaBackend will require a real .node to be loadable on the test machine — otherwise it skips. sinon.stub(SeaNativeLoader, 'getSeaNative') works once per file but the cached value persists across files in the same mocha run. Order-dependence is baked in.

The lz4 pattern this is "mirroring" gets away with module-state because the existing codebase wraps it (e.g., Lz4Util and adapters at the call sites accept the resolved module). Here SeaNativeLoader IS the wrapper, and the wrapper has no seam.

  • Suggested fix: expose the loader as a class with an injectable seam:
export class SeaNativeLoader {
  constructor(private readonly load: () => SeaNativeBinding = defaultRequire) {}
  get(): SeaNativeBinding { /* ... */ }
  tryGet(): SeaNativeBinding | undefined { /* ... */ }
}

Then SeaBackend takes loader: SeaNativeLoader via constructor (matches the existing IClientContext pattern in lib/DBSQLClient.ts). Keep getSeaNative() as a thin convenience over a process-global default instance.

Much cheaper to change now (0 callers) than after PRs 3–8 land.

Found by: architecture, devils-advocate, maintainability adjacent (3 reviewers).


Posted by code-review-squad · /full-review

let cachedError: Error | undefined;

function tryLoad(): SeaNativeBinding | undefined {
const nodeMajor = detectNodeMajor();
if (Number.isFinite(nodeMajor) && nodeMajor < MIN_NODE_MAJOR) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — defensive guard works backwards.

if (Number.isFinite(nodeMajor) && nodeMajor < MIN_NODE_MAJOR) evaluates to false when nodeMajor === NaN, so the load proceeds. The intent reads "fail closed if we can't tell what Node we're on" but the code does the opposite.

In practice process.version is always parseable, so the practical risk is near-zero. But it's the kind of guard that future runtime quirks will surface.

Suggested fix:

if (!Number.isFinite(nodeMajor) || nodeMajor < MIN_NODE_MAJOR) {
  // ...
}

Posted by code-review-squad · /full-review

cachedError = new Error(
`SEA native binding requires Node >=${MIN_NODE_MAJOR}; running Node ${process.version}. Continue using the Thrift backend on this runtime.`,
);
return undefined;
}

try {
// The require path resolves to `native/sea/index.js` (the napi-rs
// router). `.js` is omitted so eslint's `import/extensions` rule
// accepts the call.
// eslint-disable-next-line @typescript-eslint/no-var-requires, global-require
return require('../../native/sea') as SeaNativeBinding;
} catch (err) {
if (err instanceof Error && 'code' in err) {
cachedError = new Error(loadFailureHint(err as NodeJS.ErrnoException));
return undefined;
}
cachedError = new Error(`SEA native binding failed to load with non-standard error: ${String(err)}`);
return undefined;
}
}

/**
* Returns the loaded native binding. Throws a structured error if
* the binding is unavailable on this platform / Node version.
*/
export function getSeaNative(): SeaNativeBinding {
if (cached === undefined) {
cached = tryLoad() ?? null;
}
if (cached === null) {
throw cachedError ?? new Error('SEA native binding unavailable');
}
return cached;
}

/**
* Returns the loaded binding or `undefined` if it could not be
* loaded. Use this for capability-detection at startup; use
* `getSeaNative()` at the point where SEA is actually required.
*/
export function tryGetSeaNative(): SeaNativeBinding | undefined {
if (cached === undefined) {
cached = tryLoad() ?? null;
}
return cached ?? undefined;
}
62 changes: 62 additions & 0 deletions native/sea/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# `native/sea/` — consumer-side directory for the Rust napi binding

**The Rust binding source lives in the kernel repo** at
`databricks-sql-kernel/napi-binding/napi/`. Building it requires a
local checkout of that repo — see "Build for local dev" below.

## Workspace topology

The napi crate is a **standalone Cargo workspace** (`[workspace]
members = ["."]` in `napi-binding/napi/Cargo.toml`), **not** a
sibling of `pyo3/` in the kernel root workspace.

The reason is Cargo feature unification. pyo3 builds the kernel with
the default `tls-native` feature (system OpenSSL via `native-tls`).
The napi crate has to opt INTO `tls-rustls` instead: napi modules are
loaded into Node.js processes that statically link OpenSSL 3.x, and
dynamically linking the system's OpenSSL 1.1 (which `native-tls`
pulls in on Linux) collides with Node's symbols at module-load time
and segfaults the process before any Rust code runs. `rustls` is
pure Rust + `ring` and avoids the conflict entirely.

If napi lived in the same workspace as pyo3, `cargo build
--workspace` would unify the kernel's feature set to `tls-native ∪
tls-rustls`, link both TLS stacks into the resulting napi cdylib,
and reintroduce the same clash. Standalone-workspace is the fix.

## What lives in this directory

- `index.d.ts` — TypeScript declarations consumed by `lib/sea/`.
Generated by napi-rs from the Rust source; checked in as the
consumer-facing type contract.
- `index.js` — napi-rs's per-platform router shim. Gitignored;
populated by `npm run build:native` for local dev. In published
tarballs it ships alongside the `.d.ts` and `require()`s the
right `@databricks/sea-native-<triple>` optional dependency.
- `index.*.node` — the actual native binary, one per platform.
Gitignored. In production these live in the per-triple optional
dependencies (`@databricks/sea-native-linux-x64-gnu`, etc.); for
local dev `npm run build:native` copies one into this directory.

## Build for local dev

```bash
# From the nodejs repo root:
export DATABRICKS_SQL_KERNEL_REPO=/path/to/your/databricks-sql-kernel/napi-binding
npm run build:native # release build (default)
BUILD_PROFILE= npm run build:native # debug build (empty BUILD_PROFILE drops --release)
```

`DATABRICKS_SQL_KERNEL_REPO` is required when your kernel checkout
isn't at `../../databricks-sql-kernel/napi-binding` relative to the
nodejs repo.

## Production load path

At release time the kernel's CI publishes
`@databricks/sea-native-<triple>` npm packages — one per supported
platform — each containing a single `.node` binary. The nodejs
driver lists them as `optionalDependencies`; npm installs only the
one matching the consumer's `process.platform` / `process.arch`.
`native/sea/index.js` (the napi-rs router) then `require()`s the
installed package at load time.
144 changes: 144 additions & 0 deletions native/sea/index.d.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"test": "nyc --report-dir=${NYC_REPORT_DIR:-coverage_unit} mocha --config tests/unit/.mocharc.js",
"update-version": "node bin/update-version.js && prettier --write ./lib/version.ts",
"build": "npm run update-version && tsc --project tsconfig.build.json",
"build:native": "bash -c 'cd ${DATABRICKS_SQL_KERNEL_REPO:-../../databricks-sql-kernel/napi-binding}/napi && npx --no-install @napi-rs/cli build --platform ${BUILD_PROFILE:---release} && cp index.* $OLDPWD/native/sea/'",
"watch": "tsc --project tsconfig.build.json --watch",
"type-check": "tsc --noEmit",
"prettier": "prettier . --check",
Expand Down Expand Up @@ -47,6 +48,7 @@
],
"license": "Apache 2.0",
"devDependencies": {
"@napi-rs/cli": "2.18.4",
"@types/chai": "^4.3.14",
"@types/http-proxy": "^1.17.14",
"@types/lz4": "^0.6.4",
Expand Down Expand Up @@ -89,6 +91,7 @@
"winston": "^3.8.2"
},
"optionalDependencies": {
"lz4": "^0.6.5"
"lz4": "^0.6.5",
"@databricks/sea-native-linux-x64-gnu": "0.1.0"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — by far the highest-consensus finding; 7 reviewers flagged it.

Only @databricks/sea-native-linux-x64-gnu: 0.1.0 is in optionalDependencies. macOS-arm64, macOS-x64, linux-arm64-gnu, linux-x64-musl (Alpine), win32-x64-msvc, etc. all have zero installable binding.

Three coupled consequences:

  1. Silent degradation — on every non-linux-x64-gnu platform, the loader will hit MODULE_NOT_FOUND. Users may not realize they're stuck on Thrift.
  2. Misleading hint (see M2) — the loader tells them to install a package that doesn't exist.
  3. Supply-chain squat risk — the unlisted names (@databricks/sea-native-darwin-arm64, …-darwin-x64, etc.) are not on the npm registry. Confirm @databricks org scope is locked to Databricks publishers, OR pre-register empty placeholder packages now, so an external party can't typosquat @databricks/sea-native-darwin-arm64 before Databricks does and have it autoload via the napi-rs router for darwin-arm64 consumers.
  • Suggested fix: Decide between two coherent models:
    • (a) Intentional M0 scope — explicitly document "M0 = linux-x64-gnu only; other platforms continue using Thrift exclusively" in the README and update the loader's error message to match.
    • (b) Multi-platform support — list all planned triples in optionalDependencies (npm tolerates missing optionalDeps on the registry — they skip silently with a warning).

Whichever you pick, address the supply-chain squat question separately (lock the scope or pre-register placeholders).

Found by: security, ops, devils-advocate, performance, architecture, agent-compat, language (7 reviewers).


Posted by code-review-squad · /full-review

}
}
Loading
Loading