[vuanhphung]

What type of PR is this?

• Other (performance)

Description

After a large inline Arrow result on Thrift is released (for example with use_cloud_fetch=False), the process keeps almost all of its memory. A 1.7 GB fetchall_arrow() left about 1.6 GB resident on the live warehouse. It is not a leak: after freeing a 562 MB result, glibc's malloc_stats() shows 590 MB of arena memory with 25 MB in use, and malloc_trim(0) (as a diagnostic only) brings RSS back to baseline. Each FetchResults row set (25 batches, ~3.7 MB here) was concatenated into a bytearray grown on the malloc heap. The Arrow table reads it zero-copy, so these buffers live as long as the result, interleaved with short-lived reply buffers, and glibc can't give the space back once something above them stays allocated.

This PR builds each row set's IPC stream in its own private anonymous mmap, which goes back to the OS when the table is released. The map is reserved from the compressed size (4x estimate; untouched pages cost no memory) and filled batch by batch as each batch is decompressed, so peak memory per row set is the same as before. If the estimate is too small, the map grows via mremap; once the stream is complete it is shrunk to its exact size. The map is used on Linux only, where glibc showed the retention. Streams under 128 KiB (glibc's default mmap threshold), mmap failures, and other platforms use the old bytearray. Decompression, the IPC read and errors are unchanged. Only convert_arrow_based_set_to_arrow_table in utils.py changes. #1032 uses the same technique for CloudFetch files; the two are independent, and this PR carries its own small mmap helper because cloudfetch/downloader.py can't import utils.py.

How is this tested?

• Unit tests
• E2E Tests
• Manually

New unit tests cover large sets (lz4 on and off) read from a private map, growth when the estimate is too small, the final shrink, the heap fallback when the map can't grow or mmap fails, non-Linux platforms, small sets staying on the heap, schema-only sets, and the table keeping its map alive after the inputs are gone. They also pass on pyarrow 14.0.2. The full unit suite passes. E2E: the TestPySQLCoreSuite fetch and Arrow tests (inline and CloudFetch) and test_complex_types.py pass. Manually, inline results (lz4 on and off) matched CloudFetch results for 2M rows of mixed types (nulls, decimals, timestamps, dates, arrays, structs, maps) via fetchall_arrow, fetchmany_arrow and fetchall.

Live Thrift warehouse, use_cloud_fetch=False, 3 fresh processes per cell interleaved with main, median RSS increase over the pre-query baseline:

Workload	Peak, main	Peak, this PR	After free, main	After free, this PR	Fetch time, main → PR
fetchall_arrow() 110 MB	125 MB	119 MB	104 MB	6 MB	2.79 → 2.80 s
fetchall_arrow() 562 MB	602 MB	582 MB	525 MB	12 MB	13.98 → 13.86 s
fetchall_arrow() 1.7 GB	1809 MB	1758 MB	1616 MB	25 MB	42.07 → 42.14 s
fetchmany_arrow(100k) loop, 562 MB	29 MB	17 MB	21 MB	5 MB	14.37 → 13.87 s
fetchall() rows, 110 MB	1661 MB	1655 MB	182 MB	75 MB	8.47 → 8.54 s

Conversion time, measured in-process during the same runs, is unchanged: 2.36 s vs 2.28 s for the 1.7 GB result. A single 51 MB row set (wide rows) converts with no transient memory on both main and this PR (31 ms vs 32-33 ms).

Related Tickets & Documents

Same technique as #1032 (CloudFetch). Both edit CHANGELOG.md under Unreleased, so whichever merges second has a one-line conflict there.

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Low

Looks good. There is one low note: the lz4 path now briefly needs about twice the row set's memory while the batches are joined. The mmap handling is otherwise correct: anonymous MAP_PRIVATE with MAP_ANONYMOUS added implicitly by CPython, py_buffer keeps the mapping alive, and failures fall back to the heap. The tests cover the large, small, failure and schema-only paths.

[peco-review-bot]

Verdict: 2 Low

Looks good: the change is correct and well contained. My two notes are both low severity: the tests for the grow and fallback paths don't check that those paths actually run, and the 4x over-reservation is never trimmed after the map is filled.

[peco-review-bot]

Verdict: 2 Low

Looks good: the mmap-backed buffer is correct. Buffer lifetime is tied to the Arrow table through py_buffer, the fallbacks are complete, and the tests are thorough. Two low-severity concerns: each live row set holds its own mapping (mapping-count limit), and the heap fallback when the map can't grow makes extra transient copies.

[peco-review-bot]

Verdict: 1 Low · 2 Nit

The mmap-backed stream buffer is correct: Thrift's arrowBatches is always a list, so iterating it twice is safe, and resizing only happens before the buffer is handed to pyarrow, so BufferError can't occur. My three findings are all low: some new tests break on non-Linux machines, the grow-or-fall-back step is more aggressive than it needs to be, and the keep-alive test can't actually fail.

Other findings

• ⚪ Nit — Several new tests assume the mmap path is taken, but none of them patch _INLINE_ARROW_USE_MMAP to True:
• test_large_set_is_read_from_a_private_memory_map calls mmap_spy.assert_called_once().
• test_map_grows_... reads _RecordingMap.resizes[-1], which raises IndexError when the list is empty.
• test_map_is_shrunk_... asserts len(resizes) == 1.
• test_falls_back_... asserts attempts is non-empty.

On macOS or Windows the flag is False, so these tests fail. CI runs only on Linux, so this won't break CI, but contributors running pytest tests/unit locally on a Mac will see failures. Fix: mark them @pytest.mark.skipif(not sys.platform.startswith("linux"), ...), or patch _INLINE_ARROW_USE_MMAP=True and skip only when mmap.MAP_PRIVATE is missing.

• ⚪ Nit — test_table_keeps_the_mapping_alive only deletes batches and schema_bytes. Neither of them backs the mapping: the _ArrowStreamBuffer and its mmap are locals inside convert_arrow_based_set_to_arrow_table and are already unreachable once it returns. So the test passes as long as the call returns at all, and doesn't show that the table's buffer is what keeps the map alive. A stronger version would hold a weakref to the mmap object (via the spy/subclass pattern used in the other tests), assert it is still alive while result exists, and assert it is collected after del result; gc.collect(). That would also cover the claim that memory is returned when the table is freed.

[peco-review-bot]

🔵 Low — On overflow the map grows straight to max(end, 2 * len(self._map)). If that doubled size fails (strict overcommit, ENOMEM), the code never retries with the smaller end. It copies the whole stream onto the heap instead, and for a moment both the map and the bytearray hold a full copy, so the reported peak-memory guarantee doesn't hold. Retrying resize(end) before falling back would make the fallback rarer and keep peak memory closer to what the PR describes.