[aakash-saravanan-db]

Feature or gap

Kernel-backed Python cursors need to complete Volume file and stream operations with the existing cursor behavior.

What this PR addresses

Route server-marked instructions through native completion, preserve lazy BinaryIO reads and empty uploads, and retain local errors and cancellation behavior. Downloads enforce allowed roots, including resolved symlink targets.

Dependencies

Builds select databricks/databricks-sql-kernel#465 at 855847bdca71e07285cfc60332fb2f33f8e47786. The connector and compatible native kernel will release together; release acceptance requires the real compatible native version, an updated dependency floor, and clean installed-package validation. Published native 1.4.0 lacks the required APIs.

Tests and validation

282 focused connector tests and source-built file/stream comparisons pass. Rebuilt-native configured GET timeout proof passes and preserves prior destination bytes; named sync, async, nonempty-stream and empty-stream workflows pass. Retained performance evidence remains qualified by its recorded source build; published-package and scheduled activation are pending.

[peco-review-bot]

Verdict: 1 High · 1 Medium · 1 Low

Needs changes. The main blocker is that the code relies on new kernel APIs while the dependency floor still allows older wheels, which would break every kernel query, not only Volume operations. There is also a KeyError crash when the best-effort canceller is missing.

Other findings

• 🔵 Low — This PR deletes the kernel e2e tests for staging and adds no replacements. All new coverage uses mocks that invent is_volume_operation, complete_volume_operation return strings ("needs_input_stream", "completed", "not_volume") and exception attributes (local_os_error, http_status, local_path). Nothing in CI checks that the real kernel binding actually has these names and values. Please add kernel e2e cases, such as PUT→GET→REMOVE round trips (file and input_stream) and a rejected symlink outside the allowed root, so a mismatch with the kernel contract fails the build.

[peco-review-bot]

🟠 High — pyproject.toml:73 still allows databricks-sql-kernel ^1.4.0, but the code now calls three kernel APIs that don't exist yet: ExecutedStatement.is_volume_operation (read here on every sync execute), kernel_handle.is_volume_operation (read in KernelResultSet.__init__ for every result, including metadata ResultStreams), and fetch_result() in place of await_result() for async results.

If the resolver installs any wheel in the allowed range that lacks them, this isn't limited to Volume ops. Every execute(), every metadata call and every get_async_execution_result() raises AttributeError, so ordinary SELECT 1 breaks on use_kernel=True. The PR description says the floor "has not been selected". This should block merge: bump the floor in the same change. Until then, reading is_volume_operation with getattr(..., False) would at least keep non-volume queries working.

[aakash-saravanan-db]

The optional-canceller defect is corrected at ab1efce1: registration uses .get, the Statement remains retained, and cleanup skips absent cancellers. All 276 affected tests pass; final native live checks preserve lazy BinaryIO and one-read empty/nonempty streams.

The new Volume marker/completion API dependency is a valid merge prerequisite. The source build pins compatible 22f96fac; a compatible published native version/floor and installed-package proof remain explicitly open. This PR is not merge-ready, and no old-native fallback or invented release version was added.

The fetch_result subclaim is separate: released 1.4.0 exposes it, but the substitution is removed from this PR. Merged native #451 provides terminal-response reuse with the original await_result boundary. Actual public-cursor owner and reader/refetch tests show identical await/fetch requests, rows and metadata on the selected native; redundant #1056 is superseded without merge.

[peco-review-bot]

Thanks for confirming the Volume-marker dependency is a merge prerequisite, and for dropping the fetch_result() substitution. That part of the finding is resolved. The core issue is still there, though. pyproject.toml:73 still says databricks-sql-kernel ^1.4.0. Line 641 still reads executed.is_volume_operation with no guard. KernelResultSet.__init__ still reads kernel_handle.is_volume_operation the same way. So if a published wheel in that range lacks the marker, ordinary sync executes and metadata calls fail with AttributeError before the code ever checks for a Volume op. A source build pinned to 22f96fac doesn't limit what the resolver installs. Leaving this open until the floor is raised to the first published wheel that ships the marker, or until both reads are guarded so non-volume queries keep working.

[peco-review-bot]

🟡 Medium — Canceller registration just above is explicitly best-effort: if stmt.canceller() raises, the error is swallowed and nothing is stored under id(cursor). This line then indexes self._sync_cancellers[id(cursor)] directly. For a volume statement that means KeyError after stmt.execute() has already succeeded. The outer except turns it into a kernel-wrapped error, and because close_stmt is still True the statement gets closed.

The result: a missing canceller (meant to be harmless) now fails every PUT/GET/REMOVE with a confusing error. Use .get(id(cursor)) and handle None (store the entry with no canceller, and skip it in cancel_command/close_command/close_session). No test covers a volume op when canceller() raises.

[peco-review-bot]

Verdict: 1 High · 1 Medium · 1 Low

Needs changes: 1 high, 1 medium and 1 low finding. Reading the new is_volume_operation attribute without a guard (while the dependency floor stays at ^1.4.0) would make every kernel query fail on currently allowed wheels, not just Volume operations. Apart from that, the client and result-set bookkeeping for retained cancellers looks consistent.

Other findings

• 🟡 Medium — The PR deletes the two kernel staging e2e tests and adds nothing in their place. The connector tests are now all mocks: they invent is_volume_operation, the "needs_input_stream"/"completed"/"not_volume" outcome strings and the local_os_error/VOLUME_STORAGE_HTTP_ERROR exception attributes. None of them check that the real binding emits those values. The new symlink-target restriction (a security boundary documented in CONNECTION_PARAMETERS.md) has no in-repo test at all.

The PR description mentions live staging runs, but they aren't in the diff. Please add at least one use_kernel=True e2e round-trip covering PUT, GET, REMOVE and a stream PUT, plus one test that a symlink pointing outside the allowed root is rejected. These would replace the deleted tests.

[peco-review-bot]

🟠 High — KernelResultSet.__init__ now reads kernel_handle.is_volume_operation with no guard, and execute_command does the same at client.py:641 (executed.is_volume_operation). This attribute comes from the unreleased kernel change (databricks-sql-kernel#465), but pyproject.toml still allows databricks-sql-kernel ^1.4.0. That is a problem beyond PUT/GET: every KernelResultSet goes through this constructor, including plain SELECTs, metadata ResultStreams and async await_result() streams. On any 1.4.x wheel the resolver can pick, every use_kernel=True query would therefore fail with AttributeError instead of just lacking Volume support.

The PR description says the floor hasn't been selected yet. Until it is, either bump the floor in the same change, or read the attribute with getattr(handle, "is_volume_operation", False) and gate the native completion on its presence. Gating would also need a guard for complete_volume_operation. Otherwise, older wheels go from working to completely broken.

[peco-review-bot]

🔵 Low — When an unfinished staging result set is closed, close() first calls backend.cancel_command(command_id), which looks up the retained canceller and calls cancel(). It then calls backend.close_command(command_id), which pops the same _volume_handles entry and calls cancel() a second time. So the same canceller fires twice on every early close. This is probably harmless, but it's redundant. Calling cancel_command from close() is only needed because cancellation has to happen before the native close(). One option is to pop the entry in that first step, so close_command finds nothing left to cancel.

[aakash-saravanan-db]

The possible duplicate best-effort cancellation on an unfinished close is an optional cleanup observation. Successful transfer close is already gated and sends no cancel; a pending close must cancel before native completion/cleanup so it cannot publish later. Retaining that ordering and the existing close behavior. The actual-native completed sync/async and pending-close controls pass; they do not establish a universal single-cancel guarantee for every unfinished path.

[peco-review-bot]

Agreed that the ordering matters and should stay: cancelling before the native close is the right call, and successful closes never send a cancel. My point was narrower. On an unfinished close, the same retained canceller is still called twice, once by cancel_command and again by close_command, and the current code still does this. It's low severity and you can defer it. If you pick it up later, one fix keeps the ordering: pop the _volume_handles entry in the first step, or have close_command skip handles that were already cancelled. Leaving this thread open in case someone wants to follow up.

[peco-review-bot]

Verdict: 1 High

One high concern blocks merge: the dependency floor in pyproject.toml still allows kernel wheels that lack the new volume API, and the new code reads is_volume_operation on every execute and every result set. On those wheels all kernel queries would break, not just Volume statements. The rest looks coherent: the completion flow, the order of cancel and close, and the error mapping all match the Thrift behaviour, and the new branches have unit tests.

[peco-review-bot]

🟠 High — KernelResultSet.__init__ now reads kernel_handle.is_volume_operation unconditionally, and execute_command reads executed.is_volume_operation (client.py:643) on every sync execute. These attributes come from the unmerged kernel change (databricks-sql-kernel#465). pyproject.toml still allows databricks-sql-kernel ^1.4.0. On any wheel in that range without the attribute, every use_kernel=True result raises AttributeError, and that includes plain SELECT and metadata ResultStreams, not just Volume statements. The pre-execute NotSupportedError guard has also been removed, so an older wheel gets no clear error either. The PR description says the floor "has not been selected". Before merge, either bump the floor to the first kernel release with is_volume_operation / complete_volume_operation, or read the attribute with getattr(handle, "is_volume_operation", False). With the getattr option, keep the old fail-loud guard for wheels that lack the API, so PUT/GET does not quietly do nothing.

[peco-review-bot]

Verdict: 1 High

One high-severity merge blocker: the new is_volume_operation reads are unguarded, so they break every kernel-backed query on the 1.4.x wheels that pyproject.toml still allows, not just Volume operations. The rest of the change looks consistent. Follow-up: the removed kernel e2e staging tests have no replacement e2e for PUT/GET/REMOVE or stream uploads against a real warehouse, and there is no unit test for Volume completion through get_async_execution_result.

[peco-review-bot]

🟠 High — Every KernelResultSet now reads kernel_handle.is_volume_operation without a guard, and execute_command reads executed.is_volume_operation the same way (client.py ~L643). The PR description says the published 1.4.0 wheel lacks these APIs, yet pyproject.toml still allows ^1.4.0. If the resolver picks 1.4.x, this is not just missing Volume support: every use_kernel=True result breaks with AttributeError. That covers plain SELECTs, metadata ResultStreams and async await_result() streams.

It also requires ResultStream, not just ExecutedStatement, to expose is_volume_operation in the new kernel. The unit fakes add the attribute, so this is not proven.

Fix options:

• Bump the floor in the same change to the release that ships these APIs.
• Or read the attribute with getattr(kernel_handle, "is_volume_operation", False), and the same for executed, so older wheels keep the old non-staging behavior.

The description treats this as a merge gate, but the code as written turns it into a regression for all queries, not only a missing feature.

[aakash-saravanan-db]

Valid and still open as a merge prerequisite. Published native 1.4.0 lacks these APIs; changing KERNEL_REV only selects the source build. 07538ed selects the reviewed 855847bd source for local/CI validation. The accessible package proxies still expose 1.4.0, so a compatible published version must be selected before updating the dependency floor and validating a clean installed package on ordinary queries and Volume workflows. No fallback or invented release version is being added; the PR description explicitly keeps this gate open.

[peco-review-bot]

Thanks for confirming. Nothing has changed in the code since the finding: line 88 still reads kernel_handle.is_volume_operation with no guard, and the ^1.4.0 floor still lets the resolver install the published 1.4.0 wheel. On that wheel, every use_kernel=True result set fails, not only Volume operations. I'm leaving this thread open as the merge gate you described. It can close once the floor points at a published release that ships is_volume_operation on both ExecutedStatement and ResultStream, and that release passes a clean-install check covering ordinary queries as well as Volume workflows. Not adding a fallback is a reasonable choice, as long as the floor bump is in the same merge.

[aakash-saravanan-db]

The release plan is a coordinated native-kernel and connector version bump. The published 1.4.0 incompatibility remains valid: before connector release, the native dependency floor/lock must select the real compatible version, and clean installed ordinary queries and Volume workflows must pass. Source validation uses kernel 855847bd, which supplies the marker/completion APIs. Package availability is a release-integration prerequisite rather than a blocker to reviewing that source pair. No 1.4.0 compatibility, completed package validation, fallback or invented version is being claimed.

[peco-review-bot]

Thanks for spelling out the release plan. Nothing in the code has changed: line 88 still reads kernel_handle.is_volume_operation with no guard, and pyproject.toml still allows ^1.4.0. I'm keeping this thread open as the merge gate. You've said it stays open too, and it shouldn't be resolved until the dependency floor/lock points at the published compatible release. That release also needs to pass a clean-install check on ordinary queries, metadata, async and Volume workflows. Testing the source pair against kernel 855847bd is fine for review, but those results don't carry over to what users get from installing the package.

[aakash-saravanan-db]

On the review-body coverage follow-up: the removed e2e tests asserted the obsolete unsupported-PUT behavior. Companion databricks/databricks-driver-test#2122 has public-cursor sync/async PUT/GET/REMOVE and nonempty/empty BinaryIO cases using the real native binding, with byte and deletion assertions. Those source-built named workflows pass. A direct async Volume cursor unit case would strengthen local coverage and remains optional. This evidence does not satisfy the still-open compatible published-native floor, clean installed-package or scheduled-activation gates.