Skip to content

fix(kernel): pass cursor row limit to kernel - #922

Merged
vuanhphung merged 2 commits into
mainfrom
vu-phung/pecoblr-4127-kernel-row-limit
Aug 20, 2026
Merged

fix(kernel): pass cursor row limit to kernel#922
vuanhphung merged 2 commits into
mainfrom
vu-phung/pecoblr-4127-kernel-row-limit

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Forward cursor row_limit to the kernel statement and remove the temporary connector-side stream truncation. This uses databricks-sql-kernel#269 and advances KERNEL_REV to the merged commit.

Validation: python -m pytest tests/unit -m 'not realkernel' (895 passed, 4 skipped). Live kernel coverage exercises None, 0, 1, and 5.

PECOBLR-4127

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Medium

Looks good overall — the row_limit forwarding is wired correctly and the _drain() rewrite preserves buffer/offset/row-index accounting. One medium concern: the tests silently dropped the row_limit=0 boundary case, which the removed connector code treated as a real zero-row limit, leaving a possible behavior change (0 → unlimited, matching SEA) uncovered.

Comment thread tests/e2e/test_kernel_backend.py Outdated

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — forwarding row_limit to the kernel and removing the connector-side stream truncation is coherent: set_row_limit is applied before both the sync and async execute branches, the _drain rewrite accounts for _buffer_offset/_buffered_count/_next_row_index consistently, and unit + e2e tests are updated (including the new None case). One low-severity note: negative row_limit was previously normalized to "unlimited" and that behavior is now dropped and untested.

Comment thread src/databricks/sql/backend/kernel/client.py
@vuanhphung
vuanhphung added this pull request to the merge queue Aug 20, 2026
Merged via the queue into main with commit a68464c Aug 20, 2026
60 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants