Repository navigation
fix(kernel): honor use_cloud_fetch when opening sessions #1019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
78099d6
af7fecf
3b57c66
eece87e
0a04d94
0d12e2d
4ba9d74
b324a11
0261d7c
c1e9c90
1f33ada
0eb26c6
8082069
c6388c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 6f4b6d607a61bdde61dcda2cf82bc8e80fd32cb9 | ||
| 0b5d0edfed32c0745d5abc2468fc3070e2eb2a17 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -293,6 +293,7 @@ def __init__( | |
| # The kernel binding owns type and range validation. | ||
| self._request_timeout_secs = kwargs.get("request_timeout_secs") | ||
| self._max_connections = kwargs.get("max_connections") | ||
| self._use_cloud_fetch = kwargs.get("use_cloud_fetch", True) | ||
| # Opt-in native GEOMETRY / GEOGRAPHY support. True requests | ||
| # the canonical Arrow struct and surfaces as ``{"srid": int, "wkb": | ||
| # bytes}`` through pyarrow; False requests WKT / EWKT strings. This is | ||
|
|
@@ -390,12 +391,13 @@ def open_session( | |
| ) -> SessionId: | ||
| if self._kernel_session is not None: | ||
| raise InterfaceError("KernelDatabricksClient already has an open session.") | ||
| # ``session_configuration`` flows through to the kernel's | ||
| # ``session_conf`` map verbatim; the SEA endpoint enforces | ||
| # its own allow-list and rejects unknown keys. | ||
| # The kernel consumes client settings locally before forwarding server parameters. | ||
| session_conf: Optional[Dict[str, str]] = None | ||
| if session_configuration: | ||
| session_conf = {k: str(v) for k, v in session_configuration.items()} | ||
| if not self._use_cloud_fetch: | ||
|
aakash-saravanan-db marked this conversation as resolved.
Resolved
peco-review-bot[bot] marked this conversation as resolved.
Resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High — This branch now injects The PR description already calls this a merge gate. Please don't merge until the floor in (Anchored to the nearest changed line — see the description for the exact location.)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for confirming. The code at line 398 still adds There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High — This branch now sends On any wheel in the allowed range, users who already set The PR description already lists the pin bump as a pre-merge step. Please make it a hard gate: bump the floor in
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for confirming the merge gate. The code hasn't changed yet, though: |
||
| session_conf = session_conf or {} | ||
|
peco-review-bot[bot] marked this conversation as resolved.
Resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium —
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for confirming. Your plan to keep this unmerged until the native release with kernel #436 ships, and until both |
||
| session_conf["cloudfetch_enabled"] = "false" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — This only works if the installed kernel reads On a 1.4.x wheel, the key goes into Please bump the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This connector is released together with an updated native kernel; compatibility with the released 1.4.0 binary is not required. The documentation now explicitly requires the native release containing kernel #436 and a coordinated published-dependency update. No future version is assigned yet. KERNEL_REV pins the changed source, and validation uses a locally rebuilt native wheel from that revision. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docs half of this is done: CONNECTION_PARAMETERS.md now names the #436 kernel requirement. The packaging half isn't. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High — This adds On 1.4.0, Possible fixes:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added an explicit pre-merge gate to the PR description: the native release containing kernel #436 must be published and the minimum dependency updated in this PR before it can merge. No compatibility fallback or guessed future version is introduced. The current changes are validated from the pinned native source. The option documentation also explicitly requires a Python bool; accepting string values would change the existing Thrift input contract. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, gating the merge on the kernel release is one of the fixes I suggested, so I'm fine with that approach. But the gate is only written in the PR description so far. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — The fix only works with databricks-sql-kernel #436 (large-result delivery with CloudFetch off), but only
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for confirming. Agreed: the right fix is to keep this PR unmerged until the native release containing kernel #436 ships and both |
||
| # The kwarg builds run INSIDE the try so the ``finally`` scrub | ||
| # below always fires — including when ``kernel_auth_kwargs`` | ||
| # itself raises mid-build (e.g. an OAuth token-exchange failure | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.