Skip to content

perf(thrift): fetch more inline rows per round trip for fetchall and large fetchmany - #1033

Open
vuanhphung wants to merge 1 commit into
mainfrom
perf/thrift-inline-batch
Open

vuanhphung wants to merge 1 commit into
mainfrom
perf/thrift-inline-batch

Conversation

@vuanhphung

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • Other (performance)

Description

On Thrift, every FetchResults asks for arraysize rows (100k by default), even when the caller has already asked for everything. For narrow inline results that cap is what limits throughput, not the reply size. A 10M-row boolean result comes back inline, about 100k rows (a few hundred KB) per reply, so fetchall_arrow() took 97 round trips and ~6 s.

ThriftResultSet._fill_results_buffer now takes the number of rows the caller still needs. fetchall()/fetchall_arrow() and fetchmany(size)/fetchmany_arrow(size) with size > arraysize request those rows, capped at 2M per call (the JDBC driver's limit). fetchone(), iteration and fetchmany(size <= arraysize) still send maxRows = arraysize, so streaming consumers keep their batch size. maxBytes is unchanged, and the server bounds each inline reply by bytes. For wide rows it already returned only 61,440 rows (~3.9 MB) per reply whether maxRows was 100k or 2M, so wide results neither gain nor lose. CloudFetch links ignore maxRows, so CloudFetch is unaffected. Column-based results (no pyarrow, and metadata calls) use the same code; the no-pyarrow case is in the table. The SEA and kernel result sets don't use this code.

This touches _fill_results_buffer like #1030, so whichever merges second needs a trivial rebase.

Live A/B against a PRO warehouse (Thrift; fresh processes, main and this PR interleaved, warm-up discarded, n=5 each; fetch = time after execute() returns):

Workload FetchResults fetch p50 fetch p95 peak RSS
10M-row boolean, fetchall_arrow 97 → 5 6.07 → 0.38 s 6.54 → 0.45 s 126 → 127 MB
1M-row int, fetchall_arrow 9 → 1 0.69 → 0.17 s 0.74 → 0.53 s 127 → 131 MB
1M lineitem rows inline (cloud fetch off), fetchall_arrow 16 → 16 2.30 → 2.31 s 2.38 → 2.57 s 301 → 301 MB
1M-row int, column-based (no pyarrow), fetchall 9 → 1 2.71 → 2.27 s 2.97 → 2.39 s 386 → 398 MB
10M-row boolean, fetchmany_arrow(1_000_000) 97 → 10 6.25 → 0.68 s 6.69 → 0.82 s 123 → 124 MB
1M-row int, fetchmany(10_000) (streaming) 9 → 9 2.16 → 2.19 s 2.29 → 2.22 s 135 → 136 MB

How is this tested?

  • Unit tests
  • E2E Tests
  • Manually

New unit tests use a fake server that honours maxRows exactly. They pin the rows requested for each fetch method on Arrow and column-based results: arraysize for fetchone, iteration and small fetchmany; the remaining rows for large fetchmany; up to the cap for fetchall. They also cover the cap and an arraysize above it, and check that row order is exact across batches. The full unit suite passes. Against a live warehouse, the test_driver.py e2e tests pass (35 passed, Thrift and SEA): large wide/narrow results, async large results, fetchone/fetchall/fetchmany/iteration, Arrow fetches, row limits, cancel, and the metadata calls (tables, columns, schemas, catalogs).

Related Tickets & Documents

N/A

This pull request and its description were written by Isaac.

…large fetchmany

Every FetchResults asked for arraysize (100k) rows, so a 10M-row boolean
result took 97 round trips although each reply was tiny. fetchall*() and
fetchmany*(size > arraysize) now ask for the rows they still need, capped at
2M rows (the JDBC driver's limit). fetchone(), iteration and
fetchmany(size <= arraysize) still send arraysize, and replies stay bounded
by maxBytes.

Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>

@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.

✅ No issues identified by the review bot.

This branch was previously deployed

1 inactive deployment
azure-prod — 5d103655 Deployed Nov 12, 2026 by vuanhphung via test-with-coverage #1147
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant