Skip to content

fix(kernel): preserve qualified interval parameter types - #514

Open
cathleeny wants to merge 10 commits into
mainfrom
PECOBLR-4169
Open

fix(kernel): preserve qualified interval parameter types#514
cathleeny wants to merge 10 commits into
mainfrom
PECOBLR-4169

Conversation

@cathleeny

@cathleeny cathleeny commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Collaborator Author

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

@cathleeny
cathleeny deployed to azure-prod August 26, 2026 01:29 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 01:29 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 01:29 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 01:29 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 19:54 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 19:54 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 19:54 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 19:54 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 20:05 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 20:05 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 20:05 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 20:05 — with GitHub Actions Active
@cathleeny
cathleeny marked this pull request as ready for review August 26, 2026 22:09
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 22:12 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 22:12 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 22:12 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 22:12 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown

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 peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread native/kernel/index.d.ts Outdated
@cathleeny
cathleeny marked this pull request as draft August 26, 2026 22:13
Signed-off-by: Cathleen Yan <cathleen.yan@databricks.com>
Signed-off-by: Cathleen Yan <cathleen.yan@databricks.com>
Signed-off-by: Cathleen Yan <cathleen.yan@databricks.com>
@cathleeny
cathleeny marked this pull request as ready for review August 26, 2026 22:54
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 22:56 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 22:56 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 22:56 — with GitHub Actions Active
@cathleeny
cathleeny deployed to azure-prod August 26, 2026 22:56 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown

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

1 similar comment
@github-actions

Copy link
Copy Markdown

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

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — the switch to the kernel rawParams path correctly preserves qualified INTERVAL MONTH/INTERVAL DAY types while retaining DECIMAL(p,s) and NULL→VOID handling, and the mutual-exclusivity check still fires before the positionalParams ?? namedParams coalesce. One low-severity note: the new INTERVAL e2e test passes on any thrown Error, so it can succeed for the wrong reason.

Comment thread tests/e2e/kernel/execution-e2e.test.ts
@cathleeny cathleeny added javascript Pull requests that update javascript code and removed kernel-e2e Preview-run the SEA Kernel E2E suite on this PR integration-test Trigger the cross-repo driver-test Node.js integration suite on this PR javascript Pull requests that update javascript code labels Aug 26, 2026
Signed-off-by: Cathleen Yan <cathleen.yan@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Comment thread CHANGELOG.md
Signed-off-by: Cathleen Yan <cathleen.yan@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tests/e2e/kernel/execution-e2e.test.ts
@vuanhphung

Copy link
Copy Markdown
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants