chore: upgrade SQL kernel to 1.1.0 - #528
Conversation
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Straightforward kernel 1.0.0→1.1.0 bump; the mechanical version-string/lockfile/integrity changes are internally consistent. One medium concern: the new JSDoc documents that the kernel now replaces rather than appends the base User-Agent and requires the leading prefix be SEA-allow-listed — the driver sends a NodejsDatabricksSqlConnector/... prefix, so this should be verified against a live warehouse (and the now-stale append-semantics comments in KernelAuth.ts updated). A low note flags the new empty-catalog=matches-nothing semantics for listSchemas/listColumns.
| * token the SEA server keys on while still surfacing the caller's | ||
| * identity. The NodeJS driver folds its `userAgentEntry` into a | ||
| * - `User-Agent` **replaces** the `DatabricksSQLKernel/...` base. | ||
| * Its leading prefix must therefore be allow-listed by the SEA |
There was a problem hiding this comment.
🟡 Medium — This 1.1.0 bump documents a behavior change in kernel UA handling, not just a version string. The old contract was that the kernel appends the caller's User-Agent to its base UA, preserving the DatabricksJDBCDriverOSS/... / DatabricksSQLKernel/... token "the SEA server keys on". The new contract (this diff) is that the caller's User-Agent replaces the kernel base entirely, and "Its leading prefix must therefore be allow-listed by the SEA server."
The driver unconditionally pushes its own composed UA as the last User-Agent entry (lib/kernel/KernelAuth.ts:503-505), and that value starts with NodejsDatabricksSqlConnector/... (buildUserAgentString), not with any Databricks* kernel prefix. Under append semantics the kernel base token survived; under the new replace semantics the effective UA sent to SEA is now NodejsDatabricksSqlConnector/... with no kernel/JDBC prefix.
Please confirm that NodejsDatabricksSqlConnector is on the SEA server's UA allow-list. If SEA gates on the leading prefix (as this JSDoc states), kernel-path connections could be rejected server-side — something the mocked unit suite (1,312 passed) would not catch. If it is allow-listed, no code change is needed, but this should be verified against a live warehouse before release.
Relatedly, the driver's own explanatory comments now contradict the shipped kernel: lib/kernel/KernelAuth.ts:218 ("appends a User-Agent entry to its base UA rather than replacing it") and :454 ("the kernel folds the last User-Agent into its base") describe the pre-1.1.0 append behavior and are now stale. Worth updating in this PR so future readers don't rely on the wrong contract.
| listTables(catalog?: string | undefined | null, schemaPattern?: string | undefined | null, tablePattern?: string | undefined | null, tableTypes?: Array<string> | undefined | null): Promise<Statement> | ||
| /** | ||
| * Columns of tables matching the filter. | ||
| * Columns of tables matching the filter. An empty catalog matches |
There was a problem hiding this comment.
🔵 Low — The 1.1.0 JSDoc for listSchemas / listColumns now specifies that an empty catalog matches nothing. The driver forwards request.catalogName verbatim into listSchemas(request.catalogName, ...) and listColumns(request.catalogName, ...) (KernelSessionBackend.ts:365,383) with no normalization. A caller that passes an empty-string catalogName (as opposed to undefined) expecting a wildcard/all-catalogs result would now silently get zero rows on the kernel path.
Unlike getPrimaryKeys (which explicitly rejects ''/undefined at KernelSessionBackend.ts:401-407), getSchemas/getColumns neither reject nor document this. This is likely benign if callers only ever pass undefined for "all", but the empty-string edge case is now a behavior difference from the Thrift backend and is untested. Consider a unit test asserting the empty-catalog behavior, or normalizing ''→undefined if wildcard semantics are intended.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
4bb3cbb to
7cd00b9
Compare
There was a problem hiding this comment.
Verdict: 1 Low
Mechanical kernel bump 1.0.0 → 1.1.0 — version strings, lockfile hashes, package.json, KERNEL_REV, and changelog are all consistent. One doc-consistency nit: the regenerated index.d.ts now documents User-Agent as replacing the kernel base UA, but the driver's own comments in lib/kernel/KernelAuth.ts still describe the old append semantics. Executable behavior is unaffected.
| * replacing it), preserving the `DatabricksJDBCDriverOSS/...` | ||
| * token the SEA server keys on while still surfacing the caller's | ||
| * identity. The NodeJS driver folds its `userAgentEntry` into a | ||
| * - `User-Agent` **replaces** the `DatabricksSQLKernel/...` base. |
There was a problem hiding this comment.
🔵 Low — This regenerated binding doc changes the kernel's User-Agent contract from append to base UA to replace the base UA (and warns the connector's leading prefix must now be allow-listed by the SEA server). The driver's hand-authored comments in lib/kernel/KernelAuth.ts still describe the old append semantics and are now stale/incorrect:
KernelAuth.ts:218— "the kernel appends aUser-Agententry to its base UA rather than replacing it".KernelAuth.ts:454-455— "the kernel folds the lastUser-Agentinto its base UA —DatabricksJDBCDriverOSS/...".
Under 1.1.0 the connector's composed UA (NodejsDatabricksSqlConnector/..., built at KernelAuth.ts:505 via buildUserAgentString) becomes the entire effective User-Agent rather than a token folded into a DatabricksJDBCDriverOSS/... base. The headers.push code itself is fine under both semantics, so this is comment-only drift — but it's worth updating these comments alongside the binding regeneration so the next reader doesn't rely on the retired append behavior. Also worth confirming (out of the driver's control) that NodejsDatabricksSqlConnector is allow-listed server-side, since the base prefix the SEA server previously keyed on is no longer emitted.
Summary
Upgrade all eight optional kernel native packages to 1.1.0, pin source builds to the matching kernel release commit, and refresh the generated binding surface. The kernel dependency is now stable rather than experimental, as noted in the changelog.
Test plan
npm run buildenv -u CODEX_CI npm test(1,312 passed)