[aakash-saravanan-db]

Feature or gap

Native execution rejects Volume file statements supported by the existing backend. ExecContext must complete transfers, and QueryContext must expose raw instructions without file side effects, matching the existing backend.

What this PR addresses

Enable ExecContext transfers with the existing staging context and forward allowed paths to the shared kernel. Change native QueryContext from ErrNotSupportedByKernel to raw instructions without file effects, matching Thrift; document the effect on error-based fallbacks. Reject a missing ExecContext staging context before execution, including nested comments, comments adjacent to the keyword and accepted whitespace separators. Add a separate published-native build check, without local archive replacements. A credential-free race-checked native test covers raw execution with and without staging context and context cancellation during blocked PUT/GET.

Dependencies

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

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. Build (published kernel) currently fails with the missing setter in 1.4.0. The go.mod/go.sum bump awaits publication. Repository maintainers must make this check required; adding the job alone does not enforce branch protection.

Tests and validation

Tagged and untagged local suites pass. The current live build forces rebuilding and checks a linker map against the archive and executable hashes. The real watcher/C ABI test returns context.Canceled during blocked PUT/GET and preserves the cancelled GET destination. Live tests verify byte equality, overwrite, allowed/denied paths, missing files and deletion. Matched raw GET/REMOVE controls confirm QueryContext has no transfer side effects; relative-root cases also pass.

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.6290 → 1.6577 s (+1.76%). All comparable verb medians meet the 10% criterion. Earlier +10.31% lifecycle and +10.87% GET cohorts remain preserved in kernel #442, with partial attribution and a proposed timing investigation. Historical native-revision claims are qualified because Go cached an executable after its external archive changed; current acceptance uses a forced rebuild and checked linker map. The full JSON/CString/native setter path measured 414.4 ns per call in a separate diagnostic and does not explain those gaps.

[peco-review-bot]

Verdict: 1 Medium · 1 Low · 3 Nit

Mostly sound wiring of native PUT/GET/REMOVE through ExecContext. One medium concern: user-facing docs and the kernel example still describe staging as rejected with ErrNotSupportedByKernel. The other findings are minor: error typing, test coverage and an unconditional FFI call.

Other findings

• ⚪ Nit — The rewritten test only covers the TransferFiles: true + missing-context rejection, and it drops the handle-less Close contract checks the old test had (closed=false, nil error). Other paths now behave differently and have no unit coverage:
• TransferFiles: false (QueryContext) with a staging statement and no context must reach execute, not be rejected.
• A REMOVE or comment-prefixed PUT with TransferFiles: true must still be rejected.
• conn.ExecContext and conn.QueryContext must pass true and false respectively to runQuery.

A test table in this function, plus a fake-backend assertion on ExecRequest.TransferFiles in the root package, would lock these in.

• ⚪ Nit — With TransferFiles true, every ExecContext call (INSERT, DDL and so on, not just PUT/GET/REMOVE) now marshals the paths and makes an extra FFI call to kernel_statement_set_staging_allowed_local_paths. With no staging context it sends "[]". It is cheap but unconditional, and it makes every Exec depend on the new symbol and on that call succeeding. A setter error here also fails an unrelated INSERT. Consider guarding with isStagingStatement(req.Query), or only calling it when paths are non-empty, unless the kernel requires an explicit empty list to deny transfers.
• ⚪ Nit — The new retain_result parameter is undocumented in the header comment, and the Go side always passes false. Please document what retain_result controls, e.g. whether the instruction row stays readable after the transfer, so the hard-coded false in operation.go can be checked against the kernel contract.

[peco-review-bot]

🟡 Medium — This turns kernel staging from rejected (wrapping ErrNotSupportedByKernel) into supported, but the user-facing docs still describe the old behavior. examples/kernel/main.go:91-99 still says staging is "rejected at execute" and branches on errors.Is(err, dbsqlerr.ErrNotSupportedByKernel). That branch can no longer match: the example passes no staging context, so it now gets the plain "staging ctx must be provided" error. Please update the example, and any README/CONNECTION_PARAMETERS rows that list staging as kernel-unsupported, in the same PR. That way callers learn they need driverctx.NewContextWithStagingInfo and stop relying on the sentinel.

[aakash-saravanan-db]

Updated the kernel example to pass driverctx.NewContextWithStagingInfo when optional upload paths are provided. Added the ExecContext and QueryContext contracts to CONNECTION_PARAMETERS.md and the feature to CHANGELOG.md.

[peco-review-bot]

Thanks for the update, but the checked-out head doesn't show it yet. examples/kernel/main.go:92-98 still has the "rejected at execute" comment, still branches on errors.Is(err, dbsqlerr.ErrNotSupportedByKernel), and still prints "staging is not supported on the kernel backend (use Thrift for staging)". I couldn't find a NewContextWithStagingInfo call anywhere in the example. A search of CHANGELOG.md also turned up no new kernel staging entry. Those changes may not have been pushed yet. Once a commit with the example and doc changes lands, this will be checked again.

[aakash-saravanan-db]

The stale unsupported-transfer example is removed. CONNECTION_PARAMETERS.md and CHANGELOG.md describe ExecContext transfers, required staging context, QueryContext raw instructions, and the native publication dependency. Tested changes are at 484a2f6.

[peco-review-bot]

Thanks. The CHANGELOG.md entry (line 4) and the new "Volume file transfers" section in CONNECTION_PARAMETERS.md (around line 241) both look good. The example hasn't been updated yet, though: examples/kernel/main.go:92-98 in the checked-out head still has the "rejected at execute" comment, still branches on errors.Is(err, dbsqlerr.ErrNotSupportedByKernel), and still prints "use Thrift for staging". The head may not include 484a2f6 yet. If it does, that block still needs to be removed or switched to driverctx.NewContextWithStagingInfo. I'll check again on the next push.

[peco-review-bot]

Verdict: 1 Medium · 1 Low · 1 Nit

The wiring is correct, with one medium test-coverage gap: nothing tests the new TransferFiles flag on the QueryContext side or in connection.go. On the kernel backend, ExecContext now sends allowed paths to the kernel, and IsStaging=false correctly skips the Go transfer code. Missing-context behaviour matches the Thrift staging ctx must be provided. error.

Other findings

• ⚪ Nit — The doc comment on TransferFiles only explains what happens when the flag is false, and it names the caller rather than the meaning. Read on its own, "QueryContext exposes transfer instructions without touching local files" sounds like setting the field enables instruction-only mode, which is the opposite of what it does. Suggest something like: "TransferFiles asks the backend to perform PUT/GET/REMOVE local file transfers itself (ExecContext). When false (QueryContext), staging statements return their transfer-instruction rows untouched."

[peco-review-bot]

Verdict: 2 Medium · 2 Low

The wiring looks correct (ExecContext transfers files, QueryContext returns instructions, and a missing staging context is rejected before execution), but I have 2 medium and 2 low concerns. The main ones: every kernel ExecContext now pays for the new path-configuration call, even for non-staging statements, and the forwarding of allowed paths to the kernel has no meaningful test.

[peco-review-bot]

🔵 Low — The vendored header now declares kernel_statement_set_staging_allowed_local_paths, and execute calls it unconditionally whenever TransferFiles is set (i.e. every ExecContext). Any databricks_kernel build linked against a currently published native module (without databricks-sql-kernel#439) will fail to link or resolve the symbol. The PR description acknowledges this. Consider gating the merge, or the CHANGELOG entry, on the native module being published, so main stays buildable with released bindings.

[aakash-saravanan-db]

The PR description explicitly blocks merging until native packages containing kernel #439 are published and go.mod is updated. KERNEL_REV pins the source used for local validation; no unpublished version is invented.

[peco-review-bot]

Thanks, it helps to know the PR description already blocks the merge. That block is a process step, though, and nothing in the code enforces it. Right now execute calls the new symbol unconditionally, and go.mod still points at the published native module, so merging today would break builds that use the released bindings. I'll leave this open as a reminder. It can be resolved once the go.mod bump to native packages that include kernel #439 is pushed to this branch.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with two low-severity notes: the new allowed-paths setter doesn't evict the conn on a session-fatal error the way its sibling setters do, and the new transfer path has no automated end-to-end test. Nits: the json.Marshal error on []string can't actually fail, and it's returned unwrapped; removing the staging demo leaves a stray blank line before the closing brace in examples/kernel/main.go.

[peco-review-bot]

Verdict: 2 Low

Looks good, with two low-severity notes. Both the ExecContext and QueryContext paths pass the new TransferFiles flag correctly. A missing staging context still fails before execution, and the imports still compile after the rejection code was removed.

[peco-review-bot]

Verdict: 2 Low

The change looks correct. Kernel ExecContext forwards the staging allowlist, and QueryContext keeps returning raw instructions. I found two low-severity issues, plus a merge-gate reminder: KERNEL_REV and the header pin an unreleased native revision, and go.mod still has to be bumped before merge. Nits: the isStagingStatement doc comment no longer says what the function detects (that it is case-insensitive and skips leading comments), and examples/kernel/main.go now ends main with a stray blank line.

[peco-review-bot]

Verdict: 1 Low · 1 Nit

Looks good overall: there are two low-severity test-coverage gaps and no correctness problems. The ExecContext/QueryContext wiring is right. The missing-staging-context check keeps the Thrift error message. Always setting the allowed-paths option on ExecContext statements ([] when no context is given) avoids silent no-op transfers when the staging-statement detector misses one.

Other findings

• ⚪ Nit — No unit test covers the TransferFiles branch in execute. Nothing checks that a nil or empty staging context is encoded as [] (deny) rather than skipped, which would fall back to raw instructions and make ExecContext report success with no file moved. Nothing checks that QueryContext leaves the option unset either. TestQueryTransferInstructionsNeedNoStagingContext only shows that the pre-check guard is skipped; it fails at new_statement before this block runs. The only coverage is the credential-gated e2e test. Moving the paths-to-JSON choice into a pure helper with a CGO-free test, like paramBindArg/bindparams_test.go do, would lock in the safety property that an empty list means deny.

[peco-review-bot]

Verdict: 2 Low

Looks good overall: the ExecContext/QueryContext split is clean, and the kernel denies transfers when no allowed paths are given. Two low-severity notes: the paths setter runs on every non-staging ExecContext statement, and the unit tests don't cover the new setter path. Merge gate: the header declares kernel_statement_set_staging_allowed_local_paths, so tagged builds will fail to link until go.mod points to a published native package that includes kernel#439. As the PR description says, that must happen before merge.

[peco-review-bot]

Verdict: 1 Low · 1 Nit

Looks good overall, with two low-severity concerns. The ExecContext/QueryContext split via TransferFiles is wired correctly, and the missing-staging-context guard keeps the same error as the Thrift path. One merge-gate item that has no line to anchor to: go.mod / native bindings still need to be bumped to a published release that has the new kernel_statement_set_staging_allowed_local_paths symbol before this merges.

Other findings

• ⚪ Nit — TestQueryTransferInstructionsNeedNoStagingContext only shows that a zero-value KernelBackend gets past the Go-side guard and then fails at new_statement because there is no session. It doesn't check the behavior the name promises: that kernel_statement_set_staging_allowed_local_paths is not called when TransferFiles is false. If someone removed the if req.TransferFiles gate in execute, this test would still pass, because it fails before that code runs. It also depends on the wording of the native error message. The live e2e test does cover raw instructions, but it is credential-gated. Consider pulling the path-config decision into a small pure function that a unit test can check, or rename the test to say what it actually covers (the pre-execute guard).

[peco-review-bot]

Verdict: 1 High · 1 Nit

The design is sound, but there is one blocker: the new C symbol is called on every kernel ExecContext, and go.mod still pins native bindings v1.4.0, so tagged builds won't link until the bindings are bumped. Nits: the json.Marshal error on the paths encoding is returned without a kernel: prefix, unlike the other errors in this function (the marshal practically can't fail).

Other findings

• ⚪ Nit — The new Go-side wiring has no unit coverage. That wiring is: nil paths become [] (an empty list denies transfers, while leaving the option unset returns raw instructions), JSON encoding of the allowed paths, retain_result=false, and closing stmt on a setter error. The unit tests cover only the pre-execution guard and the TransferFiles flag passed through from conn. Everything else depends on the credential-gated e2e test. If a regression made this step skip the setter when the staging context is empty, an undetected staging statement under ExecContext would quietly return raw instructions with IsStaging()==false, which is the silent no-op data-loss path the old rejection was meant to prevent. Consider moving the paths → JSON decision into a small pure helper (like paramBindArg) and testing it under CGO_ENABLED=0.

[peco-review-bot]

🟠 High — execute now calls C.kernel_statement_set_staging_allowed_local_paths on every ExecContext (whenever req.TransferFiles is true), not only on staging statements. That symbol comes from kernel#439, but go.mod still pins the native bindings at v1.4.0, which doesn't have it. So any databricks_kernel-tagged build that uses the published modules will fail to link. That breaks the whole kernel backend, not just volume transfers. The PR description calls this a merge gate, but nothing in the code enforces it. Please bump the databricks-sql-kernel-bindings/lib/* requires in go.mod to the release that contains kernel#439 as part of this PR (or in a blocking follow-up) before merging, so the KERNEL_REV/header pin and the linked archive can't get out of sync.

[aakash-saravanan-db]

Confirmed. Published v1.4.0 archives cannot link this new symbol. The local tagged build and live tests use an archive built from the pinned source; that validates implementation only. Kernel#443 tracks publishing the new native modules and updating every go.mod requirement/sum in this PR before merge. No version is assigned, so this remains an unresolved merge gate and the PR is not ready to merge.

[peco-review-bot]

Thanks for confirming, and for linking the tracking issue in kernel#443. The go.mod bump hasn't happened yet, so this finding is still open and works as the merge gate. The thread stays unresolved until the databricks-sql-kernel-bindings/lib/* requires and go.sum are updated to a published release that contains kernel#439. Once that's pushed, I'll re-check it.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Mostly sound, with one medium test-coverage gap. Native ExecContext transfers are wired correctly, and QueryContext keeps returning raw instructions. The main gaps are unit coverage for the new native allowed-paths configuration and a brittle FFI-dependent test. The merge gate (publishing the native module and bumping go.mod for the new C symbol) is still open, as the description notes.

[peco-review-bot]

🟡 Medium — The new kernel_statement_set_staging_allowed_local_paths branch has no unit coverage: there are no tests for the JSON encoding of nil vs. empty paths, the failure path (where stmt should be closed and evictIfSessionFatal called), or a check that QueryContext never sets the option. Only the credential-gated e2e test reaches this code, so a regression here (for example, sending null instead of [] when the context has no paths, which according to the header comment switches back to caller-managed transfers and silently returns instructions from ExecContext) would get through CI. The guard in Execute only catches statements that isStagingStatement recognizes. For anything it misses, the [] deny-list is the only thing preventing a silent no-op success. Consider pulling the path-encoding step into a pure-Go helper (as staging.go already does) and adding a test that asserts nil→[].

[aakash-saravanan-db]

The real native option boundary is covered by the kernel's C ABI option tests and non-ignored ADBC set/clear/invalid-value test. The Go connection test checks ExecContext forwards TransferFiles=true and QueryContext false; the credential-gated lifecycle checks transferred bytes, denied paths, and actual raw QueryContext rows. A separate matched Thrift/native lifecycle also passed with relative and parent-directory roots. Nil is explicitly normalized to [] before marshaling. The setter-error branch frees the CString, closes the statement, and invokes evictIfSessionFatal. A pure marshaling helper would not exercise setter failure or native resource cleanup, so I have not added a test-only production seam. The coordinated native publication and go.mod/go.sum update in kernel #443 remain an explicit pre-merge gate.

[peco-review-bot]

Thanks for the detailed breakdown. I agree the nil→[] normalization is in place and the setter-error branch cleans up correctly. My concern is narrower than that: none of the coverage you list runs in this repo's default CI. The C ABI and ADBC tests live in the kernel repo, the lifecycle tests need credentials, and the connection test only checks the TransferFiles flag. Nothing here would fail if someone later removed the paths == nil guard. A small helper like encodeStagingPaths(ctx) ([]byte, error), with a test that asserts nil gives [], wouldn't be a test-only seam. It would be the same encoding step Execute already does, and it would protect the one case that turns a denied transfer into a silent success. You're right that it wouldn't cover setter failure or native cleanup. That's fine; this one assertion is the part that matters most. If you still decide not to add it, I'd suggest saying so in the PR description, next to the kernel #443 gate, so reviewers can see that the gap is intentional.

[peco-review-bot]

Verdict: 1 High · 2 Low

Mostly sound — the routing is clean (ExecContext transfers files, QueryContext keeps raw instructions), and the missing-staging-context check still runs before execution. The main risk is the acknowledged native-bindings merge gate, plus two low items: the allowed-paths setup now runs on every ExecContext statement, and the new connection test only covers the flag plumbing.

[peco-review-bot]

🟠 High — The vendored header now declares kernel_statement_set_staging_allowed_local_paths, and operation.go calls it unconditionally on every ExecContext. But go.mod still pins the prebuilt databricks-sql-kernel-bindings/lib/* modules at v1.4.0, and those predate databricks-sql-kernel#439. A databricks_kernel-tagged build against the published archives will fail to link with an undefined symbol. The PR description already lists this as a merge gate. Flagging it inline so the PR can't merge until the bindings bump lands with it (and CHANGELOG notes the minimum bindings version).

[aakash-saravanan-db]

Agreed and still open: kernel #443 tracks publication plus the go.mod/go.sum updates in this PR before merge. Published native 1.4.0 lacks this symbol, so tagged builds using those archives cannot link. The validated local build replaces the archive with one built from the pinned kernel source; it does not validate the declared published dependency. No release version is assigned or invented.

[peco-review-bot]

Thanks for confirming. The finding stays open as a merge gate until the published bindings that include this symbol are released and go.mod/go.sum in this PR are bumped to them. When that happens, please also add a CHANGELOG note giving the minimum bindings version for databricks_kernel-tagged builds. Your local build against a source-built archive is useful, but you're right that it doesn't check the published dependency the PR declares, so the link failure is still there for consumers today.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Mostly sound — 1 medium and 1 low concern. ExecContext/QueryContext now route through a TransferFiles flag cleanly, the missing-context pre-check fails closed, and Thrift behavior is unchanged. The main risk is that every kernel Exec now depends on a native symbol that isn't in the bindings pinned in go.mod.

[peco-review-bot]

🟡 Medium — This branch runs on every kernel ExecContext, including plain INSERT/UPDATE/DDL, not only staging statements. As a result, every databricks_kernel build now links kernel_statement_set_staging_allowed_local_paths unconditionally. go.mod still pins databricks-sql-kernel-bindings/lib/* at v1.4.0, and those archives don't contain this symbol. So any tagged build that uses the published bindings rather than a locally built archive won't link, which breaks every kernel user, not just volume users. The PR description lists this as a merge gate, but nothing in the code or build enforces it. Before merge, please either bump the bindings in go.mod in this PR, or add a check (CI or Makefile) that KERNEL_REV matches the pinned bindings.

There's a second-order cost: each non-staging Exec now pays an extra json.Marshal, a C string allocation, and an FFI call. Setting [] on every statement is a reasonable fail-closed default against isStagingStatement false negatives. Still, if the open 10% slowdown traces back here, consider applying it only when isStagingStatement matches, and relying on the kernel's default to deny transfers. Before changing it, confirm what the kernel default does when the option is left unset.

[aakash-saravanan-db]

Confirmed; the published-archive linkage gate remains unresolved in kernel #443. go.mod/go.sum must be updated in this PR before merge once a real release is assigned. The local archive replacement used for validation is explicitly recorded and does not validate published v1.4.0. On the separate setter cost, the complete JSON/CString/thread-lock/native call measured 414.4 ns median over five 10,000-call blocks after 10,000 warmups. The kernel default leaves raw instructions available; [] enables completion while denying local files. This is why ExecContext configures the option unconditionally.

[peco-review-bot]

Thanks for confirming, and for the setter numbers: about 414 ns per call is small, so I'm dropping the cost question. I'm keeping this open because the main issue is still there. The current snippet now guards the setter with if req.TransferFiles, but that doesn't help with linking. The cgo reference to kernel_statement_set_staging_allowed_local_paths is still compiled into every databricks_kernel build. With go.mod still pinned to bindings v1.4.0, builds that use the published archives will still fail to link. Also, your reply says ExecContext sets the option on every statement, but the code at line 82 now only sets it when req.TransferFiles is true. Please check that the comment and the behaviour say the same thing. Before merge, this needs either the go.mod/go.sum bump or a CI or Makefile check that KERNEL_REV matches the pinned bindings.

[peco-review-bot]

🔵 Low — The new test only covers TransferFiles: true with no staging context. Two parts of the new contract have no unit test:

• The TransferFiles: false path (QueryContext) must not be rejected when the staging context is missing. This is the behavior that replaced the old blanket rejection, and if someone drops the req.TransferFiles && guard, no unit test fails.
• No unit test checks that a context holding allowed paths gets past the pre-check.

Only the credential-gated e2e test exercises either path. Please add a table entry such as {query, TransferFiles:false} and assert that the pre-check error does not appear. With a zero-value KernelBackend the call will still fail later, but the assertion can check the error message.

[aakash-saravanan-db]

The earlier zero-session test was removed after review comment 4544844987 identified its dependence on a native new_statement(NULL) error string. Restoring that pattern would reproduce the same test-quality issue. TestConnectionTransferMode now delegates through a real handle-less Operation and covers both routing flags. The durable TestKernelE2EVolumeFileLifecycle also verifies missing context, positive allowed paths, transferred bytes, and the actual raw GET instruction without side effects; those cases passed locally. The native option boundary is covered by C ABI and non-ignored ADBC tests. Kept this behavioral coverage without a test-only production seam.

[peco-review-bot]

I agree that tests shouldn't depend on the native new_statement(NULL) error string, but the assertion I suggested doesn't. It only checks that the error does not contain "staging ctx must be provided", so whatever the native layer returns later doesn't matter. TestConnectionTransferMode runs against a fake backend that just records req.TransferFiles (connection_test.go:985). That proves the flag is passed through, but it never reaches the guard at internal/backend/kernel/backend.go:894. That leaves the req.TransferFiles && condition and the allowed-paths case untested outside the credential-gated e2e test, which doesn't run in normal CI. It's low priority, so I'll leave the call to a maintainer, but the gap is still there.

[peco-review-bot]

Verdict: 2 Low

The wiring looks correct: ExecContext always sets the allowed-paths option, using an empty list when there is no staging context, so a staging statement the detector misses is still denied. QueryContext keeps returning raw instructions. I found two low-severity test-coverage gaps. Not mergeable yet: as the PR description says, the vendored header now calls kernel_statement_set_staging_allowed_local_paths, which published 1.4.0 lacks, so -tags databricks_kernel builds against the go.mod modules fail to link until a new native release ships and go.mod/go.sum are bumped.

[peco-review-bot]

🔵 Low — The new test only covers TransferFiles: true with no staging context. Nothing checks the other branch of the guard at backend.go:894: when TransferFiles is false (the QueryContext path), a staging statement must get past the pre-check so raw instructions come back. If a refactor dropped the req.TransferFiles && term, QueryContext would quietly break, and only the credential-gated e2e test would catch it. A CGO-free test could show that Execute with TransferFiles: false and a staging query does not return the staging ctx must be provided error. For example, use a nil-session backend and assert that the error, if any, is something else. That test should also cover a lowercase keyword such as get ..., since the table-driven test only uses uppercase queries.

[aakash-saravanan-db]

The connection test asserts the captured flags are exactly [true, false], and the live raw GET/REMOVE controls verify that QueryContext returns instructions without file side effects. These tests cover the public contract. A nil-session test that relies on a later failure being different from an error string would couple coverage to an invalid backend state; I am retaining the existing flag and real-API coverage.

[peco-review-bot]

Agreed on one point: the connection test pins the flags that connection.go sends, so the QueryContext → TransferFiles: false wiring is covered. That test runs against a fake backend that only records req.TransferFiles, though, so it never reaches the guard at kernel/backend.go:894. The only TransferFiles reference in kernel tests is the true case at kernel_test.go:538. If the req.TransferFiles && term were dropped, the connection test would still pass, and only the credential-gated live GET/REMOVE tests would fail. I take the point that asserting on a later nil-session error is brittle. One way around it is to move the pre-check into a small pure helper, for example requiresStagingCtx(req, ctx) bool, and table-test it with both flag values and a lowercase keyword. That would cover the gap without relying on invalid backend state. It's low severity and your call, but the gap I flagged is still open.

[peco-review-bot]

Verdict: 1 Low · 1 Nit

Looks good overall, with 2 low findings, both about test robustness. The routing is correct: ExecContext sets TransferFiles, QueryContext keeps the raw instruction rows, the missing-context check matches Thrift's error, and the cleanup on the setter error path is right. Merging is still blocked on the native release and go.mod/go.sum update, as the PR description says; until then the new Build (published kernel) job is expected to fail.

Other findings

• ⚪ Nit — The denied-path and missing-file PUT cases match on the kernel's free-text error messages ("restricted", "File transfer failed"). Those strings belong to the separate databricks-sql-kernel repo, so a wording change there will break this e2e test with no driver change. If the kernel exposes a status code or category for these failures (e.g. KernelError / dbsqlerrint.CategoryFromError returning a permission or invalid-request category), assert on that instead, or at least keep the substring match narrow and document where it comes from.

[peco-review-bot]

Verdict: 2 Low

Looks reasonable, with two low concerns: whether ctx cancellation still works during kernel-native transfers, and unit coverage of the new TransferFiles gate. As the PR description says, merging is blocked on the native release: the tagged build links against kernel_statement_set_staging_allowed_local_paths, which published 1.4.0 lacks, and go.mod/go.sum still need updating.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low-severity test-robustness issue. The ExecContext/QueryContext transfer-mode wiring, the check that a staging context is present before execution, and statement cleanup on the new setter's error path all follow the existing patterns in execute. Before merge, the published-kernel CI job still has to pass with the new native packages.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with two low-severity notes. The ExecContext/QueryContext split via TransferFiles is wired correctly, the kernel's IsStaging=false keeps the connection layer from transferring the same file twice, and the docs and examples match the new behavior. One parity gap remains: the kernel only checks for a staging context when it recognizes the SQL as staging, while Thrift checks every operation the server marks as staging.

[peco-review-bot]

🔵 Low — defer k.CloseSession(context.Background()) drops the returned error. If errcheck runs on test files, lint will fail here. Elsewhere the repo uses //nolint:errcheck or _ = for this pattern, e.g. defer db.Close() //nolint:errcheck. Either use defer func() { _ = k.CloseSession(context.Background()) }() or add the nolint annotation.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 2 Low

The wiring looks correct, with 2 low-severity test findings. ExecContext sets TransferFiles=true and QueryContext sets false; the kernel reads staging paths only when transferring and fails early when the staging context is missing, with the same error the Thrift path returns. As the PR description says, this must not merge until the native bindings with kernel_statement_set_staging_allowed_local_paths are published and go.mod/go.sum are updated: until then the new "Build (published kernel)" job and every tagged build against the published 1.4.0 modules fail to link.

[peco-review-bot]

Verdict: 1 Low

Looks good overall. One low finding: the CHANGELOG wording misses that kernel-backed QueryContext also changed behaviour. The ExecContext/QueryContext split is wired through cleanly, the missing-staging-context check matches the Thrift error, and sending the allowed-paths option (an empty list if none) on every kernel ExecContext stops raw instructions from counting as a successful transfer.

[peco-review-bot]

🔵 Low — The CHANGELOG says QueryContext continues to return transfer instructions. That is true for Thrift but not for the kernel backend. Before this PR, KernelBackend.Execute rejected every PUT/GET/REMOVE with ErrNotSupportedByKernel, whether it came from ExecContext or QueryContext. Now the guard only applies when req.TransferFiles is true, so kernel QueryContext quietly starts returning the raw 4-column instruction rows.

This matters for callers who followed the old guidance in examples/kernel/main.go: "on ErrNotSupportedByKernel, use Thrift for staging". If such a caller routes staging through QueryContext, it will now get a successful result with no file moved and no error that would trigger its fallback.

Please change the note to say that kernel-backed QueryContext used to reject these statements and now returns raw instructions, matching Thrift, so callers can review any fallback that depends on that error.

[aakash-saravanan-db]

Corrected in 5345fb0. The changelog now explicitly says native QueryContext changes from ErrNotSupportedByKernel to raw instructions with no file transfer, matching Thrift, and tells callers to review error-based fallback handling and use ExecContext for transfers. Documentation-only change; the existing raw-context, race and live tests remain applicable.

[peco-review-bot]

Thanks for following up. The checked-out head doesn't show the correction yet. CHANGELOG.md line 4 still says "QueryContext continues to return transfer instructions", and nothing in the entry mentions the change from ErrNotSupportedByKernel for kernel-backed QueryContext. 5345fb0 may not be pushed to this branch, or the edit may have landed somewhere else. Once the reworded entry is visible at head, this thread can be resolved.

[peco-review-bot]

Verdict: 1 Low

Looks good. I found one low-severity issue: the staging setter is called on every kernel ExecContext, not just staging statements. The ExecContext/QueryContext split is wired correctly through ExecRequest.TransferFiles. The pre-execution missing-staging-context error matches Thrift's message, and the removed tests and imports leave no dangling references. As the PR description says, the published-kernel CI job and the go.mod bump still have to pass before merge.

[peco-review-bot]

Verdict: 1 Medium · 2 Nit

The changes look correct, with one merge-gating concern: the new C setter isn't in the published kernel module that go.mod still pins, so this PR must not merge before the dependency bump. Execution and transfer wiring, the missing-staging-context check, and the cancellation and raw-instruction tests otherwise look sound.

Other findings

• ⚪ Nit — The new Build (published kernel) job is expected to fail until the kernel publishes a release with the new setter (per the PR description). As written it will be red on every PR and on main after merge. That trains people to ignore it, and the description already says it isn't a required check. Consider adding the job in the same change as the go.mod bump, or gating it (e.g. continue-on-error with a TODO) until the published module exists.
• ⚪ Nit — The comment says ExecContext opts into completion "including transfers missed by the SQL recognizer". For those statements the pre-check in Execute (backend.go:894) doesn't fire, so a missing staging context sends [] to the kernel, and the caller gets whatever error the kernel raises for a denied path. A detected statement gets the driver error "staging ctx must be provided." instead. So the same user mistake (no staging context) can produce two different error messages and categories depending on how the SQL is written, e.g. a prefix the Go recognizer doesn't handle. Consider documenting this or normalizing the kernel's denial error. No functional data-loss risk, since transfers are still denied.

[peco-review-bot]

🟡 Medium — kernel_statement_set_staging_allowed_local_paths gets called on every ExecContext, but the published kernel module this repo pins (v1.4.0, which go.mod/go.sum still reference) doesn't export that symbol. If this merges before the bindings are published and go.mod/go.sum are bumped, every -tags databricks_kernel build that links the published archive fails with an undefined symbol. That includes downstream users, not only CI. The PR description already lists this as a pre-merge gate. Please hold the merge (draft PR or a blocking label) until the dependency bump lands in this PR, so the header, KERNEL_REV and the module version change together.

[aakash-saravanan-db]

Confirmed on the current head: the published-native CI log fails to link kernel_statement_set_staging_allowed_local_paths against 1.4.0. This PR is explicitly blocked from merge until matching native modules are published, go.mod/go.sum are updated, and the published-native job passes. Source-build tests do not satisfy that gate. Kernel issue #443 tracks the coordinated publication and installed-package validation; no release version has been assigned.

[peco-review-bot]

Thanks for confirming, and for the link to kernel issue #443. The code still calls kernel_statement_set_staging_allowed_local_paths, and go.mod/go.sum still pin v1.4.0, so the finding is still outstanding. I'm leaving this thread open as a visible merge gate. Please also mark the PR as draft or add a blocking label so nobody merges it by accident. It can be resolved once the published native modules are out, go.mod/go.sum are bumped in this PR, and the published-native CI job passes.

[peco-review-bot]

Verdict: 1 Medium · 1 Low · 1 Nit

Mostly sound. ExecContext and QueryContext now set the transfer mode correctly, the missing-staging-context guard matches Thrift's error, and the native setter's error path closes the statement. I have one medium concern: the kernel revision pinned in KERNEL_REV doesn't match the one the PR description says was tested. There are also two low-severity notes, about the new CI job and a gap in unit-test coverage.

Other findings

• ⚪ Nit — TestExecuteRequiresStagingContext only covers TransferFiles: true. No untagged unit test pins the other side of the guard: with TransferFiles: false (the QueryContext path) and no staging context, the call must not be rejected. That case is only exercised by the cgo/databricks_kernel-gated staging_cancel_test.go. A refactor that dropped the req.TransferFiles && term from the guard in backend.go:894 would make the kernel QueryContext reject staging statements again, and nothing in the default suite would catch it. Consider adding a case that asserts the guard does not fire for TransferFiles: false, for example by checking the error does not contain the staging message.

[peco-review-bot]

Verdict: 1 Medium

Looks correct overall. ExecContext/QueryContext routing, the missing-staging-context rejection and the nested-comment handling in the staging detector look right and have tests. The one medium concern is the release dependency: merging before the kernel bindings bump would break every tagged kernel build.

[peco-review-bot]

🟡 Medium — Every kernel-backed ExecContext now calls kernel_statement_set_staging_allowed_local_paths, not only staging statements, because TransferFiles is always true for Exec. go.mod still pins the published bindings at v1.4.0, and that release doesn't export this symbol. Until the bindings are bumped, any consumer building with -tags databricks_kernel against the published modules gets a link failure, whether or not they use Volumes. The new build-published-kernel job will catch this, but only once it's a required check. Please treat the go.mod/go.sum bump as a hard merge blocker (e.g. keep the PR in draft) rather than relying on the description alone.

[peco-review-bot]

Verdict: 1 High · 1 Low

The logic looks correct: the ExecContext/QueryContext transfer split, the up-front staging-context check and the detector hardening are well tested. One blocker is that kernel-tagged builds can't link against the pinned v1.4.0 bindings until the bindings are published and go.mod is bumped.

[peco-review-bot]

🟠 High — kernel_statement_set_staging_allowed_local_paths is called on every ExecContext here, and declared in the vendored header. It is not in the databricks-sql-kernel-bindings v1.4.0 archives that go.mod still pins (go.mod:70-76). That means any -tags databricks_kernel build that uses the published modules will fail to link with an undefined symbol. The new Build (published kernel) job is there to catch exactly this, and it fails today. The description lists this as a pre-merge gate, but nothing in the code blocks a merge. Keep this PR unmergeable (draft/label) until the bindings are published and go.mod/go.sum are bumped in this same PR. Otherwise every kernel-tagged consumer breaks.

[aakash-saravanan-db]

Converted this PR to draft. Published 1.4.0 still lacks the setter, and Build (published kernel) remains a real failing gate. #443 tracks version assignment/publication, go.mod/go.sum updates and installed-package validation before this PR is made ready. Local source builds do not satisfy that gate.

[peco-review-bot]

Thanks for converting this to draft and opening #443 to track it. Draft status does what I asked for now, but the code at line 95 still calls a symbol that the pinned v1.4.0 bindings don't export. I'll leave this thread open as a visible blocker. It should close once go.mod/go.sum in this PR are bumped to a published bindings version that includes kernel_statement_set_staging_allowed_local_paths, and Build (published kernel) passes.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.