[aakash-saravanan-db]

The kernel backend accepts use_cloud_fetch=False but currently still downloads large results from cloud storage. Forward the existing connection option and request paged results through the SQL connection, preserving the enabled default and caller session settings.

Validation: 309 focused unit tests passed. Built and installed the connector wheel, then compared both backends on small and large results, query errors, connection reuse, and close. The disabled large-result case returned all 250,000 rows and 32,000,000 payload bytes with every value verified and zero cloud-storage downloads; enabled mode still used cloud downloads.

[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 Low · 1 Nit

Looks good, with 2 low-severity notes on the new e2e test. use_cloud_fetch now reaches the kernel client from Session._create_backend. When it is False, the client adds can_cloud_download=false to the session conf, replacing any caller-supplied copy of that key regardless of case and without changing the caller's dict. Unit tests cover the default, explicit True, None and case-variant inputs. Nit: the e2e test is named ..._disabled_... but also runs with use_cloud_fetch=True, so a neutral name would fit better.

Other findings

• ⚪ Nit — The pass/fail result depends on the exact text of kernel log messages: the "Using CloudFetch reader" substring, and any message containing both inline and reader. These are internal INFO strings in the Rust kernel, so a harmless rewording in a kernel release would break this connector e2e test even though results still arrive correctly. The test also depends on INFO records reaching caplog through pyo3-log's cached effective level, which this module already handles carefully elsewhere with reset_logging().

The existing test for the databricks.sql.kernel.pyo3 breadcrumb only warns when its log target is missing, for exactly this reason. Possible fixes:

• Treat the log-text checks as soft, and keep the hard assertions on row count, payload and digest.
• Or pin the reader choice with something that doesn't depend on log text, such as a kernel-exposed disposition or a check that no presigned-URL requests are made.

If you keep the log assertions, call reset_logging() around the connect, the same way test_kernel_and_driver_logs_share_file does, so test order can't hide the INFO records.

[peco-review-bot]

✅ No issues identified by the review bot.