fix(kernel): stop premature sync statement close (H4); bump KERNEL_REV to batch 2#830
Open
vikrantpuppala wants to merge 2 commits into
Open
fix(kernel): stop premature sync statement close (H4); bump KERNEL_REV to batch 2#830vikrantpuppala wants to merge 2 commits into
vikrantpuppala wants to merge 2 commits into
Conversation
…V to batch 2 Connector half of the kernel batch-2 fixes (kernel PR #123). Bumps KERNEL_REV to pick up the batch-2 kernel surface. H4 — don't close the kernel Statement at sync execute-return: execute_command's finally used to `stmt.close()` immediately after `stmt.execute()` succeeded. For a large CloudFetch result with paginated chunk links (all_fetched=false), the kernel fetches later links lazily (get_result_chunks against the LIVE statement) during consumption, so a premature CloseStatement broke those fetches. The kernel now auto-closes the server statement when its result stream drains (ExecutedStatement::next_batch end-of-stream), with the executed-handle Drop as the backstop for partial/abandoned reads. So the connector now flips close_stmt=False on a successful execute and only closes on the error path (no executed handle was produced). The other batch-2 fixes (cancelled-class -> OperationalError, U2M refresh fail-fast, metadata statement close-on-drop, per-binding OAuth client_id) are entirely kernel-side and need no connector code beyond the KERNEL_REV bump. Tests: unit (sync execute does-not-close on success / does-close on failure) + e2e (large multi-chunk result drains without premature close + cursor reuse; server cancel maps to OperationalError not ProgrammingError). All e2e verified live against dogfood. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
#123 picked up a follow-up commit fixing the wiremock cancelled-state assertions (ErrorCode::Cancelled). Bump the placeholder pin so the connector CI builds the corrected kernel. Still to be re-pinned to the squash-merge SHA before #830 merges. Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Connector half of the kernel batch-2 CUJ-gap fixes (databricks-sql-kernel#123). Bumps
KERNEL_REVto pick up the batch-2 kernel surface, plus the one connector-side change (H4).H4 — don't close the kernel
Statementat sync execute-returnexecute_command'sfinallyused tostmt.close()immediately after a successfulstmt.execute(). For a large CloudFetch result with paginated chunk links (all_fetched=false), the kernel fetches later links lazily (get_result_chunksagainst the live statement) during consumption — so a prematureCloseStatementbroke those fetches. (Verified earlier by live repro that the commonall_fetched=truecase was fine, so this only bit very large paginated-link results.)The kernel now auto-closes the server statement when its result stream drains (
ExecutedStatement::next_batchend-of-stream, kernel #123), with the executed-handleDropas the backstop for partial/abandoned reads. So the connector now flipsclose_stmt = Falseon a successful execute and closes only on the error path (where no executed handle / result set was produced to reap the statement).The rest of batch 2 is kernel-only
cancelled-class →
OperationalError, U2M refresh fail-fast, metadata statement close-on-drop, and per-binding OAuthclient_idare all entirely kernel-side and need no connector code beyond theKERNEL_REVbump.Testing
use_kernel=True): large multi-chunk result (5M rows) drains without premature close + cursor reuse after auto-close; server cancel surfaces asOperationalError(notProgrammingError); plus the existing batch-1 e2e (diagnostic-info, staging, params) re-verified against the batch-2 wheel.Depends on #123 (kernel).
This pull request and its description were written by Isaac.