[vuanhphung]

What type of PR is this?

• Other (performance)

Description

The warehouse returns small inline results as dozens of LZ4-compressed Arrow batches: range(100) and range(1000) both arrive as 72 batches. convert_arrow_based_set_to_arrow_table decompressed each batch with lz4.frame.decompress, which releases and reacquires the GIL on every call. When many threads run statements, each reacquire waits behind the other runnable threads. Against a mock that replays the captured reply with 50 ms RTT, a 1,000-row statement at 64 threads made ~600 voluntary context switches and used 5.3 ms of CPU, half of it sys time (1.4 ms with one thread), and throughput stalled at ~390 statements/s. Replaying the decompressed bytes from a dict, with no C call, removed the effect, which confirmed the cause.

For three or more compressed batches, the frames are now concatenated and decoded with one pyarrow.CompressedInputStream read. Arrow decodes concatenated LZ4 frames natively; python-lz4's one-shot decompress silently returns only the first frame. One- and two-batch results such as SELECT 1, uncompressed results, and pyarrow builds without the lz4 codec keep the existing path. CloudFetch, SEA and the kernel backend are unchanged.

Trade-off: Arrow takes the stream's decode buffers from pyarrow's memory pool, which keeps a heap per thread, so peak RSS grows by about 0.25 MB per busy thread (+21 MB at 64 threads, +50 MB at 200 in the mock runs).

How is this tested?

• Unit tests
• E2E Tests
• Manually

New unit tests decode real LZ4-compressed Arrow batches (1, 2, 3 and 72 batches, compressed and not, a highly compressible result that needs several reads, and the no-codec fallback) and check that 72 batches no longer call lz4.frame.decompress (72 calls on main). They pass on pyarrow 14.0.1, 23.0.1 and 26.0.1, and the full unit suite passes. Live e2e (test_driver.py core, large wide/narrow and async suites): 132 passed; the 6 test_staging_ingestion_* failures are a missing ingestion user in my environment (stage://tmp/None/... permission denied) and fail the same way on main.

Performance: main vs this branch in fresh, interleaved processes, median of 5 runs, 1,000-row result, one connection and a cursor per statement per thread. Mock = captured warehouse reply with 50 ms RTT; live = shared warehouse.

Setup	Threads	Fetch	Statements/s	p50 (ms)	p99 (ms)	CPU/statement (ms)
mock	16	fetchall_arrow	283 → 294	53 → 53	86 → 69	2.2 → 1.5
mock	64	fetchall_arrow	393 → 748	161 → 85	194 → 129	5.3 → 1.8
mock	200	fetchall_arrow	386 → 696	503 → 277	625 → 482	5.5 → 2.1
mock	64	fetchall	175 → 215	360 → 289	450 → 415	9.1 → 5.7
live	200	fetchall_arrow	380 → 499	487 → 359	877 → 1073	5.4 → 2.1
live	64	fetchall	175 → 204	355 → 302	548 → 500	8.9 → 6.0

On the live warehouse, 64-thread fetchall_arrow and all 1- and 16-thread runs were capped by the shared warehouse, and the higher live p99 at 200 threads coincides with ~30% more load on it. SELECT 1 (one batch, unchanged path) and the kernel backend measured the same on both sides.

Related Tickets & Documents

None.

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Low · 1 Nit

Looks good: the change is well-scoped and well-tested, with two low-severity points. One is that the exception type for corrupt payloads now depends on how many batches the result has; the other is a test that assumes the environment's pyarrow has the lz4 codec.

Other findings

• ⚪ Nit — test_convert_arrow_based_set_decodes_many_lz4_batches_in_one_call expects (72, 0) decompress calls without checking utils._PYARROW_HAS_LZ4. On a pyarrow build without the lz4 codec (exactly the case the fallback handles), this test fails instead of skipping. Pin the flag with patch.object(utils, "_PYARROW_HAS_LZ4", True) plus a skip when pyarrow.Codec.is_available("lz4") is False, or just skip when the codec is missing.

[vuanhphung]

Nit from the review body (one-call test assumes the lz4 codec): fixed in 2140f9d. test_convert_arrow_based_set_decodes_many_lz4_batches_in_one_call is now skipped when utils._PYARROW_HAS_LZ4 is False. The fallback path on such builds is still covered by test_convert_arrow_based_set_without_pyarrow_lz4_codec.

[peco-review-bot]

Verdict: 2 Low

Looks good overall. The batched LZ4 path is correct and tested with real compressed data, and the fallback paths are unchanged. I have two low concerns: real out-of-memory errors get reported as decompression failures, and corruption in a middle frame isn't tested.

[peco-review-bot]

Verdict: 3 Low

No blocking issues: the single-call decode is correctly limited to 3+ LZ4 batches and falls back to the old path when pyarrow lacks the lz4 codec. The error types match the old path (MemoryError is re-raised unwrapped; other decode errors become RuntimeError). I found 3 low-severity items: a test that will fail on pyarrow builds without lz4, a decode stream that is never closed, and a difference from main on malformed frames.

[peco-review-bot]

🔵 Low — test_convert_arrow_based_set_lz4_memory_error_is_not_wrapped patches pyarrow.CompressedInputStream, but that call only happens when _PYARROW_HAS_LZ4 is true. On a pyarrow build without the lz4 codec, the function uses the lz4.frame.decompress fallback and returns normally, so assertRaises(MemoryError) fails. Your PR description lists this setup as supported. test_convert_arrow_based_set_decodes_many_lz4_batches_in_one_call is already guarded; this test needs the same @unittest.skipUnless(utils._PYARROW_HAS_LZ4, ...) decorator.

[vuanhphung]

Fixed in c6891ff: test_convert_arrow_based_set_lz4_memory_error_is_not_wrapped now has the same @unittest.skipUnless(utils._PYARROW_HAS_LZ4, ...) guard.

[peco-review-bot]

I checked the current head and the fix isn't there yet. The only @unittest.skipUnless(utils._PYARROW_HAS_LZ4, ...) in tests/unit/test_thrift_backend.py is at line 2238, which guards test_convert_arrow_based_set_decodes_many_lz4_batches_in_one_call. Line 2291, just above test_convert_arrow_based_set_lz4_memory_error_is_not_wrapped at line 2292, has no guard. Maybe c6891ff hasn't been pushed to this branch, or the decorator ended up on a different test. Once the guard is directly above that test on the PR head, this thread is resolved.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low-severity note. The many-batch LZ4 path is well tested against real compressed data: 1, 2, 3 and 72 batches, corrupted frames, MemoryError passthrough, the no-codec fallback, and outputs much larger than the first read size. Errors are mapped to RuntimeError so the error type stays the same on both paths.