[aakash-saravanan-db]

Feature or gap

Native SQL file statements expose instructions without transferring bytes. The existing staging option must complete the file operation before result consumption.

What this PR addresses

Forward stagingAllowedLocalPath to the shared kernel, preserve zero-row completed results and map ordinary transfer failures to StagingError while preserving Cancelled/Timeout operation-state precedence and lifecycle handling. Omitting the option keeps raw instructions for caller-managed transfers.

Dependencies

Shared kernel transfers provide the execution and native interfaces. The source pin is 37c7a3e and identifies the locally tested implementation. Generated binding declarations match that source.

Pre-merge gate: native publication and dependency updates. Published 1.4.0 lacks the required interface. Publish matching packages, update this PR's dependency metadata/lockfiles, install the published artifacts and repeat validation before merge. No replacement version is assigned. Writable scheduled fixtures and non-skipped lifecycle coverage remain pending.

Tests and validation

Build, type check, Prettier, 114 execution/backend tests and 36 mapping tests pass. Rebuilt native artifacts pass synchronous/asynchronous one-MiB byte round trips, overwrite, denied paths, missing files and repeated deletion.

Acceptance protocol: five alternating measurements per side after equal warmups, direct connections, sequential one-MiB PUT/GET/REMOVE, full consumption and byte equality; connection/authentication time excluded. Thrift → native lifecycle 1.6387 → 1.5883 s (-3.07%). All comparable verb medians meet the 10% criterion.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity CHANGELOG issue. I checked the forwarding of the allowed paths (a string, an array, or an empty array), the FILE_TRANSFER_ERROR → StagingError mapping, and the interaction with DBSQLSession's Thrift staging handler. That handler never runs twice on the kernel path, because the kernel always reports isStagingOperation: false. Nits: the edited CONNECTION_PARAMETERS.md row no longer lines up with the rest of the table, and the hand-edit to the generated native/kernel/index.d.ts should be regenerated from the pinned kernel revision.

[peco-review-bot]

Verdict: 2 Nit

Looks good. No correctness issues found. stagingAllowedLocalPath is now passed to the native execute options on both the direct and submit paths. FILE_TRANSFER_ERROR maps to StagingError and keeps sqlState and metadata. The facade's Thrift staging handler is not triggered for kernel operations, because kernel metadata reports isStagingOperation: false, so a transfer is never run twice.

Other findings

• ⚪ Nit — CHANGELOG.md: the new Unreleased bullet is followed by a blank line, which splits it from the rest of the list. Other entries also end with a PR reference like (#NNN), and this one doesn't.
• ⚪ Nit — KernelErrorMapping.ts: the if (...) {...} else switch (...) shape re-indents the whole switch and makes the diff noisy. An early assignment such as if (FILE_TRANSFER_ERROR) { error = new StagingError(message); } else { switch ... }, or a helper function, would read more cleanly.

[peco-review-bot]

Verdict: 1 Nit

Looks good, with no blocking issues. The stagingAllowedLocalPath value is normalized to an array and forwarded on both the direct and submit paths, and FILE_TRANSFER_ERROR errors map to StagingError while keeping sqlState and kernelMetadata; unit tests cover both. Follow-up: as the description notes, until matching native packages are published and pinned, an older native binary would silently ignore stagingAllowedLocalPaths and return instruction rows instead of transferring files.

Other findings

• ⚪ Nit — In KernelErrorMapping.ts, the new if (...) { ... } else switch (...) form leaves the whole switch without braces and re-indents it, which bloats the diff. Either wrap the switch in else { ... } or return/assign early on the FILE_TRANSFER_ERROR branch so the existing switch can stay as it was.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Low

Looks good overall. One low concern: when a transfer error also carries a Cancelled or Timeout code, the StagingError mapping takes precedence over the cancellation/timeout error. Follow-up, not caused by this PR: whenever stagingAllowedLocalPath is set, DBSQLSession.executeStatement fetches result metadata up front, so on the kernel runAsync: true path the call blocks until the statement finishes. Nit: in the e2e test, the DROP SCHEMA in finally can hide the original assertion failure if the drop itself throws.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity test-coverage gap. Forwarding the option, mapping errors to StagingError with Cancelled/Timeout taking precedence, and leaving DBSQLSession's Thrift staging handler unused on the kernel path (isStagingOperation is always false there) all look correct. Two notes that don't anchor to a line: KERNEL_REV pins ea26771… but the PR description cites source pin b92a867, so one of them should be reconciled. The native package version/lockfile update is still an acknowledged pre-merge gate.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Nit

The code looks correct; I found no inline issues. stagingAllowedLocalPath is now passed to the kernel on both the direct and async paths, and the facade's Thrift-side staging handler does not run a second time on kernel because KernelOperationBackend always reports isStagingOperation: false. File-transfer errors now come back as StagingError, while Cancelled and Timeout still come back as OperationStateError. Before merge: the PR description says the kernel source is pinned at ea26771, but KERNEL_REV is 37c7a3ec…, so one of them needs fixing. Until matching native packages are published, an older native binding will most likely drop stagingAllowedLocalPaths without any error and return raw instruction rows with no warning — the removed warn log used to cover this case. That makes the dependency gate already listed in the description a must-do.

Other findings

• ⚪ Nit — KernelErrorShape.errorCode (KernelErrorMapping.ts) is the only field in the interface without a JSDoc line. Its siblings message and sqlstate both have one.

[aakash-saravanan-db]

The description identifies the validated 37c7a3e native source. 17f71ae also addresses the documentation nit from review 5869566218: the interface explains why the specific transfer error code accompanies the broader error category. TypeScript build and formatting pass, and rebuilt production JavaScript is byte-identical to the validated cohort. Native publication/dependency updates remain blocked in kernel #443; this PR is draft.

[peco-review-bot]

✅ No issues identified by the review bot.