Fix mistake in test#82
Merged
Merged
Conversation
Signed-off-by: Levko Kravets <[email protected]>
kravets-levko
requested review from
arikfr,
moderakh,
superdupershant,
susodapop and
yunbodeng-db
as code owners
This comment was marked as resolved.
This comment was marked as resolved.
moderakh
approved these changes
Oct 26, 2022
msrathore-db
added a commit
that referenced
this pull request
May 30, 2026
…DI seam, packaging, tests Rebuilds native/sea against the merged kernel main (binding renamed @databricks/sea-native -> @databricks/sql-kernel via kernel #82) and addresses the /full-review findings on PR #380: - C1: commit the napi-rs router (native/sea/index.js, un-ignored) + a prepack assertion so the publish tarball can never ship without it. Companion kernel fix (databricks-sql-kernel#93) corrects the base package name so the router require paths resolve. - C2: drop the @sea-native tsconfig path alias; use a relative import so no unresolvable specifier leaks into the emitted .d.ts. - C3/C11: error hints no longer name a 404-ing package; dlopen hint now includes the underlying dlerror string + concrete remediation. - C4: document M0 = linux-x64-gnu-only scope + npm scope-lock note. - C5: SeaNativeBinding = typeof import('../../native/sea') (no drift). - C6: SeaNativeLoader is now a class with an injectable load seam; getSeaNative/tryGetSeaNative are thin process-global wrappers. - C7: re-exports renamed Sea* to avoid colliding with Thrift types. - C8: version.test.ts fails loud on the linux-x64 CI runner + shape checks; new loader.test.ts covers the hint branches, Node gate, shape check, and caching via the injected seam. - C9: Node-version guard fails closed (NaN or < floor). - C13: e2e smoke uses the shared tests/e2e/utils/config.ts creds. - C10/C12 are resolved upstream in merged kernel main / deferred to M1. index.d.ts regenerated from merged main: ExecuteOptions dropped (catalog/schema/sessionConf are session-level on openSession), close() awaits DeleteSession, schema() is sync, sessionId/statementId getters added. Verified: 11 unit tests; e2e SELECT 1 against a live warehouse, direct and through mitmproxy (SEA REST: POST /sql/sessions -> POST /sql/statements -> DELETE /sql/sessions/<id>). Co-authored-by: Isaac
msrathore-db
added a commit
that referenced
this pull request
Jun 1, 2026
…DI seam, packaging, tests Rebuilds native/sea against the merged kernel main (binding renamed @databricks/sea-native -> @databricks/sql-kernel via kernel #82) and addresses the /full-review findings on PR #380: - C1: commit the napi-rs router (native/sea/index.js, un-ignored) + a prepack assertion so the publish tarball can never ship without it. Companion kernel fix (databricks-sql-kernel#93) corrects the base package name so the router require paths resolve. - C2: drop the @sea-native tsconfig path alias; use a relative import so no unresolvable specifier leaks into the emitted .d.ts. - C3/C11: error hints no longer name a 404-ing package; dlopen hint now includes the underlying dlerror string + concrete remediation. - C4: document M0 = linux-x64-gnu-only scope + npm scope-lock note. - C5: SeaNativeBinding = typeof import('../../native/sea') (no drift). - C6: SeaNativeLoader is now a class with an injectable load seam; getSeaNative/tryGetSeaNative are thin process-global wrappers. - C7: re-exports renamed Sea* to avoid colliding with Thrift types. - C8: version.test.ts fails loud on the linux-x64 CI runner + shape checks; new loader.test.ts covers the hint branches, Node gate, shape check, and caching via the injected seam. - C9: Node-version guard fails closed (NaN or < floor). - C13: e2e smoke uses the shared tests/e2e/utils/config.ts creds. - C10/C12 are resolved upstream in merged kernel main / deferred to M1. index.d.ts regenerated from merged main: ExecuteOptions dropped (catalog/schema/sessionConf are session-level on openSession), close() awaits DeleteSession, schema() is sync, sessionId/statementId getters added. Verified: 11 unit tests; e2e SELECT 1 against a live warehouse, direct and through mitmproxy (SEA REST: POST /sql/sessions -> POST /sql/statements -> DELETE /sql/sessions/<id>). Co-authored-by: Isaac
msrathore-db
added a commit
that referenced
this pull request
Jun 1, 2026
…DI seam, packaging, tests Rebuilds native/sea against the merged kernel main (binding renamed @databricks/sea-native -> @databricks/sql-kernel via kernel #82) and addresses the /full-review findings on PR #380: - C1: commit the napi-rs router (native/sea/index.js, un-ignored) + a prepack assertion so the publish tarball can never ship without it. Companion kernel fix (databricks-sql-kernel#93) corrects the base package name so the router require paths resolve. - C2: drop the @sea-native tsconfig path alias; use a relative import so no unresolvable specifier leaks into the emitted .d.ts. - C3/C11: error hints no longer name a 404-ing package; dlopen hint now includes the underlying dlerror string + concrete remediation. - C4: document M0 = linux-x64-gnu-only scope + npm scope-lock note. - C5: SeaNativeBinding = typeof import('../../native/sea') (no drift). - C6: SeaNativeLoader is now a class with an injectable load seam; getSeaNative/tryGetSeaNative are thin process-global wrappers. - C7: re-exports renamed Sea* to avoid colliding with Thrift types. - C8: version.test.ts fails loud on the linux-x64 CI runner + shape checks; new loader.test.ts covers the hint branches, Node gate, shape check, and caching via the injected seam. - C9: Node-version guard fails closed (NaN or < floor). - C13: e2e smoke uses the shared tests/e2e/utils/config.ts creds. - C10/C12 are resolved upstream in merged kernel main / deferred to M1. index.d.ts regenerated from merged main: ExecuteOptions dropped (catalog/schema/sessionConf are session-level on openSession), close() awaits DeleteSession, schema() is sync, sessionId/statementId getters added. Verified: 11 unit tests; e2e SELECT 1 against a live warehouse, direct and through mitmproxy (SEA REST: POST /sql/sessions -> POST /sql/statements -> DELETE /sql/sessions/<id>). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]>
msrathore-db
added a commit
that referenced
this pull request
Jun 1, 2026
#380) * sea-napi-binding: scaffold native/sea/ crate with version() smoke test 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 <[email protected]> * sea-napi-binding: Database/Connection/Statement/ResultStream methods 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 <[email protected]> * sea-napi-binding: cleanup — drop unused tracing deps; address bloat findings 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 <[email protected]> * sea-napi-binding: relocate Rust source to kernel workspace 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 <[email protected]> * sea-napi-binding: build:native uses --platform so index.js router is generated Signed-off-by: Madhavendra Rathore <[email protected]> * 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). Signed-off-by: Madhavendra Rathore <[email protected]> * sea-napi-binding: address review — sql-kernel rename, loader class + DI seam, packaging, tests Rebuilds native/sea against the merged kernel main (binding renamed @databricks/sea-native -> @databricks/sql-kernel via kernel #82) and addresses the /full-review findings on PR #380: - C1: commit the napi-rs router (native/sea/index.js, un-ignored) + a prepack assertion so the publish tarball can never ship without it. Companion kernel fix (databricks-sql-kernel#93) corrects the base package name so the router require paths resolve. - C2: drop the @sea-native tsconfig path alias; use a relative import so no unresolvable specifier leaks into the emitted .d.ts. - C3/C11: error hints no longer name a 404-ing package; dlopen hint now includes the underlying dlerror string + concrete remediation. - C4: document M0 = linux-x64-gnu-only scope + npm scope-lock note. - C5: SeaNativeBinding = typeof import('../../native/sea') (no drift). - C6: SeaNativeLoader is now a class with an injectable load seam; getSeaNative/tryGetSeaNative are thin process-global wrappers. - C7: re-exports renamed Sea* to avoid colliding with Thrift types. - C8: version.test.ts fails loud on the linux-x64 CI runner + shape checks; new loader.test.ts covers the hint branches, Node gate, shape check, and caching via the injected seam. - C9: Node-version guard fails closed (NaN or < floor). - C13: e2e smoke uses the shared tests/e2e/utils/config.ts creds. - C10/C12 are resolved upstream in merged kernel main / deferred to M1. index.d.ts regenerated from merged main: ExecuteOptions dropped (catalog/schema/sessionConf are session-level on openSession), close() awaits DeleteSession, schema() is sync, sessionId/statementId getters added. Verified: 11 unit tests; e2e SELECT 1 against a live warehouse, direct and through mitmproxy (SEA REST: POST /sql/sessions -> POST /sql/statements -> DELETE /sql/sessions/<id>). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]> * fix(sea): sync package-lock for napi-rs/cli + skip native smoke test until binding ships CI was red on every job at `npm ci` (EUSAGE: "Missing @napi-rs/[email protected] from lock file") — package.json added @napi-rs/cli (build:native devDep) and the @databricks/sql-kernel-linux-x64-gnu optional dep, but package-lock.json was never regenerated. Updated the lock (the unpublished optional dep is recorded as optional, so `npm ci` tolerates/skips it). Also: the version smoke test fail-louded when the binding was absent on a linux-x64 CI runner, assuming the optional dep installs there. It doesn't yet — the @databricks/sql-kernel-* packages aren't published and the standard CI doesn't run build:native — so the fail-loud was spurious. Gate it behind SEA_NATIVE_EXPECTED=1 (set once a CI step provisions the binding); default to skip. The e2e smoke test already skips when the binding is absent. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]> * fix(sea): drop the unpublished sql-kernel optional dep (unblocks npm ci) `@databricks/[email protected]` is not published yet, so the lockfile recorded it with no resolved version and `npm ci` rejected the mismatch ("lock file's @databricks/sql-kernel-linux-x64-gnu@undefined does not satisfy @0.1.0") — npm ci does NOT silently skip a pinned-but- unresolvable optional dependency. Remove it from optionalDependencies until the per-triple binding packages are actually published; the loader already handles an absent binding (and version.test skips unless SEA_NATIVE_EXPECTED is set). Re-add the optional deps once the `@databricks/sql-kernel-*` packages ship. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]> * test(sea): decouple loader tests from the runner's Node version The unit-test matrix spans Node 14–20, but SeaNativeLoader's version gate read the live `process.version` before the injected `load` seam ran. On the 14 and 16 runners every "successful load" / "load-failure hints" / "shape check" test hit `requires Node >=18` instead of the path it asserted (7 failures per job). Make Node-major detection a second injectable ctor seam (default = live process.version), inject a supported major in the load-path tests, and inject the version-under-test in the gate's own tests (no more mutating process.version). Tests now pass identically on every matrix Node. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]> * fix(sea): make @napi-rs/cli optional + prettier-format loader test Two CI failures, both unrelated to driver behavior: 1. unit-test/lint/e2e intermittently failed at `npm ci` with ECONNRESET fetching @napi-rs/[email protected] from the internal npm proxy (cold-cache matrix jobs; the warm-cache node-14 job kept passing). @napi-rs/cli is only used by `build:native`, which CI never runs. Move it from devDependencies to optionalDependencies — matching the existing lz4 pattern — so a proxy hiccup is a non-fatal skip instead of failing the whole install. `build:native`'s `npx --no-install` still resolves it on dev machines where the optional install succeeds. 2. prettier flagged loader.test.ts: the two-arg `new SeaNativeLoader(cb, fn)` calls needed multi-line formatting. Ran prettier --write. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]> --------- Signed-off-by: Madhavendra Rathore <[email protected]>
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
3 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.
User agent string has a whitespace between version and comment part. While library version had a suffix - that whitespace was captured into it. But after we tagged non-beta version - it started to fail. The solution is to explicitly "eat" that whitespace