[vuanhphung]

What type of PR is this?

• Other (performance)

Description

Large CloudFetch results on Thrift spend much of their fetch time on round trips while download threads sit idle. Instrumenting a live Thrift warehouse showed that the server ignores maxRows for result links and fills each FetchResults reply with links up to maxBytes of file data. With the 100 MiB buffer_size_bytes default that is 4–5 links of ~21 MB. SELECT * FROM samples.tpch.lineitem (30M rows, 4.9 GB of Arrow) needed 60 FetchResults; each batch kept only 4–5 of the 10 download threads busy, and the next FetchResults was sent only after the batch's slowest file had been consumed.

ThriftResultSet._fill_results_buffer now sends maxBytes = max(buffer_size_bytes, max_download_threads × largest link seen so far), capped at 4 × buffer_size_bytes. That is about one file per download thread (~210 MB with the defaults). As today, every link in a batch starts downloading as soon as the batch arrives. That matters because links expire 15 minutes after they are issued and the Thrift path has no link refresh. A larger fixed budget (JDBC's 386 MB, or the server's ~1 GiB cap) was not meaningfully faster in the A/B, and it leaves links queued behind a slow consumer. maxRows/arraysize, direct results, inline Arrow results (including use_cloud_fetch=False), and the rows each fetch method returns are unchanged.

The trade-off is memory for streaming consumers. The download manager could already hold up to max_download_threads files, but 4–5-link batches never filled it; now they do. Lowering max_download_threads shrinks both the batch and the buffer. Closing a result early (after 500k of 10M rows) leaves 7–8 downloads in flight, which read ~47–49 MB after the close and add 0.3–0.5 s to interpreter exit (main: 0–2 downloads, ≤5 MB, 0.09 s); close() itself still returns in ~0.1 s, and cancelling those downloads comes in #1038.

Combined with #1032, which decompresses CloudFetch files into private mmaps, that streaming cost goes away and the speedup stays. A separate live run (median of 5 fresh processes, 1.6 GB of lineitem) measured peak RSS growth for main / this PR / this PR + #1032: fetchmany_arrow(1M) 788 / 888 / 540 MB, fetchall_arrow 1924 / 2039 / 1703 MB. Fetch time was 7.87 / 4.07 / 4.13 s.

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

Workload	FetchResults	fetch p50	fetch p95	peak RSS
lineitem, 4.9 GB, fetchall_arrow, n=5	60 → 26	23.0 → 11.8 s	26.2 → 12.1 s	5446 → 5614 MB
⅓ of lineitem, 1.6 GB, fetchall_arrow, n=12	18 → 8	7.6 → 5.3 s	8.4 → 10.2 s	2124 → 2366 MB
⅓ of lineitem, 1.6 GB, fetchmany_arrow(1M), n=3	18 → 8	8.0 → 4.9 s	8.1 → 4.9 s	891 → 1258 MB

Wall time including execute for the 4.9 GB case went from 35.0 to 23.5 s. The 1.6 GB tails (main max 16.3 s, this PR max 10.4 s) come from single slow file downloads, because a batch still waits for its slowest file. Pipelining across batches would remove that wait and is left for a separate change.

How is this tested?

• Unit tests
• E2E Tests
• Manually

New unit tests cover the budget: it scales with max_download_threads and the largest link, never drops below buffer_size_bytes, and stays unchanged for inline results. The full unit suite passes. The test_driver.py e2e tests for large wide/narrow results, async large results, fetchone/fetchall/fetchmany/iteration, Arrow fetches, row limits and cancel pass against a live warehouse (31 passed, Thrift and SEA).

Manual live checks on a 30M-row, 32-link result verified exact row counts and order across batch boundaries for fetchall_arrow, fetchmany_arrow, mixed fetchmany/fetchone, and fetchall. They also covered close() and cancel() mid-stream (download threads are released; same behaviour as main) and use_cloud_fetch=False (unchanged). Every link started downloading with at least 899 s of its 900 s validity left, including with a slow consumer.

Related Tickets & Documents

N/A

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

The change looks correct and is well tested. The main concern is that the per-fetch maxBytes has no cap and grows with max_download_threads and the largest link seen. A docstring also still calls buffer_size_bytes the max fetch size, which is no longer true.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity robustness issue: the sizing heuristic assumes every CloudFetch link has bytesNum set. The budget logic is correct and its upper limit is enforced. The unit tests cover scaling, the cap, the floor and inline results. The memory trade-off is documented in the docstrings and the CHANGELOG.

[peco-review-bot]

Verdict: 1 Nit

Looks good. I found no correctness issues. The maxBytes budget is limited to between 1× and 4× buffer_size_bytes. It only changes after CloudFetch links have been seen, so inline Arrow results are unaffected. All six ThriftResultSet constructors in thrift_backend.py already pass max_download_threads. The unit tests pin down the scaling, the cap, the minimum and the inline cases.

Other findings

• ⚪ Nit — _max_link_bytes only ever goes up, so one unusually large link raises maxBytes for every later FetchResults on that result set, until it reaches the 4× cap. The cap bounds this, so it isn't a correctness issue. Still, a short comment saying the high-water mark is intentional would help future readers.

[vuanhphung]

On the review-body nit about _max_link_bytes being a high-water mark: that is deliberate. Each partition usually ends in a smaller tail file (6–10 MB next to ~20 MB files in the 1.6 GB lineitem runs), so a running min or the latest size would keep shrinking the batch back towards buffer_size_bytes. The trade-off is that one large link can make later batches hold somewhat more links than download threads. The comment in _fetch_max_bytes already names that case, and the 4x cap bounds it. Leaving the code as is to avoid another review round for a comment-only change. (Edited to fix my earlier wording, which had the direction of that trade-off backwards.)