fix(kernel): preserve qualified interval parameter types

[cathleeny]

Summary

• Send all parameters through the kernel's raw-parameter path -- This prreserves qualified interval parameter types such as INTERVAL MONTH and INTERVAL DAY in the kernel backend.
• Add unit and end-to-end coverage for interval binding.

Testing

• npm test -- tests/unit/kernel/positionalParams.test.ts tests/unit/kernel/execution.test.ts
• npx tsc --noEmit --project tsconfig.build.json
• Local Node → rebuilt kernel N-API → mock SEA run emitted {"ordinal":1,"type":"INTERVAL MONTH","value":"2-6"}

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[cathleeny]

TODO: Pin latest kernel ref when the kernel rawParam PR is merged

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Low

Looks good — a focused, well-tested change that preserves qualified interval types (INTERVAL MONTH/INTERVAL DAY) by routing all kernel bound params through the raw-parameter path; mutual-exclusivity is still enforced and unit/e2e coverage was updated correctly. One low note: the hand-edit to the generated native/kernel/index.d.ts boundary should be confirmed against the regenerated surface for the new KERNEL_REV.

[peco-review-bot]

Verdict: 1 Low

Looks good — a focused, well-tested change that routes all kernel params through the new rawParams native surface to preserve qualified INTERVAL MONTH/INTERVAL DAY types. Correctness (mutual-exclusion guard, positionalParams ?? namedParams selection, VOID/named spreading) is sound and covered by unit + e2e tests. One low note on the CHANGELOG omitting the native-package caveat its sibling entries use.

Nits (not blocking): the preserves INTERVAL MONTH on the SEA wire e2e test asserts only caught instanceof Error, so any unrelated failure (connection, syntax) would satisfy it — the unit test covers type preservation directly and robustly, so this is acceptable but indirect. Also buildKernelPositionalParams/buildKernelNamedParams and the KernelPositionalParams.ts filename now build rawParams, so the naming trails the behavior. The unrelated tokenCachePassphrase addition in native/kernel/index.d.ts appears to be a benign re

[...truncated to keep verdict scannable]

[peco-review-bot]

Verdict: 1 Low

Looks good — a clean, well-tested fix that sends both positional and named params through the kernel raw-parameter path so INTERVAL MONTH/INTERVAL DAY types are preserved instead of collapsed to bare INTERVAL. Logic is correct (mutual-exclusion guard intact, positionalParams ?? namedParams selects the set one) and carries unit + e2e coverage. One low note on a weak e2e assertion. Minor nit (not filed inline): KernelNativeRawParameterInput is now hand-declared in KernelNativeLoader.ts because the published kernel types don't yet expose rawParams; per the file's own prior comment this reintroduces the driver/kernel contract-drift risk the typed re-exports avoided — worth replacing with the generated type once the pinned kernel publishes it.

[vuanhphung]

Can you trigger Comparator (Node) — Thrift vs SEA with this branch to verify?

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Medium

Looks solid — the rawParams routing, queryId-based SqlError classification, and vendorCode/0-code handling are internally consistent and well-tested. One medium note: StatusError moving from implements Error to extends HiveDriverError is a cross-backend, public-API-visible behavior change (affects all Thrift users via Status.assert) that the CHANGELOG doesn't mention.

[peco-review-bot]

Verdict: 1 Low

Solid, well-tested change: interval qualified types are preserved by routing through the kernel raw-parameter path, and StatusError now correctly extends HiveDriverError with SQLSTATE + vendor-code (including 0) retention. The empty-vs-non-empty queryId split (StatusError vs OperationStateError) is consistent across the direct/async paths and has good unit coverage. One low concern: the switch to rawParams-only forwarding depends on the source-pinned kernel and its CHANGELOG entry omits the published-native-bump caveat that the sibling getTypeInfo change flagged. Nit: in mapKernelErrorToJsError/StatusError, sqlState is assigned twice for the StatusError branch (constructor field + trailing defineErrorMetadata) — harmless but redundant.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Medium

Focused, well-tested change that switches kernel parameter binding to the raw-parameter path to preserve qualified INTERVAL types — logic and test coverage look solid. One medium concern: routing all params through the hand-declared rawParams field (not part of the generated napi surface) can silently drop every bound parameter on native bindings that predate the raw path, since the loader doesn't verify raw-param support and napi ignores unknown option fields. Nit: the new e2e preserves INTERVAL MONTH test asserts only caught instanceof Error, so it would pass on any execute/fetch failure, not specifically an INTERVAL-type validation error.

[peco-review-bot]

Verdict: 1 Medium · 1 Nit

Looks good — a clean, well-covered refactor to route all params through the kernel raw-parameter path so qualified INTERVAL types survive. One medium concern: forwarding only rawParams will silently drop parameters on native bindings that predate rawParams support, and the CHANGELOG omits the customer-install caveat the analogous prior entry carried. Plus a nit about two now-unused type re-exports.

Other findings

• ⚪ Nit — KernelNativeTypedValueInput and KernelNativeNamedTypedValueInput are now unreferenced anywhere in the tree (the only importer, KernelPositionalParams.ts, switched to KernelNativeRawParameterInput). If they're no longer part of the intended kernel surface, consider dropping them; otherwise a short comment noting they're retained as documentation of the codec surface would prevent a future "dead export" cleanup churn.

[peco-review-bot]

🟡 Medium — The driver now forwards parameters only through execOptions.rawParams and no longer sets positionalParams / namedParams. napi-rs deserializes object arguments field-by-field and silently ignores unknown keys — so if a consumer runs against a native binding that predates rawParams support (i.e. a published @databricks/databricks-sql-kernel-* optional dependency rather than the freshly-bumped KERNEL_REV source build), every bound parameter is silently dropped and the query executes with unbound placeholders (wrong results, not an error).

The loader comment itself acknowledges rawParams is "available in the source-pinned kernel before its published types." The CHANGELOG entry for this PR does not carry the customer-install caveat that the earlier getTypeInfo() entry did ("Customer-facing npm installs require a follow-up bump to a published native package…"). Consider either (a) adding the equivalent CHANGELOG caveat here, or (b) keeping a fallback that still sets positionalParams/namedParams when rawParams is unsupported, so older bindings don't silently drop parameters.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Looks good overall — the routing of all params through rawParams is correct (the mutual-exclusion check runs before the positionalParams ?? namedParams coalesce), the interval flattening is cleanly removed, and unit/e2e coverage is updated to match. One medium concern: the change now makes param_raw the only parameter path, so a native binding without param_raw support loses all parameter binding, not just intervals — worth a CHANGELOG caveat mirroring the sibling entry. Plus a low note to confirm the generated native/kernel/index.d.ts was regenerated, not hand-edited.

[peco-review-bot]

Verdict: 1 High

One high-severity concern: the native loader was bumped to require kernel binding 0.2.1, but package.json optionalDependencies still install 0.2.0 — which predates the rawParams/param_raw surface this fix depends on. The TypeScript changes themselves (rawParams codec, mutual-exclusion check preserved, INTERVAL type pass-through, unit + e2e coverage) are clean and well-tested.

Other findings

• 🟠 High — Native binding version mismatch: loader now demands 0.2.1, but the installed packages are pinned to 0.2.0.

This PR bumps every bindingPackageVersion !== '0.2.0' check in native/kernel/index.js to !== '0.2.1' (and the WASI check), so with NAPI_RS_ENFORCE_VERSION_CHECK set the loader now throws expected 0.2.1 but got 0.2.0. But the optionalDependencies in package.json still install the 0.2.0 native packages:

"@databricks/databricks-sql-kernel-linux-x64-gnu": "0.2.0",
... (all 8 entries 0.2.0)

Two problems follow:

1. With the version check enabled, kernel load fails outright because the installed 0.2.0 package never matches the expected 0.2.1.
2. More importantly, the whole feature depends on the new rawParams → StatementSpec::param_raw path landing in the native binding. The 0.2.0 packages that npm actually installs predate that surface, so execOptions.rawParams is sent to a binding that doesn't understand it — parameters would be silently dropped or error at runtime for customer-facing installs. (Compare the prior CHANGELOG entry, which explicitly called out that a follow-up published-native bump was required; this entry does not, and the dep pins were not bumped.)

Bump the eight optionalDependencies (and confirm the published native package at 0.2.1 carries the param_raw surface) so the driver code and the installed binding agree.

[peco-review-bot]

Verdict: 1 Medium

Looks good — the driver-side change is clean and thoroughly tested (positional/named/NULL/empty-string/qualified-INTERVAL coverage, mutual-exclusivity preserved, removed types fully cleaned up). One medium concern: the CHANGELOG entry omits the published-native-package caveat its sibling kernel entry carries, and if the pinned 0.2.0 native packages lack the new rawParams/param_raw surface, customer installs could silently drop all bound parameters.

[peco-review-bot]

🟡 Medium — This entry advertises the fix as a user-facing behavior change but omits the published-native-package caveat that the sibling kernel entry directly below it carries. The PR moves all parameter binding off positionalParams/namedParams and onto the new rawParams surface (StatementSpec::param_raw, RawParameterInput), which is a brand-new field added to the native contract in this PR (via the KERNEL_REV bump to bb4a077). The customer-installed native packages remain pinned at 0.2.0 (package.json:94-101).

If 0.2.0 predates the param_raw surface, this is worse than the getTypeInfo case: napi-rs deserializes only the object fields it knows, so an unrecognized rawParams would be silently ignored — every bound parameter would drop on the wire for customer npm installs, changing query results with no error. That is precisely the failure mode the code comment in KernelSessionBackend.executeStatement says must never be a no-op ("a dropped param would silently change results").

Please either confirm 0.2.0 already exposes param_raw (in which case the source-build routing is safe for customers), or add the same "requires a follow-up published-native-package bump" caveat + kernel PR / ticket reference this entry currently lacks.

[github-actions]

Node.js integration tests triggered. View workflow runs. The result posts back here as the "Node.js Integration Tests" check.