[aakash-saravanan-db]

Pre-merge gate: coordinated native publication and dependency updates. Publish the matching native packages and update this PR's dependency metadata and lockfiles before merging. No native release version has been assigned; published 1.4.0 lacks the new interface.

Enable file-based SQL PUT, GET, and REMOVE through the native backend. Forward the existing permitted-local-path option to the shared kernel, consume completed instruction rows, and preserve exception classes for denied paths, filesystem permissions, missing upload files, and storage failures. Streaming PUT fails explicitly, including comment-prefixed and no-whitespace forms.

Depends on shared kernel transfers; the source pin identifies the locally tested implementation. Ordinary queries remain usable on older wheels through interface detection; transfers require the new interface. Documentation identifies this as source-build support until publication.

Validation: 360 focused client/result tests and Black checks pass. The locally built wheel passes synchronous and asynchronous one-MiB lifecycles, byte equality, overwrite, deletion, local-path denial, and missing local/remote files. A matched read-only-destination check verifies PermissionError and preserved bytes. Five alternating measurements per side after equal warmups give lifecycle medians 1.6409 → 1.6199 s (-1.28%) for Thrift → native, with complete consumption and byte verification. The separate intermittent PUT slowdown remains tracked in kernel #445; this passing cohort does not explain it.

[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: 2 Low

Looks good overall, with two low-severity concerns. The error mapping matches Thrift (ProgrammingError for local path/argument errors, FileNotFoundError for a missing local file, OperationalError for HTTP failures). Path validation also matches Thrift for str/list/tuple/Path inputs, and the unit tests cover forwarding, malformed paths, streaming PUT and older wheels.

[peco-review-bot]

🔵 Low — The floor is still ^1.4.0, and CHANGELOG/CONNECTION_PARAMETERS say 1.4.0 cannot do these transfers. The feature-detect fallback keeps 1.4.0 from regressing: it still raises NotSupportedError. But a user who installs [kernel] from this release gets a documented feature that never works. The merge gate is currently only a TODO comment and a note in the PR description. Raise the floor, and regenerate the lockfile, in the same PR once kernel #443 ships, so packaging matches the docs.

[aakash-saravanan-db]

Confirmed: the dependency floor and lockfile must change in this PR before merge. Kernel#443 remains open with no version assigned, so I cannot supply a real published floor yet. The docs now label transfers Source build and explicitly exclude published 1.4.0. This remains an unresolved publication/merge gate, rather than a claim of released support.

[peco-review-bot]

Thanks for confirming. Labeling the docs as "Source build" and noting that published 1.4.0 is excluded does make the current state accurate, and I agree you can't set a real floor until kernel#443 ships a version. The finding is still open, though: pyproject.toml still says ^1.4.0, and the only merge gate is a TODO comment. I'm keeping this thread open so it stays in the unresolved queue as a hard blocker. Please raise the floor and regenerate the lockfile once the kernel release is published, before merge.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity gap: the streaming-PUT check and the PUT/GET/REMOVE detection use different patterns. The error mapping, the fallback for older kernel wheels, and the way allowed local paths are passed to the kernel are consistent, and unit and e2e tests cover them. The merge gate in the description (kernel #443, raising the minimum kernel version) still applies before release.

[peco-review-bot]

Verdict: 2 Low

Looks good apart from two low-severity concerns. Path forwarding, streaming-PUT rejection, the fallback for older wheels, and the error mapping are consistent with Thrift's handlers in client.py and are well covered by unit and e2e tests. The open items are the dependency-floor merge gate and an inconsistent http_status guard on the PermissionError mapping.

[peco-review-bot]

🔵 Low — The code now depends on Statement.set_staging_allowed_local_paths, but the dependency floor is still ^1.4.0. The CHANGELOG says published 1.4.0 lacks this API. The callable(set_paths) gate fails safe with NotSupportedError, so nothing breaks silently. Still, a resolver that picks 1.4.x installs a connector whose advertised PUT/GET/REMOVE support doesn't work. The only protection is a TODO comment and the PR description. The floor bump and lockfile refresh should land in this PR before merge, as the description says. Something mechanical, such as a CI check that the installed kernel exposes the method, would make the gate harder to miss.

[aakash-saravanan-db]

Agreed: this remains an unresolved pre-merge dependency gate, tracked in kernel #443. The latest published native version is still 1.4.0, and no new release version has been assigned. pyproject.toml and poetry.lock must be updated in this PR once the matching wheel exists, followed by installed-package validation. The source-build lifecycle passes; interface detection and documentation do not satisfy the published-version gate.

[peco-review-bot]

Thanks for confirming. We agree on the facts: the floor is still ^1.4.0, no published wheel exposes set_staging_allowed_local_paths yet, and the only gate is the TODO that points to kernel #443. Since nothing has changed in code, I'm keeping this thread open so it stays visible as a blocker until the floor bump and poetry.lock refresh land in this PR. You could also mark the PR as draft or add a "blocked" label while you wait on the kernel release. That way the requirement shows up somewhere other than this comment.

[peco-review-bot]

Verdict: 1 Medium

Mostly sound, with one medium test-coverage gap: the deleted verb-detection tests left _staging_verb's negative cases untested. Error mapping matches Thrift: local missing files raise FileNotFoundError, forbidden paths raise ProgrammingError, and remote 404s raise OperationalError. Each exception is wrapped only once in execute_command and get_execution_result. Separately, the pyproject floor is still ^1.4.0 with only a TODO. The new API is capability-gated (older wheels fail with NotSupportedError), but per the learning log the floor bump in kernel #443 must land before release.

[peco-review-bot]

Verdict: 1 Medium

Mostly solid, with one medium concern: errors that are already mapped to FileNotFoundError/PermissionError can be wrapped a second time into OperationalError at nested call sites. The capability gate falls back to NotSupportedError on older wheels, so a 1.4.0 wheel fails loudly instead of silently. Even so, the ^1.4.0 floor and lockfile still need the bump tracked in kernel #443 before release, as the PR description says.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Looks correct overall: verb detection, path forwarding and validation, and exception classes all match Thrift's _handle_staging_* behavior. There is one medium merge-gate concern (dependency floor) and one stale docstring.

[peco-review-bot]

🟡 Medium — The connector now depends on Statement.set_staging_allowed_local_paths for PUT/GET/REMOVE, but the dependency floor is still ^1.4.0, and the CHANGELOG says 1.4.0 does not ship that method. A resolver that installs 1.4.0 makes every transfer fail with NotSupportedError. The current state is safe: there is a fallback for older wheels and an explicit TODO, and the PR description calls this a merge gate. Still, the support claims in CHANGELOG and CONNECTION_PARAMETERS.md only hold for an unpublished source build. Please don't merge until the floor (and lockfile) are raised to the first published kernel release with #439. Otherwise the feature ships advertised but cannot be used from PyPI.

[aakash-saravanan-db]

Agreed; this remains an explicit pre-merge gate under https://github.com/databricks/databricks-sql-kernel/issues/443. Latest published native is still 1.4.0 and no replacement version is assigned. The dependency floor/lockfile must be updated in this PR after matching publication, followed by released-artifact validation. Source-build results do not establish support for the published package.

[peco-review-bot]

Thanks for confirming. Nothing has changed in the code yet: the floor is still ^1.4.0 and the TODO points at kernel#443, so this thread stays open as the merge gate. Before merging, raise the databricks-sql-kernel floor and the lockfile to the first published release that ships Statement.set_staging_allowed_local_paths. Then validate against the released wheel and adjust the CHANGELOG and CONNECTION_PARAMETERS.md if the support claims change.

[peco-review-bot]

Verdict: 1 Low

Mostly looks good, with one low inconsistency in how staging_allowed_local_path is validated. Both new paths fail closed: older wheels still raise NotSupportedError, and the error mapping reproduces Thrift's exception classes. The merge gate on kernel #443 and the dependency-floor bump are tracked in the PR description and the pyproject TODO. Nit: the PUT/GET/REMOVE and __input_stream__ regexes could be precompiled at module level.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity edge case in the streaming-PUT detection regex. Error-class mapping matches Thrift: allowed-path violations raise ProgrammingError, a missing upload file raises a raw FileNotFoundError, and HTTP failures raise OperationalError. Older wheels fail closed. As the PR description already says, pyproject.toml still allows kernel 1.4.0, which lacks this API, so the floor bump in kernel #443 has to land before merge.

[peco-review-bot]

Verdict: 2 Low

Looks good overall, with two low-severity issues. Verb detection, streaming-PUT rejection, the fallback that rejects transfers on older wheels, and the error mapping all match Thrift's classes (Thrift's open() also raises a raw FileNotFoundError), and the unit tests cover them well. The remaining concern is the dependency floor gating that the PR description already names. Nit: examples/staging_ingestion.py now says "list of path strings", but lists of Path objects work on both backends, and the unit tests check that.

[peco-review-bot]

🔵 Low — The CHANGELOG and CONNECTION_PARAMETERS.md now advertise kernel-mode PUT/GET/REMOVE, but the dependency floor is still ^1.4.0. The docs themselves say 1.4.0 can't do these transfers. The callable(set_paths) gate fails loudly with NotSupportedError, so this isn't a silent regression. Still, the only thing enforcing the requirement is a TODO comment. If this merges or ships before kernel #443, users who install [kernel] get a wheel that rejects every transfer. The PR description already treats this as a merge gate, so I'm flagging it so the floor bump and lockfile update land in this PR rather than a follow-up.

[aakash-saravanan-db]

Agreed. https://github.com/databricks/databricks-sql-kernel/issues/443 gates merge on publication of the matching wheel and the dependency floor/lockfile update in this PR. The local native build validates the source integration only. No replacement release version is assigned, so 1.4.0 is not claimed to support these transfers and the PR remains unmerged.

[peco-review-bot]

Thanks for confirming that #443 is the merge gate. Nothing has changed at this location yet: the floor is still ^1.4.0 and only the TODO enforces the requirement. I'm leaving this thread open so it stays in the unresolved queue. Once the matching kernel wheel is published, bumping the floor and updating the lockfile in this PR will address it.

[peco-review-bot]

🔵 Low — The kernel path runs each list element through os.fsdecode, so bytes entries (e.g. [b"/tmp/a"]) are accepted and decoded. Thrift's _handle_staging_operation passes elements straight to os.path.abspath/os.path.commonpath alongside the str local file, which raises TypeError for mixed str/bytes. So the same config behaves differently on the two backends. Also, the error message says "a string or a list of paths" while the updated example docstring says "a list of path strings". Pick one accepted element contract (str/PathLike, or explicitly str-only), then make the docs, the message and the Thrift behaviour match it. Add a unit test for the bytes element case too.

[aakash-saravanan-db]

Fixed in dbf9701. Normalization uses os.fspath and requires str results, preserving lists of strings/text-valued PathLike objects while rejecting bytes. The example and validation message document that contract. The four PUT/GET byte-list cases failed before the fix and pass afterward; the full focused client/result suites pass (360 cases). A direct call through the existing Thrift staging handler confirms the mixed str/bytes TypeError. The standalone Path rejection remains consistent with Thrift.

[peco-review-bot]

Thanks for the detailed write-up. The fix you describe (os.fspath plus a str check, with the message and docs updated to match) would resolve this. But the head I can see doesn't contain it yet. Line 591 still calls os.fsdecode(path), which means bytes elements are still accepted. The validation message at lines 585 and 595 also still says "a string or a list of paths", and os.fspath doesn't appear anywhere in this file. It looks like dbf9701 either hasn't been pushed to this branch or touched a different spot. Once the change is on the PR head, this should be good to go.

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