[aakash-saravanan-db]

Feature or gap

SQL PUT, GET and REMOVE upload, download and delete files in Unity Catalog Volumes. The kernel backend currently rejects these commands.

What this PR addresses

Enable file transfers with permitted local paths, completed results and Python exception types, OS codes and filenames. Recognize commands after SQL comments. Streaming PUT remains unsupported in kernel mode.

Dependencies

Requires kernel #439 for transfers and error metadata. Merge requires matching native wheels, an updated dependency floor and installed-package validation; published 1.4.0 lacks the interface.

Tests and validation

439 focused tests, formatting and focused type checking pass. The built connector wheel passes live sync/async byte round trips, overwrite, deletion, path policy and missing-file cases with the locally built native dependency. Five-run Thrift → kernel lifecycle medians: 1.6113 → 1.7050 s (+5.81%), within 10%. Scheduled transfer coverage remains pending.

[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 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.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low-severity concern: the kernel path silently ignores a caller's input_stream. The path-type checks, leading-verb and nested-comment detection, the gate for older wheels, and the error-class mapping all match Thrift's _handle_staging_* behavior and have thorough unit tests. Two minor notes: the dependency floor and lockfile updates are still pending (already called out as the pre-merge gate per the kernel-floor rule in the learning log), and the test files include some unrelated Black reformatting of parametrize decorators.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Mostly solid: the error-class mapping, path normalization and older-wheel gating are well tested. I found two issues: the new e2e lifecycle test has no skip for kernel wheels without transfer support (medium), and KERNEL_REV doesn't match the kernel revision named in the PR description (low).

[peco-review-bot]

Verdict: 1 Low

Looks good overall. The error-class mapping (OSError subclass chosen from the real OS code, ProgrammingError for policy denial, OperationalError for remote errors), path normalisation and the old-wheel fallback are consistent and well tested. There is one low concern about streaming-PUT detection relying on the SQL text. Nits: NotSupportedError looks unused in tests/e2e/test_kernel_backend.py once the staging tests are removed. The kernel dependency floor in pyproject/lockfiles still has to be bumped before merge, as the PR's own merge gate (kernel #443) says.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with two low-severity notes. Paths are only forwarded for PUT/GET, and when detection misses a statement, empty paths go to the kernel, so access is denied rather than granted. The error mapping matches Thrift: raw FileNotFoundError/PermissionError from local open(), ProgrammingError for policy denial. Wheels without the new interface still fail with an explicit NotSupportedError. The pending dependency-floor bump is already listed as a pre-merge gate in the description.

[peco-review-bot]

Verdict: 1 Nit

Looks good overall. I found one low-severity issue: the new statement-level kwarg is only gated on the method existing, and it is tested with a permissive MagicMock. The error mapping matches the existing backend: a denied path raises ProgrammingError and local filesystem errors raise built-in OSError subclasses, as open() does today. Separately, the pyproject kernel floor still has to be bumped before merge, as the PR's pre-merge gate notes.

Other findings

• ⚪ Nit — set_staging_allowed_local_paths(normalized_paths, input_stream_token="__input_stream__") now runs for every statement, including plain SELECT. The capability gate only checks that the method exists (callable(set_paths)). It does not check that the method accepts the input_stream_token keyword.

Take a kernel build that has the method but an earlier signature without input_stream_token, for example an intermediate KERNEL_REV from before the token validation moved into the kernel. On that build the call raises TypeError. _wrap_kernel_exception then turns it into OperationalError, so every query fails on that connection, not just transfers.

The new unit tests can't catch this. stmt is a MagicMock, so assert_called_once_with(..., input_stream_token=...) passes whatever the real PyO3 signature is. Two possible fixes:

• Gate the kwarg on the method's introspected signature, the same way _kernel_session_accepts_kwarg does for Session.
• Rely on the planned dependency-floor bump to rule out such builds.

Either way, add a fixed-signature fake statement so the forwarding contract is actually locked in by a test.

[aakash-saravanan-db]

For review 5870373936: a90f623 replaces the permissive transfer setter mock in the existing parameterized forwarding test with a fixed-signature autospec. All 434 focused tests pass. Introspection of the actual released 1.4.0 wheel confirms that Statement has no transfer setter; the built 37c7a3e setter is (self, /, paths=None, *, input_stream_token=None), and the real sync/async lifecycle including SELECT passes. We rely on the documented coordinated dependency-floor gate, which remains blocked in #443 and keeps this PR draft; intermediate unpublished review builds are not a compatibility promise.

[peco-review-bot]

Verdict: 2 Low

The change looks correct, with two low-severity points about how exceptions are built and documented. Paths are forwarded only for statements whose leading verb is PUT or GET. When verb detection misses, the code fails safe: an empty allow-list is sent and the transfer is denied. Older wheels still reject transfers before execute, and the statement is closed on every rejection path.

[peco-review-bot]

Verdict: 1 Medium · 1 Nit

The transfer wiring and error mapping look sound overall, with one medium concern: the new path setter is called on every statement, so a source-built kernel whose setter lacks the new keyword would break ordinary queries too. The other finding is low: a cosmetic [Errno None] in some local-error messages. Merge is still gated on publishing the kernel release and bumping the dependency floor, as the PR description says.

Other findings

• ⚪ Nit — When the binding supplies os_error_filename but no OS code, the code builds OSError(None, message, filename). str() of that exception is "[Errno None] File transfer failed: 'local-file.bin'". Users see a literal Errno None in tracebacks, which the Thrift path (raw open() errors) never shows. Keeping errno=None for attribute fidelity is reasonable, but consider building the exception without the errno slot (e.g. OSError(message) and then setting .filename) so the message reads cleanly. Alternatively, add a test that pins the rendered message so this is a deliberate choice.

[peco-review-bot]

🟡 Medium — set_staging_allowed_local_paths(normalized_paths, input_stream_token="__input_stream__") now runs for every statement, including plain SELECTs, whenever the setter exists. The only check is callable(set_paths), so nothing confirms the installed binding accepts the input_stream_token keyword. Transfer support is limited to source builds, and pyproject has no floor that guarantees the keyword. A wheel built from an earlier kernel #439 revision would have the setter but not the keyword. On that wheel, every execute() would raise TypeError, which wrap_kernel_exception turns into OperationalError. That breaks ordinary queries, not just transfers.

Each execute_command creates a fresh statement, so a non-transfer statement has no state to reset. Two ways to fix it:

• Call the setter only when staging_verb is not None. Ordinary queries then never depend on the new interface.
• Also gate the keyword on the setter's introspected signature, and raise the existing "Volume transfers require…" NotSupportedError when it is missing.

Either way, ordinary queries keep working on any binding.

[aakash-saravanan-db]

The reproduced premise is an intermediate, unpublished kernel build rather than a supported dependency combination. Published 1.4.0 has no set_staging_allowed_local_paths method, so ordinary queries skip the call; the required pinned 37c7a3e method accepts input_stream_token, as verified by introspecting the actual rebuilt extension and by the real SELECT/transfer lifecycle. The fixed-signature autospec also matches that real signature. This PR stays draft until kernel #443 publishes matching packages and the dependency floor is bumped; source validation requires the exact pinned source. We do not promise compatibility with earlier unmerged revisions of that interface. The coordinated dependency gate is intentional, so a second signature-adaptation path is outside this change.

[peco-review-bot]

Agreed on the facts: published 1.4.0 has no setter, so ordinary queries skip the call there, and the pinned 37c7a3e setter does accept input_stream_token. The gap is that this guarantee lives only in the PR description and the draft status. Nothing in pyproject enforces it yet, and on the current head the only check is callable(set_paths). Any build that has the setter but not the keyword (an intermediate kernel #443 revision, or a local source build at another commit) would break every ordinary execute(), not just transfers. I'm keeping this open as a merge gate rather than asking for the second signature-adaptation path. It's resolved once the databricks-sql-kernel floor is raised to the first release that ships the keyword. Moving the setter call inside staging_verb is not None would also resolve it, and it's a one-line change if you'd rather not depend on how the releases are ordered.

[aakash-saravanan-db]

For the rendering nit in review 5870554923: the filename-only fallback intentionally uses the standard Python exception constructor, keeping errno=None and .filename populated without inventing an OS code. Its standard str() may include Errno None; this only applies when a binding omits the code. The actual current native OS failures carry real codes and the live comparison verifies their standard errno/filename values. We are not adding a custom message-rendering contract or another exception type for that fallback.

[peco-review-bot]

Verdict: 1 High

Not mergeable yet: there is one high-severity release-gating problem. The new kernel API is called on every statement while the dependency floor still allows wheels that don't have it, so all kernel-mode queries would fail. The error mapping, path normalization and comment-aware verb detection look correct, and the tests cover them well.

[peco-review-bot]

🟠 High — stmt.set_staging_allowed_local_paths(...) now runs for every statement, including plain SELECT 1, with no capability check. pyproject.toml:73 still allows databricks-sql-kernel ^1.4.0, and the PR description says published 1.4.0 lacks this method. On any wheel the resolver can currently pick, every cursor.execute on use_kernel=True fails: the AttributeError is caught by the except Exception at L668 and comes out as OperationalError("Unexpected error from databricks_sql_kernel during execute_command ..."). So today this breaks all queries, not only file transfers.

The PR description mentions the needed floor bump, but nothing in the diff enforces it, so merging early (or a partial cherry-pick) breaks things for users. Either bump the floor in this PR to the kernel release that ships #439, or gate the call: e.g. only call it when hasattr(stmt, "set_staging_allowed_local_paths"), and for PUT/GET/REMOVE raise NotSupportedError when the method is missing. Also add a unit test using a statement fake that lacks the method, since MagicMock accepts any attribute and can't catch this.

[aakash-saravanan-db]

Agreed: the native dependency floor is a merge and release gate. Published 1.4.0 is incompatible with this change, including ordinary queries. This PR remains draft and must not merge until the first published kernel release containing the setter is assigned, the dependency and lockfiles are updated, and installed-artifact validation passes. The prerequisite is tracked in databricks/databricks-sql-kernel#443.

The intended design is a coordinated native minimum. A second compatibility path and old-wheel tests would add code for an unsupported driver/native pairing. No version is invented and no clean-review claim is made while this gate remains open.

[peco-review-bot]

Thanks for confirming. Dropping the compatibility shim is fine if the native floor moves in lockstep, and you don't need old-wheel tests for a pairing you won't support. The current code still makes the call unconditionally, though, and pyproject.toml still allows ^1.4.0. So the concern isn't addressed yet, and I'm leaving this thread open as the visible merge gate. It can be resolved once the PR raises the databricks-sql-kernel floor and updates the lockfile to the first published release with set_staging_allowed_local_paths (tracked in databricks/databricks-sql-kernel#443). That way a non-capable wheel fails at install time, not on every execute.