Skip to content

Commit 522260a

Browse files
committed
fix(sea): F2 round-2 — root-cause lazy-load + lint clean (DA M1)
DA round 2 findings on F2 / F4 / F3b H1 had a common root cause the team-lead correctly flagged: `SeaNativeLoader.ts` does `const native = require('../../native/sea/index.js')` at module load, so any import from this module triggers the `.node` artifact load. When `yarn build:native` hasn't run, that throws MODULE_NOT_FOUND BEFORE mocha can fire `before()` skip-gates, crashing test discovery for every e2e suite that imports anything from this module — including transitive imports via `DBSQLClient` → `SeaBackend` → `SeaNativeLoader`. Round 1 dance: every e2e test got a `createRequire` + filesystem-check workaround. Round 2 fixes the actual defect: the loader itself memoises the require behind `getSeaNative()`, so importing the module is free for code paths that never reach SEA. Filesystem-check guards in the e2e tests stay as defense- in-depth (they produce a friendlier "run yarn build:native" diagnostic than napi-rs's raw MODULE_NOT_FOUND). Verified the fix by moving the .node artifact aside and running the F2 e2e: previously CRASHED at file load with MODULE_NOT_FOUND; now SKIPS cleanly via the suite's `before()` gate with an actionable message. Restored binary, re-ran the test: passes against pecotesting in 480ms. Also addresses F2 M1 lint: - `lib/sea/SeaNativeLoader.ts:131` — added `import/extensions` to the eslint-disable comment on the lazy-require, since the `.js` extension is required (napi shim is CommonJS, not TS) - `tests/e2e/sea/complex-types-e2e.test.ts:148` — switched to destructuring `const { schema } = table` per prefer- destructuring rule Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
1 parent d076067 commit 522260a

2 files changed

Lines changed: 44 additions & 17 deletions

File tree

lib/sea/SeaNativeLoader.ts

Lines changed: 43 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,27 +15,39 @@
1515
/**
1616
* 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).
18+
* The napi shim is required LAZILY on first `getSeaNative()` call so
19+
* importing this module is free for callers that never reach a SEA
20+
* code path. Concretely: a test or build that exercises only the
21+
* Thrift backend, or a Mocha file evaluation that only checks an
22+
* env-var skip-gate, does not need the platform-specific `.node`
23+
* artifact at module-load time. The eager `require()` we previously
24+
* had at the top of this file crashed test discovery with
25+
* `MODULE_NOT_FOUND` before `before()` skip-gates could fire — a
26+
* defect that propagated forward into every e2e file that imported
27+
* anything from this module (DA round-1 H1 against F2 and F4).
2228
*
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.
29+
* The require result is memoised on first call so subsequent
30+
* `getSeaNative()` calls are O(1). Failures are surfaced verbatim:
31+
* the napi-rs auto-generated shim already produces a descriptive
32+
* `MODULE_NOT_FOUND` listing the platform-arch tuple it tried, which
33+
* is the actionable diagnostic for "run `yarn build:native` first."
34+
* The structured platform/arch diagnostics planned in the file's
35+
* earlier doc-comment remain a follow-on; the lazy-load addresses
36+
* the immediate defect.
2837
*/
2938

3039
import type { SeaNativeConnectionOptions } from './SeaAuth';
3140

32-
// The path is relative to this file at runtime (`dist/sea/SeaNativeLoader.js`)
33-
// resolving to `dist/sea/../../native/sea/index.js` once `tsc` has emitted
34-
// to `dist/`. We use a require-time path resolution because the napi
35-
// shim is plain CommonJS and not part of the TS source tree.
41+
// Memoised require slot. `undefined` = not yet loaded; on first
42+
// `getSeaNative()` we resolve and cache. `null` is not used —
43+
// failures throw out of the require call rather than yielding null.
3644
//
37-
// eslint-disable-next-line @typescript-eslint/no-var-requires, import/no-dynamic-require, global-require
38-
const native = require('../../native/sea/index.js');
45+
// The path is relative to this file at runtime
46+
// (`dist/sea/SeaNativeLoader.js`) resolving to
47+
// `dist/sea/../../native/sea/index.js` once `tsc` has emitted to
48+
// `dist/`. The napi shim is plain CommonJS, not part of the TS source
49+
// tree, so the path is require-time resolved.
50+
let nativeBindingCache: SeaNativeBinding | undefined;
3951

4052
/**
4153
* Arrow IPC payload returned by `Statement.fetchNextBatch()`. Carries a
@@ -106,9 +118,24 @@ export interface SeaNativeBinding {
106118
* Returns the loaded native binding. Throws if the platform-specific
107119
* `.node` artifact cannot be found (napi-rs's auto-generated shim
108120
* surfaces a descriptive error in that case).
121+
*
122+
* Lazy + memoised: the first call resolves
123+
* `../../native/sea/index.js` and caches the result; subsequent calls
124+
* are O(1). Importing this module no longer eagerly loads the .node
125+
* artifact, so callers that never reach a SEA code path don't pay the
126+
* load cost or crash if the artifact is absent (DA round-1 H1 fixup).
109127
*/
110128
export function getSeaNative(): SeaNativeBinding {
111-
return native as SeaNativeBinding;
129+
if (nativeBindingCache === undefined) {
130+
// The `.js` extension is required: the napi-rs shim is plain
131+
// CommonJS, not a TS source file, so the extension cannot be
132+
// resolved away. The full eslint disable comment covers the
133+
// file-extension, dynamic-require, var-requires, and global-require
134+
// rules that all flag this otherwise-necessary pattern.
135+
// eslint-disable-next-line @typescript-eslint/no-var-requires, import/no-dynamic-require, global-require, import/extensions
136+
nativeBindingCache = require('../../native/sea/index.js') as SeaNativeBinding;
137+
}
138+
return nativeBindingCache;
112139
}
113140

114141
/**

tests/e2e/sea/complex-types-e2e.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ describe('SEA complex types — native Arrow default', function suite() {
145145
expect(batchEnvelope).to.not.equal(null);
146146

147147
const table = tableFromIPC(batchEnvelope!.ipcBytes);
148-
const schema = table.schema;
148+
const { schema } = table;
149149

150150
// Each complex column should be a native Arrow nested type, not Utf8.
151151
const arrField = schema.fields.find((f) => f.name === 'c_arr');

0 commit comments

Comments
 (0)