-
Notifications
You must be signed in to change notification settings - Fork 49
[SEA-NodeJS] (2/8) napi-rs binding consumer — TS loader + build script #380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: msrathore/sea-abstraction
Are you sure you want to change the base?
Changes from all commits
e78ed27
c8211be
04728b7
ee7e82e
01f31cd
548a14b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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'; | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
With 0 callers today this is harmless. The moment SEA is wired into
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 Found by: agent-compat, architecture, devils-advocate, language, maintainability (5 reviewers). Posted by code-review-squad · |
||
| export type { ConnectionOptions, ExecuteOptions, ArrowBatch, ArrowSchema }; | ||
| export type Connection = NativeConnection; | ||
| export type Statement = NativeStatement; | ||
|
|
||
| export interface SeaNativeBinding { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 Found by: architecture. Posted by code-review-squad · |
||
| 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}).`; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
But the actual published package (per Same issue cross-platform:
Found by: ops, agent-compat. Posted by code-review-squad · |
||
| 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') { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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 · |
||
| 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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium — turns every
Consequence for PR 3/8: when The lz4 pattern this is "mirroring" gets away with module-state because the existing codebase wraps it (e.g.,
export class SeaNativeLoader {
constructor(private readonly load: () => SeaNativeBinding = defaultRequire) {}
get(): SeaNativeBinding { /* ... */ }
tryGet(): SeaNativeBinding | undefined { /* ... */ }
}Then 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 · |
||
| let cachedError: Error | undefined; | ||
|
|
||
| function tryLoad(): SeaNativeBinding | undefined { | ||
| const nodeMajor = detectNodeMajor(); | ||
| if (Number.isFinite(nodeMajor) && nodeMajor < MIN_NODE_MAJOR) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low — defensive guard works backwards.
In practice Suggested fix: if (!Number.isFinite(nodeMajor) || nodeMajor < MIN_NODE_MAJOR) {
// ...
}Posted by code-review-squad · |
||
| 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; | ||
| } | ||
| 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. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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", | ||
|
|
@@ -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", | ||
|
|
@@ -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" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium — by far the highest-consensus finding; 7 reviewers flagged it. Only Three coupled consequences:
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 · |
||
| } | ||
| } | ||
There was a problem hiding this comment.
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 ONLYREADME.mdandindex.d.ts— noindex.js..gitignore:17ignoresnative/sea/index.js(it's regenerated bynpm run build:native)..npmignore:10allow-lists!native/sea/index.jsto ship in the tarball.package.json scriptshas noprepackorprepublishOnlyhook that runsbuild:native.lib/sea/SeaNativeLoader.ts:82doesrequire('../../native/sea')— Node's resolver looks forindex.jsin that directory.Result: any
npm publishfrom a workspace that didn't manually runnpm run build:nativefirst ships a tarball missing the napi-rs router. Consumers getMODULE_NOT_FOUNDwith 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 wiresSeaBackend.connect()to the loader, every install bricks.native/sea/index.js(analogous to the committedindex.d.ts— the file is small and stable), or (b) add aprepackscript that runsbuild:nativegated onDATABRICKS_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