[aakash-saravanan-db]

What type of PR is this?

• Bug Fix

Description

With use_kernel=True, the connector ignores use_cloud_fetch=False, so large results still require direct access to cloud storage. This PR forwards that existing connection option to the Rust kernel when opening the session, allowing complete results to be fetched through the SQL connection.

The connector sets the kernel's existing cloudfetch_enabled option. The kernel handles option precedence and result delivery. The change is limited to the kernel backend.

Related Tickets & Documents

Dependency: Requires databricks-sql-kernel #436, which fixes large-result delivery with CloudFetch disabled. KERNEL_REV pins that implementation for source builds. Before merging, publish the native Python package containing the fix and update pyproject.toml and poetry.lock to require it. The release version has not yet been assigned.

How is this tested?

• Unit tests: 254 kernel-client tests and 72 session tests pass.
• E2E tests: disabled delivery succeeds through a proxy that blocks storage access; enabled delivery attempts storage access.
• Manual validation: locally built connector/native wheels return all 250,000 rows and 32 MB of payload with the expected hash. Query errors, connection reuse, and close pass.

Five measured runs per mode, after warmup: Thrift median 3.462 s; kernel median 3.210 s (-7.3%), including complete result consumption and hash verification.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

The fix itself looks correct, with 1 medium and 1 low concern. Forwarding use_cloud_fetch and adding can_cloud_download=false to the session conf keeps the default and copies the caller's dict instead of mutating it. The connection-parameters doc still says the kernel ignores this option, and the precedence over a caller-supplied value is untested.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low concern. Forwarding the option and the can_cloud_download override are done correctly, and unit tests cover the override, including mixed-case keys, the default, and not mutating the caller's session_configuration. Nit: the edited use_cloud_fetch row in CONNECTION_PARAMETERS.md lost the column padding the other rows use.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low concern. The forwarding follows Thrift's kwargs.get("use_cloud_fetch", True) default, overrides the key case-insensitively without changing the caller's dict, and leaves the default unchanged. The only gap is that no test checks that the server actually honors the can_cloud_download conf key.

[peco-review-bot]

Verdict: 1 Nit

The fix looks correct, with one medium test-coverage concern: the new e2e test passes whether or not cloud fetch is actually disabled. Nit: the reformatting of @pytest.mark.parametrize("empty_filter", ...) in tests/e2e/test_kernel_backend.py is unrelated to this fix.

Other findings

• ⚪ Nit — This e2e test doesn't check the behavior the PR is fixing. It confirms that 250k rows arrive intact with use_cloud_fetch=False, but the same result arrives through cloud-storage downloads, so the test passes whether or not the server honors can_cloud_download=false. It would pass even if open_session dropped the key, or if the warehouse ignored the conf.

The PR description says zero cloud-storage downloads were seen during manual testing, but nothing in CI locks that in. The unit test only shows that the connector sends the key. Whether the kernel/server accepts it and stops cloud fetch is exactly what this e2e test should prove. Possible fixes:

• Assert there are no external-link/cloud-fetch downloads, e.g. by counting kernel databricks.sql.kernel log records for CloudFetch downloads, or by patching or inspecting the download path.
• At minimum, run SET can_cloud_download in the session and assert it returns false.

The test could also include an enabled-mode control showing that downloads do happen with the default setting.

[peco-review-bot]

Verdict: 1 High · 1 Low

The forwarding logic is correct and well tested, with one high concern: use_cloud_fetch=False now sends a session key that released kernel 1.4.0 (the current ^1.4.0 floor) doesn't handle, so this must not merge until the dependency floor is bumped. Nit: in the e2e proxy fixture, server_close() waits for handler threads, and each one can sit in a 30s select timeout, which may slow teardown.

[peco-review-bot]

🟠 High — This branch now injects cloudfetch_enabled="false" into session_conf, but the declared floor is still ^1.4.0, and the PR says released 1.4.0 doesn't consume this key locally. On a 1.4.0 wheel the key goes straight to the server as a session parameter. The comment this PR removes said the SEA endpoint "enforces its own allow-list and rejects unknown keys". So use_kernel=True, use_cloud_fetch=False used to connect (cloud fetch was just ignored) and may now fail at open_session for anyone the resolver puts on 1.4.0.

The PR description already calls this a merge gate. Please don't merge until the floor in pyproject.toml/poetry.lock is bumped to the first native release containing kernel#436. If that can't land together with this change, gate the injection on a capability check, as the other kernel options do.

(Anchored to the nearest changed line — see the description for the exact location.)

[aakash-saravanan-db]

Acknowledged; this is the same native-release gate tracked in the current dependency thread. This PR must remain unmerged until the native release containing kernel #436 is published and both pyproject.toml and poetry.lock require it. The release version is not assigned yet. Leaving this thread open until those dependency updates are present.

[peco-review-bot]

Thanks for confirming. The code at line 398 still adds cloudfetch_enabled="false" without any check, and the declared floor is still ^1.4.0, so the 1.4.0 regression is still possible. Leaving this thread open as the merge gate. It's resolved once pyproject.toml and poetry.lock require the native release that contains kernel#436. If that release slips, a capability check around the injection would also resolve it.

[peco-review-bot]

Verdict: 1 Medium · 1 Nit

I found no logic errors on the connector side, but merging is risky until the kernel dependency floor is raised. The kwarg forwarding, the copy-don't-mutate handling of session_configuration, and the unit/e2e coverage all look correct. The two findings are about how this behaves against the currently allowed kernel 1.4.0, and about conflicting key variants that differ only in case.

Other findings

• ⚪ Nit — Only the exact lowercase key cloudfetch_enabled is overwritten. A caller-supplied variant such as CLOUDFETCH_ENABLED: "true" is still forwarded next to the connector's "false", and the new unit test locks that in. Spark/SEA conf keys are usually matched case-insensitively. If the kernel does the same, which value wins depends on the kernel's precedence rule or map ordering, so use_cloud_fetch=False could be overridden without any error. Consider dropping case-insensitive matches of the key before setting it, or point to the kernel test that pins this precedence.

[peco-review-bot]

🟡 Medium — cloudfetch_enabled only works as a client-side setting in a kernel build that includes databricks-sql-kernel#436. pyproject.toml:73 still allows ^1.4.0. Per the comment this PR deleted, a 1.4.0 binary forwards session_conf unchanged to the SEA endpoint, which rejects unknown keys. So on the wheel that resolves today, use_cloud_fetch=False would go from "ignored" to either failing at open_session or being silently ignored by the server. The PR description already lists this as a merge gate. Please make sure this PR includes the floor bump (and the poetry.lock refresh) before merge, so the gate lives in dependency metadata and not only in CONNECTION_PARAMETERS.md prose.

[aakash-saravanan-db]

Acknowledged; this is the same native-release gate tracked in the current dependency thread. This PR must remain unmerged until the native release containing kernel #436 is published and both pyproject.toml and poetry.lock require it. The release version is not assigned yet. Leaving this thread open until those dependency updates are present.

[peco-review-bot]

Thanks for confirming. Your plan to keep this unmerged until the native release with kernel #436 ships, and until both pyproject.toml and poetry.lock require that version, is the right gate. No dependency bump is visible yet: pyproject.toml still allows ^1.4.0, and the code at line 399 still sends cloudfetch_enabled unconditionally when use_cloud_fetch=False. So this thread stays open as the blocking item until the floor bump and the lock refresh land.

[aakash-saravanan-db]

The remaining key-case nit is resolved by the shared kernel contract, not by connector-side normalization. At the pinned revision, apply_client_result_overrides sorts recognized spellings and applies the canonical lowercase spelling last, then removes every variant before sending server configuration: https://github.com/databricks/databricks-sql-kernel/blob/0b5d0edfed32c0745d5abc2468fc3070e2eb2a17/src/session.rs#L1247 . Its case-variant tests pin deterministic ordering. A focused real-wheel restricted-proxy control also passes use_cloud_fetch=False with CLOUDFETCH_ENABLED=true and can_cloud_download=true: all 250,000 rows / 32,000,000 bytes and the expected digest, with zero blocked non-workspace connections. The native release/dependency-floor merge gate remains open until its version is assigned.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

The change is small and correct, with one medium concern: the kernel dependency floor isn't bumped, so older allowed wheels would get cloudfetch_enabled=false without the large-result fix. The forwarding into session_conf is well tested, including that it doesn't modify the caller's config. Nit: the unrelated signature reformat in test_open_session_passes_geospatial_representation_to_kernel adds diff noise.

[peco-review-bot]

🟡 Medium — The fix only works with databricks-sql-kernel #436 (large-result delivery with CloudFetch off), but only KERNEL_REV is bumped. pyproject.toml / poetry.lock still allow older wheels. If the resolver picks one of those, use_cloud_fetch=False now sends cloudfetch_enabled=false to a kernel that can't deliver large results without CloudFetch. Large results would then break or come back incomplete, even though the CHANGELOG and CONNECTION_PARAMETERS.md now list the option as supported (✅). Please make the floor bump part of this PR, or block the merge on it, so the docs and the dependency graph match. Otherwise, document that the option needs kernel ≥ the release containing #436.

[aakash-saravanan-db]

Acknowledged; this is the same native-release gate tracked in the current dependency thread. This PR must remain unmerged until the native release containing kernel #436 is published and both pyproject.toml and poetry.lock require it. The release version is not assigned yet. Leaving this thread open until those dependency updates are present.

[peco-review-bot]

Thanks for confirming. Agreed: the right fix is to keep this PR unmerged until the native release containing kernel #436 ships and both pyproject.toml and poetry.lock require that version. Right now the dependency graph still allows older wheels, and the docs and CHANGELOG already mark use_cloud_fetch=False as supported. I'm leaving this thread open until the floor bump is in the diff so it stays in the unresolved queue as a merge blocker.

[peco-review-bot]

Verdict: 1 High · 1 Low

The forwarding change itself is small and correct, but it shouldn't merge until the kernel dependency floor is raised to the release containing the fix (1 high finding). There is also 1 low finding about duplicate keys that differ only in case in session_conf. Unit tests check the forwarding both when session_configuration is passed in and when it isn't, and that the caller's dict isn't modified.

[peco-review-bot]

🟠 High — This branch now sends cloudfetch_enabled="false" whenever a caller passes use_cloud_fetch=False. The PR says the kernel only delivers large results correctly with CloudFetch disabled after databricks-sql-kernel#436. But pyproject.toml:73 still allows ^1.4.0, and only KERNEL_REV (used for source builds) points at the fix.

On any wheel in the allowed range, users who already set use_cloud_fetch=False (it was silently ignored before, so results came back fine via CloudFetch) would now get the broken disabled-CloudFetch behavior on large results. On a wheel that doesn't consume the key locally, it would instead be forwarded to the server as an unknown session conf. Either way, existing working setups regress.

The PR description already lists the pin bump as a pre-merge step. Please make it a hard gate: bump the floor in pyproject.toml / poetry.lock in this PR, or hold the merge, so the resolver can't pick a kernel without the fix.

[aakash-saravanan-db]

Confirmed: this PR remains merge-gated until the native package containing kernel #436 is published and both pyproject.toml and poetry.lock require that release. The release version is still unassigned. The PR description retains this prerequisite, and the documentation cleanup leaves that gate in place.

[peco-review-bot]

Thanks for confirming the merge gate. The code hasn't changed yet, though: pyproject.toml and poetry.lock still allow kernel wheels without #436, so the regression risk is still there. I'll leave this thread open as the gate. It can be resolved once the PR bumps the floor in both files to the release that includes #436.