[aakash-saravanan-db]

Feature or gap

The native backend rejects file-based SQL PUT/GET/REMOVE even though the existing backend completes them. Applications need the same local-path policy and exception behavior.

What this PR addresses

Forward permitted paths to the shared kernel, consume completed instruction rows and preserve error classes for policy denial, local filesystem errors and storage failures. Normalize strings and text-valued PathLike lists without accepting byte paths. Detect transfer verbs after nested leading SQL comments. Streaming PUT remains explicitly unsupported, including comment-prefixed forms. Only the exact lowercase reserved token is treated as streaming; uppercase lookalikes remain ordinary filenames. Preserve real local OS error codes, including platform-specific Windows conversion, without fabricating errno for older wheels. Real OS codes select the native OSError subclass, including directory and non-directory-parent failures, and nested wrapping preserves it. Requested filenames are preserved for local OS errors; kernel #447 tracks this metadata fix.

Dependencies

Shared kernel transfers provide the execution and native interfaces. The source pin is b92a867 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. Ordinary queries retain interface detection for older wheels. Transfer support remains explicitly limited to source builds until publication.

Tests and validation

391 focused client/result tests and Black pass. Locally built artifacts pass synchronous/asynchronous byte round trips, overwrite, denial, missing files, deletion, read-only destination protection, write-only GET and nested-comment PUT/GET/REMOVE through both backends. The uppercase-token filename and original local OS code/filename also match the existing backend. Focused type checking passes; whole-source local type checking reproduces seven unrelated urllib3 errors on untouched baseline source.

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.6417 → 1.6810 s (+2.39%); PUT 0.8245 → 0.8147 s (-1.19%). All comparable verb medians meet the 10% criterion. Kernel #445 retains earlier gaps and conflicting cohorts, partial RCA and the direct timing proposal. A separate proxy diagnostic did not reproduce the gap and supplies no acceptance verdict.

[peco-review-bot]

Verdict: 1 High · 2 Low

One high-severity issue blocks merge. The new set_staging_allowed_local_paths call is unconditional, and the kernel dependency floor (^1.4.0) was not bumped. On any currently published kernel wheel, every kernel-backed query would fail, not only volume operations. Otherwise the error mapping and tests look reasonable. The two smaller findings are a stale parameters doc and an undocumented assumption about the kernel's error contract.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Mostly solid: one medium and one low concern. Streaming PUT (input_stream) is no longer rejected on the kernel path, so users get a confusing error instead of NotSupportedError. The file-transfer PermissionDenied mapping ignores http_status, unlike the NotFound branch. Nit: removing item 6 from the numbered list in CONNECTION_PARAMETERS.md leaves the source numbering at 5 → 7.

[peco-review-bot]

Verdict: 1 Medium

Mostly solid. One medium concern: on wheels that support transfers, the allowed paths are now forwarded on every statement without converting path-like values to strings, so a Path value could break all queries. Nit: removing item 6 from the list in CONNECTION_PARAMETERS.md leaves the numbering jumping from 5 to 7.

[peco-review-bot]

Verdict: 1 Low

The change looks correct; I found one low-severity style issue. Leading-verb detection fails closed: anything not detected as PUT/GET is sent an empty allow-list, and wheels without the new method reject transfers. Error mapping matches Thrift (missing local files raise FileNotFoundError, path-policy violations raise ProgrammingError, a 404 on GET raises OperationalError). The stated merge gate still applies: the pyproject.toml floor ^1.4.0 lets the installer pick a wheel that can't do transfers, so raise the floor before release.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity release-gating concern: the kernel dependency floor still allows wheels that lack the new transfer API. Path validation, how errors map to exception classes (FileNotFoundError, ProgrammingError, OperationalError), streaming-PUT rejection and the older-wheel fallback all behave like the Thrift _handle_staging_operation path, and unit tests cover them well. Nit: the _strip_leading_sql_comments docstring no longer mentions that block comments do not nest, which is why the simple scan to */ is correct.

[peco-review-bot]

🔵 Low — The connector now relies on Statement.set_staging_allowed_local_paths for kernel-mode PUT/GET/REMOVE, but the floor is still ^1.4.0. The release blocker is recorded only in a TODO comment, the CHANGELOG and the PR body. The getattr(..., callable) gate in execute_command means older wheels raise a clear NotSupportedError and don't silently no-op, so this isn't a correctness regression. Still, a resolver can install 1.4.0 and users get the 'Volume transfers require…' error even though the docs advertise support. Please raise the floor to the first kernel release that ships the API (and update the lockfile) in this PR, or in one that lands atomically with it, before merging. The TODO alone won't stop this from merging.

[aakash-saravanan-db]

The dependency floor/lockfile remains a hard merge/release gate in kernel #443; no native version containing the interface has been assigned or published, so I cannot make a truthful version bump yet. This PR stays unmerged until that publication and the in-PR bump. I also checked the docstring nit at runtime: nested SQL block comments do work on the reference path. A matched probe found that the old scan incorrectly denied the native transfer. Fixed in 04a52c5 with balanced comment scanning; 372 focused tests pass, including nested forwarding, unterminated comments and streaming rejection. I have not restored the inaccurate non-nesting claim.

[peco-review-bot]

That makes sense: you can't truthfully bump the floor until kernel #443 publishes a version with set_staging_allowed_local_paths, and the nested-comment fix in 04a52c5 is a separate improvement I'm glad to see. The pin is still ^1.4.0, so a resolver can install a wheel that doesn't have the API, and the finding still applies. The only thing stopping a merge is the TODO comment, which doesn't block anything mechanically. I'm leaving this thread open on purpose so it stays in the unresolved queue as the visible merge gate. Once the kernel release exists, please bump the floor and lockfile in this PR and that will close it out. If you want a gate that holds until then, mark the PR as draft or add a "do-not-merge" label.

[peco-review-bot]

Verdict: 1 Low

Looks good overall. One low finding: the dependency floor is enforced only by a TODO comment, not by the dependency metadata. The leading-verb detection, path normalization and validation, interface fallback for older wheels, and the error-class mapping (ProgrammingError for denied paths, and FileNotFoundError/PermissionError for local files, matching Thrift's native open() behavior) are consistent with each other and covered by unit tests.

[peco-review-bot]

🔵 Low — The new Statement.set_staging_allowed_local_paths interface isn't in any published kernel release, but the floor is still ^1.4.0. The only reminder is this TODO comment. Interface detection in execute_command keeps ordinary queries working on older wheels, so nothing regresses. Still, users who install from PyPI and read the CHANGELOG entry for Volume transfers will hit NotSupportedError.

The PR description already says this is a pre-merge gate. Raising the floor (or the lockfile) in this PR once kernel #443 publishes would turn that gate into something CI enforces, instead of relying on a comment.

[aakash-saravanan-db]

The dependency gate remains valid. In 9a595dd I removed the temporary release-process TODO from the implementation; publication and the version/lockfile bump remain explicit pre-merge requirements in this PR and kernel #443. No native release version is assigned, so changing the floor now would invent an unavailable package. The runtime and test trees, native source pin and parsed dependency metadata are identical to measured 04a52c5. The latest direct cohort has a PUT/lifecycle latency gap tracked in #445; the separate proxy diagnostic does not waive acceptance. This PR remains unmerged.

[peco-review-bot]

Agreed that the floor can't be raised yet, since no kernel release exists to point it at, and I'm not asking for a placeholder version. But 9a595dd deletes the TODO without changing ^1.4.0 or the lockfile, so the only in-tree reminder is gone and the gate now lives only in the PR description. The concern is still open: a resolver can still install a wheel that lacks Statement.set_staging_allowed_local_paths. I'll leave this thread open as the tracker, so it can be resolved in the same push that moves the floor (and the lock) to the first kernel release that ships #443.

[peco-review-bot]

Verdict: 1 Low

The change looks correct. I found one low-severity edge case: a comment between PUT and '__input_stream__' gets past the streaming-PUT guard. Interface detection keeps ordinary queries working on older wheels. The dependency floor and lockfile updates are still pending, which the PR description already calls out as a pre-merge gate.

[peco-review-bot]

🔵 Low — The streaming-PUT guard only allows whitespace between PUT and the quoted '__input_stream__' token. The comment stripper only removes leading comments, so a comment after the verb gets past the guard. For example, PUT /* stream */ '__input_stream__' INTO '/Volumes/...' reaches the kernel. The kernel then treats __input_stream__ as a relative local path, so the user gets a path-denied ProgrammingError or a FileNotFoundError instead of the explicit NotSupportedError("Streaming PUT ...") that the PR description promises.

This fails safe (no unintended file access), but the error is misleading. Two ways to fix it: allow interleaved comments in the pattern (e.g. run _strip_leading_sql_comments on the text after the verb), or document that only whitespace forms are detected. A unit case in test_kernel_rejects_streaming_put for the comment-between form would pin down whichever behavior you choose.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low-severity gap: the streaming-PUT guard misses a comment placed between PUT and the __input_stream__ literal. The error mapping (raw FileNotFoundError/PermissionError for local failures, ProgrammingError for denied paths, OperationalError for HTTP failures) matches how the Thrift handlers behave, and the older-wheel fallback keeps ordinary queries working. Reminder: pyproject still doesn't raise the minimum databricks-sql-kernel version to the release that ships set_staging_allowed_local_paths. That is the PR's stated pre-merge gate and should land before merge.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with two low-severity parity notes. Path forwarding, nested-comment stripping, older-wheel detection and the error mapping all match Thrift's error classes and have good tests. Merge gate: pyproject.toml:73 still declares databricks-sql-kernel ^1.4.0, which lacks set_staging_allowed_local_paths. As the description says, raise the floor to the first release that ships the interface before merging, or transfers will fail with NotSupportedError on resolver-chosen wheels.

[peco-review-bot]

Verdict: 1 Low

The change looks solid. There is one low-severity concern: async transfers that go through the attach-by-id fallback (it does not get the allowed local paths) are untested. Error-class mapping matches Thrift: FileNotFoundError/PermissionError come from local open(), and denied paths raise ProgrammingError. Nits: NotSupportedError is now an unused import in tests/e2e/test_kernel_backend.py. The databricks-sql-kernel dependency floor still needs bumping before merge (learning-log rule from 2026-10-28), which the PR already lists as a pre-merge gate.

[peco-review-bot]

🔵 Low — The permitted local paths are only set on the Statement built in execute_command. For async PUT/GET, get_query_state and get_execution_result fall back to attach_async_statement(command_id.guid) in three cases: a concurrent poll already holds the owning handle, a re-fetch happens after the owning stream has started, or a fresh cursor/process resumes the id. A handle attached by id never receives set_staging_allowed_local_paths. If the kernel runs the file transfer while the result stream is consumed (the PR says it "consumes completed instruction rows"), those fallback paths will either be denied or behave in some other way nobody has specified. A re-fetch that repeats the transfer is also possible. The e2e async test only covers the owning-handle path. Please either document that async transfers are only supported on the owning handle, or add a unit test that pins what the attach-by-id path does for a transfer statement. I have not checked this against the kernel implementation.

[peco-review-bot]

Verdict: 1 Nit

Looks good overall, with one low concern about losing the filename in the OSError mapping. The path forwarding fails closed: an unrecognised verb forwards [], and malformed paths are rejected only for PUT/GET. Older wheels are detected and rejected, and error classes match Thrift's raw open() behaviour; unit coverage of all three is thorough. As the PR description already notes, the databricks-sql-kernel floor in pyproject still has to be raised to the first release that ships set_staging_allowed_local_paths before merge. That follows the learning-log rule about bumping the floor when calling a newly added API.

Other findings

• ⚪ Nit — When the kernel sends os_error_filename but no os_error_code, file_error_args falls back to (message,). The FileNotFoundError/PermissionError you build then has .filename is None, even though the kernel told us which file it was. The value only shows up on the non-standard os_error_filename attribute. Code that follows Thrift and reads exc.filename (as with a raw open() failure in _handle_staging_put) gets nothing. One case where this can happen is a missing upload file that the kernel finds through a pre-check, not through an OS call. Consider building (errno.ENOENT / EACCES or 0, message, os_error_filename) whenever a filename is present. Also add a unit case for filename-without-code; right now test_file_transfer_error_matches_thrift_exception only covers the case where both are absent.

[aakash-saravanan-db]

The current producer cannot send a filename without an OS code: kernel_error::OsError has a required i32 code and an optional filename, and PyO3 derives both attributes from that same optional object. Completion attaches the requested path only when this OS error object exists. Local missing-file prechecks use the actual std::io::Error, preserving its raw code; they do not synthesize filename-only metadata. Older bindings provide neither field. The matched rebuilt missing-parent and permission probes verify both errno and filename against the existing backend. I am retaining this typed invariant and avoiding invented ENOENT/EACCES/0 values for an unproduced state. The native publication/version-floor gate remains mandatory.

[peco-review-bot]

Verdict: 1 Nit

Looks good overall, with one low-severity concern: some local filesystem errors get a different exception class than on Thrift. Leading-verb detection, streaming-PUT rejection, path normalization and the older-wheel fallback have thorough unit tests. As the PR description already says, pyproject.toml/lockfiles still need the kernel floor bumped to the first release with set_staging_allowed_local_paths before merging.

Other findings

• ⚪ Nit — Only FILE_TRANSFER_PERMISSION_ERROR and NotFound local errors become OSError subclasses here. Any other local filesystem failure that comes back with an os_error_code falls through to cls(message, context), which means OperationalError/DatabaseError. Examples: GET into a path that is a directory (EISDIR), a parent directory that is missing on GET, or ENOSPC while writing.

Thrift raises the raw IsADirectoryError / FileNotFoundError / OSError from open() in these cases. So a caller catching OSError around a GET works on Thrift but misses the error on the kernel path. The PR does preserve os_error_code/os_error_filename as attributes, but the class still differs.

To match Thrift, build OSError(*file_error_args) whenever file_transfer and os_error_code is not None and http_status is None. OSError's constructor already picks the right subclass from errno. If the divergence is intentional, document it in CONNECTION_PARAMETERS.md and add a unit case for a non-ENOENT/EACCES errno.

[aakash-saravanan-db]

Fixed the remaining local OS exception mapping in e3806ee, with kernel directory-open handling in 4f12d12. A transfer carrying a real local OS code now goes through OSError's platform constructor, which selects the appropriate subclass; nested wrapping preserves it. The existing no-code fallback remains for older bindings, without fabricated errno. Five added regression cases failed before the fix; all 391 focused tests, Black and focused mypy pass. The matched before probe confirmed IsADirectoryError and NotADirectoryError differences; rebuilt-artifact validation will repeat those exact cases. Windows runtime parity is not inferred from the Linux run.

[peco-review-bot]

Verdict: 1 Low

Looks good overall. One low-severity concern: the streaming-PUT check is case-sensitive, and nothing yet confirms the server treats the token's case the same way. Nits: the dependency floor and lockfiles still need bumping before merge (the PR description already makes this a pre-merge gate). The Black reflows of unrelated tests (test_columns_with_empty_string_filter_matches_nothing, test_open_session_passes_geospatial_representation_to_kernel) add diff noise.

[peco-review-bot]

Verdict: 1 Low

Looks good: one low-severity concern about case handling in the streaming-PUT guard. Error mapping, interface detection for older kernels, path normalization and leading-verb detection all look correct and are well covered by tests. The comment stripper fails closed: when it misreads a comment, no allowed paths are forwarded. Merging still depends on the pre-merge gate in the description: KERNEL_REV points at an unpublished kernel commit and the databricks-sql-kernel dependency floor hasn't been bumped. Nit: the unrelated Black reformatting in both test files adds noise to the diff.