Skip to content

Commit 6b70ce4

Browse files
committed
sea-napi-binding: review round 2 — lint, publish, lazy-load, tests
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).
1 parent 2f1865b commit 6b70ce4

12 files changed

Lines changed: 330 additions & 230 deletions

File tree

.gitattributes

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,9 @@ Dockerfile* text
3131
#
3232
.gitattributes export-ignore
3333
.gitignore export-ignore
34+
35+
# napi-rs auto-generates this file from the kernel's `napi-binding/napi/`
36+
# crate; regenerated by `npm run build:native`. Tell git/GitHub it's
37+
# machine-generated so it collapses in diffs and is excluded from
38+
# blame and language stats.
39+
native/sea/index.d.ts linguist-generated=true

.gitignore

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,3 +10,10 @@ coverage_unit
1010
dist
1111
*.DS_Store
1212
lib/version.ts
13+
14+
# SEA native binding — copied/generated from kernel workspace by `npm run build:native`.
15+
# The committed contract is `native/sea/index.d.ts` (TypeScript declarations).
16+
# Everything else under native/sea/ is a build artifact and must not be committed.
17+
native/sea/index.js
18+
native/sea/index.node
19+
native/sea/index.*.node

.npmignore

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,13 @@
33
!dist/**/*
44
!thrift/**/*
55

6+
# SEA napi-rs router shim + TypeScript declarations. The router (index.js)
7+
# selects the per-platform `.node` artifact from `@databricks/sea-native-*`
8+
# optionalDependencies (populated when the kernel CI publishes them);
9+
# the .d.ts is the consumer-facing type contract.
10+
!native/sea/index.js
11+
!native/sea/index.d.ts
12+
613
!LICENSE
714
!NOTICE
815
!package.json

.prettierignore

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,3 +11,9 @@ coverage
1111
dist
1212
thrift
1313
package-lock.json
14+
15+
# Generated by napi-rs from the kernel's `napi-binding/napi/` crate;
16+
# regenerated by `npm run build:native`. Format follows napi-rs's
17+
# defaults (no semicolons), not this repo's prettier config.
18+
native/sea/index.d.ts
19+
native/sea/index.js

lib/sea/SeaNativeLoader.ts

Lines changed: 86 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -13,69 +13,105 @@
1313
// limitations under the License.
1414

1515
/**
16-
* Loader for the SEA (Statement Execution API) native binding.
16+
* Lazy loader for the SEA (Statement Execution API) native binding.
1717
*
18-
* Round 1b: minimal pass-through to the napi-rs auto-generated
19-
* `index.js` shim in `native/sea/`. The shim itself picks the right
20-
* per-platform `.node` artifact (linux-x64-gnu today; more triples in
21-
* the bundling feature).
22-
*
23-
* Round 2+ will extend this with: lazy require to defer the `.node`
24-
* load until the first SEA call, structured load-error diagnostics
25-
* (which platform/arch was attempted, whether the package was
26-
* installed at all), and a JS-side `DBSQLLogger` install path that
27-
* forwards to the binding's `installLogger()` once that surface lands.
18+
* Mirrors the load-failure-tolerant pattern of `lib/utils/lz4.ts`: the
19+
* `.node` artifact ships via per-platform optional dependencies
20+
* (`@databricks/sea-native-<triple>`), so its absence must not crash
21+
* a Thrift-only consumer of the driver. Callers that actually need
22+
* SEA invoke `getSeaNative()`, which throws a structured error if
23+
* the binding could not be loaded.
2824
*/
2925

30-
// The path is relative to this file at runtime (`dist/sea/SeaNativeLoader.js`)
31-
// resolving to `dist/sea/../../native/sea/index.js` once `tsc` has emitted
32-
// to `dist/`. We use a require-time path resolution because the napi
33-
// shim is plain CommonJS and not part of the TS source tree.
34-
//
35-
// eslint-disable-next-line @typescript-eslint/no-var-requires, import/no-dynamic-require, global-require
36-
const native = require('../../native/sea/index.js');
26+
import type {
27+
Connection as NativeConnection,
28+
Statement as NativeStatement,
29+
ConnectionOptions,
30+
ExecuteOptions,
31+
ArrowBatch,
32+
ArrowSchema,
33+
} from '@sea-native';
34+
35+
export type { ConnectionOptions, ExecuteOptions, ArrowBatch, ArrowSchema };
36+
export type Connection = NativeConnection;
37+
export type Statement = NativeStatement;
3738

38-
/**
39-
* Public surface of the native binding exposed to the rest of the
40-
* NodeJS driver. Round 2 lands `openSession` + opaque `Connection` /
41-
* `Statement` classes (the binding-generated `.d.ts` is the source of
42-
* truth for their method signatures — see `native/sea/index.d.ts`).
43-
*
44-
* We deliberately keep this typed loosely (`unknown` for the class
45-
* shapes) so the loader layer doesn't have to import the binding's
46-
* generated types and the JS adapter layer can introduce its own
47-
* higher-level wrappers without conflicting with the binding's TS
48-
* declarations.
49-
*/
5039
export interface SeaNativeBinding {
51-
/** Returns the native crate version (smoke test for the binding's load path). */
5240
version(): string;
53-
/** Open a session over PAT auth. Returns an opaque Connection. */
54-
openSession(opts: {
55-
hostName: string;
56-
httpPath: string;
57-
token: string;
58-
}): Promise<unknown>;
59-
/** Opaque Connection class — instance methods on the binding-generated d.ts. */
60-
Connection: Function;
61-
/** Opaque Statement class — instance methods on the binding-generated d.ts. */
62-
Statement: Function;
41+
openSession(options: ConnectionOptions): Promise<NativeConnection>;
42+
Connection: typeof NativeConnection;
43+
Statement: typeof NativeStatement;
44+
}
45+
46+
const MIN_NODE_MAJOR = 18;
47+
48+
function detectNodeMajor(): number {
49+
// `process.version` is `vX.Y.Z`; parseInt stops at the first non-digit.
50+
return parseInt(process.version.slice(1), 10);
51+
}
52+
53+
function loadFailureHint(err: NodeJS.ErrnoException): string {
54+
const platform = `${process.platform}-${process.arch}`;
55+
const installHint = `Install the matching optional dependency (e.g. @databricks/sea-native-${platform}).`;
56+
if (err.code === 'MODULE_NOT_FOUND') {
57+
return `SEA native binding not installed for platform ${platform} on Node ${process.version}. ${installHint}`;
58+
}
59+
if (err.code === 'ERR_DLOPEN_FAILED') {
60+
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}.`;
61+
}
62+
return `SEA native binding failed to load on platform ${platform} / Node ${process.version}: ${err.message}`;
63+
}
64+
65+
let cached: SeaNativeBinding | null | undefined;
66+
let cachedError: Error | undefined;
67+
68+
function tryLoad(): SeaNativeBinding | undefined {
69+
const nodeMajor = detectNodeMajor();
70+
if (Number.isFinite(nodeMajor) && nodeMajor < MIN_NODE_MAJOR) {
71+
cachedError = new Error(
72+
`SEA native binding requires Node >=${MIN_NODE_MAJOR}; running Node ${process.version}. Continue using the Thrift backend on this runtime.`,
73+
);
74+
return undefined;
75+
}
76+
77+
try {
78+
// The require path resolves to `native/sea/index.js` (the napi-rs
79+
// router). `.js` is omitted so eslint's `import/extensions` rule
80+
// accepts the call.
81+
// eslint-disable-next-line @typescript-eslint/no-var-requires, global-require
82+
return require('../../native/sea') as SeaNativeBinding;
83+
} catch (err) {
84+
if (err instanceof Error && 'code' in err) {
85+
cachedError = new Error(loadFailureHint(err as NodeJS.ErrnoException));
86+
return undefined;
87+
}
88+
cachedError = new Error(`SEA native binding failed to load with non-standard error: ${String(err)}`);
89+
return undefined;
90+
}
6391
}
6492

6593
/**
66-
* Returns the loaded native binding. Throws if the platform-specific
67-
* `.node` artifact cannot be found (napi-rs's auto-generated shim
68-
* surfaces a descriptive error in that case).
94+
* Returns the loaded native binding. Throws a structured error if
95+
* the binding is unavailable on this platform / Node version.
6996
*/
7097
export function getSeaNative(): SeaNativeBinding {
71-
return native as SeaNativeBinding;
98+
if (cached === undefined) {
99+
cached = tryLoad() ?? null;
100+
}
101+
if (cached === null) {
102+
throw cachedError ?? new Error('SEA native binding unavailable');
103+
}
104+
return cached;
72105
}
73106

74107
/**
75-
* Convenience accessor for the smoke-test path. Equivalent to
76-
* `getSeaNative().version()` but reads more naturally in tests and
77-
* REPLs.
108+
* Returns the loaded binding or `undefined` if it could not be
109+
* loaded. Use this for capability-detection at startup; use
110+
* `getSeaNative()` at the point where SEA is actually required.
78111
*/
79-
export function version(): string {
80-
return getSeaNative().version();
112+
export function tryGetSeaNative(): SeaNativeBinding | undefined {
113+
if (cached === undefined) {
114+
cached = tryLoad() ?? null;
115+
}
116+
return cached ?? undefined;
81117
}

native/sea/README.md

Lines changed: 52 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,41 +1,62 @@
11
# `native/sea/` — consumer-side directory for the Rust napi binding
22

33
**The Rust binding source lives in the kernel repo** at
4-
`databricks-sql-kernel/napi/`, as a workspace sibling of `pyo3/`.
5-
See `databricks-sql-kernel`'s root `Cargo.toml` `[workspace] members`.
6-
7-
## Why
8-
9-
Per the architectural decision recorded in
10-
`sea-workflow/decisions.md` (D-006), every language binding (PyO3,
11-
napi-rs, future cgo) is a workspace member of the kernel crate. This
12-
keeps Arrow version pinning lockstep, the path dep clean (`path = ".."`),
13-
and CI single (`cargo build --workspace`). The pattern matches polars,
14-
ruff, arrow-rs.
15-
16-
## What lives here
17-
18-
- `index.d.ts` — generated TypeScript declarations consumed by `lib/sea/`
19-
- `index.linux-x64-gnu.node` (and other platform variants) — symlinked
20-
or copied build artifacts from the kernel workspace at run time
21-
22-
## How to build the binding for local dev
4+
`databricks-sql-kernel/napi-binding/napi/`. Building it requires a
5+
local checkout of that repo — see "Build for local dev" below.
6+
7+
## Workspace topology
8+
9+
The napi crate is a **standalone Cargo workspace** (`[workspace]
10+
members = ["."]` in `napi-binding/napi/Cargo.toml`), **not** a
11+
sibling of `pyo3/` in the kernel root workspace.
12+
13+
The reason is Cargo feature unification. pyo3 builds the kernel with
14+
the default `tls-native` feature (system OpenSSL via `native-tls`).
15+
The napi crate has to opt INTO `tls-rustls` instead: napi modules are
16+
loaded into Node.js processes that statically link OpenSSL 3.x, and
17+
dynamically linking the system's OpenSSL 1.1 (which `native-tls`
18+
pulls in on Linux) collides with Node's symbols at module-load time
19+
and segfaults the process before any Rust code runs. `rustls` is
20+
pure Rust + `ring` and avoids the conflict entirely.
21+
22+
If napi lived in the same workspace as pyo3, `cargo build
23+
--workspace` would unify the kernel's feature set to `tls-native ∪
24+
tls-rustls`, link both TLS stacks into the resulting napi cdylib,
25+
and reintroduce the same clash. Standalone-workspace is the fix.
26+
27+
## What lives in this directory
28+
29+
- `index.d.ts` — TypeScript declarations consumed by `lib/sea/`.
30+
Generated by napi-rs from the Rust source; checked in as the
31+
consumer-facing type contract.
32+
- `index.js` — napi-rs's per-platform router shim. Gitignored;
33+
populated by `npm run build:native` for local dev. In published
34+
tarballs it ships alongside the `.d.ts` and `require()`s the
35+
right `@databricks/sea-native-<triple>` optional dependency.
36+
- `index.*.node` — the actual native binary, one per platform.
37+
Gitignored. In production these live in the per-triple optional
38+
dependencies (`@databricks/sea-native-linux-x64-gnu`, etc.); for
39+
local dev `npm run build:native` copies one into this directory.
40+
41+
## Build for local dev
2342

2443
```bash
2544
# From the nodejs repo root:
26-
npm run build:native
27-
# which delegates to the kernel workspace:
28-
# cd $DATABRICKS_SQL_KERNEL_REPO/napi && napi build --release
29-
# and copies the artifact back here
45+
export DATABRICKS_SQL_KERNEL_REPO=/path/to/your/databricks-sql-kernel/napi-binding
46+
npm run build:native # release build (default)
47+
BUILD_PROFILE= npm run build:native # debug build (empty BUILD_PROFILE drops --release)
3048
```
3149

32-
`$DATABRICKS_SQL_KERNEL_REPO` defaults to a path published with the
33-
release flow; for dev it points at a local checkout of
34-
`databricks-sql-kernel`.
50+
`DATABRICKS_SQL_KERNEL_REPO` is required when your kernel checkout
51+
isn't at `../../databricks-sql-kernel/napi-binding` relative to the
52+
nodejs repo.
3553

36-
## How to consume in production
54+
## Production load path
3755

38-
At release time the kernel CI publishes `@databricks/sea-native-<triple>`
39-
npm packages with the `.node` binaries. The nodejs driver declares them
40-
as `optionalDependencies` in `package.json`; `SeaNativeLoader.ts`
41-
resolves the right one at runtime.
56+
At release time the kernel's CI publishes
57+
`@databricks/sea-native-<triple>` npm packages — one per supported
58+
platform — each containing a single `.node` binary. The nodejs
59+
driver lists them as `optionalDependencies`; npm installs only the
60+
one matching the consumer's `process.platform` / `process.arch`.
61+
`native/sea/index.js` (the napi-rs router) then `require()`s the
62+
installed package at load time.

package.json

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,7 @@
1717
"test": "nyc --report-dir=${NYC_REPORT_DIR:-coverage_unit} mocha --config tests/unit/.mocharc.js",
1818
"update-version": "node bin/update-version.js && prettier --write ./lib/version.ts",
1919
"build": "npm run update-version && tsc --project tsconfig.build.json",
20-
"build:native": "bash -c 'cd ${DATABRICKS_SQL_KERNEL_REPO:-../../databricks-sql-kernel-sea-WT/napi-binding}/napi && npx --yes @napi-rs/cli@2 build --platform --release && cp index.* $OLDPWD/native/sea/'",
21-
"build:native:debug": "bash -c 'cd ${DATABRICKS_SQL_KERNEL_REPO:-../../databricks-sql-kernel-sea-WT/napi-binding}/napi && npx --yes @napi-rs/cli@2 build --platform && cp index.* $OLDPWD/native/sea/'",
20+
"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/'",
2221
"watch": "tsc --project tsconfig.build.json --watch",
2322
"type-check": "tsc --noEmit",
2423
"prettier": "prettier . --check",
@@ -49,6 +48,7 @@
4948
],
5049
"license": "Apache 2.0",
5150
"devDependencies": {
51+
"@napi-rs/cli": "2.18.4",
5252
"@types/chai": "^4.3.14",
5353
"@types/http-proxy": "^1.17.14",
5454
"@types/lz4": "^0.6.4",
@@ -91,6 +91,7 @@
9191
"winston": "^3.8.2"
9292
},
9393
"optionalDependencies": {
94-
"lz4": "^0.6.5"
94+
"lz4": "^0.6.5",
95+
"@databricks/sea-native-linux-x64-gnu": "0.1.0"
9596
}
96-
}
97+
}

0 commit comments

Comments
 (0)