[vuanhphung]

What type of PR is this?

• Other (performance)

Description

Two client-side costs on the Thrift backend showed up in the nightly driver benchmark:

• THttpClient.read() forwarded every primitive read of the pure-Python Thrift decoder to urllib3. For replies with many fields (wide result schemas, column-based metadata results) that per-call overhead dominated: decoding a 2,415-row getColumns reply took ~370 ms vs ~1.3 ms from a buffered body with the C decoder. The transport now reads the body from urllib3 in 1 MiB chunks (or a whole field if larger), like TBufferedTransport, and implements CReadableTransport. Because the body is never read whole, a 503/429 with a truncated body still reaches the status-based retry on urllib3 1.26, and the extra memory during decode is about one chunk or one field rather than the whole reply. The backend decodes replies with TBinaryProtocolAccelerated, which falls back to pure Python when Thrift's C extension isn't built. Requests are still encoded in pure Python, because the C encoder rejects structs nested deeper than about 64 levels, such as native array parameters nested 62 or more levels. String/container limits are unchanged; >15.6 MiB fields still decode on thrift 0.24 and 0.25.
• unsafe_logger.debug("...".format(payload)) stringified every request and response even though that logger only has a NullHandler by default (~25 ms per MB of inline Arrow). The payloads are now passed as %s args, so output is unchanged when a handler is attached.

Live A/B against a Medium PRO warehouse (p50 of 3 interleaved rounds, main → this PR):

Case	main	this PR
tables() across all catalogs (60k rows)	11.5 s	8.3 s
columns() on samples (2.5k rows)	1.27 s	1.10 s
100k-row inline Arrow result (cloud fetch off)	860 ms	610 ms
2000-column LIMIT 0 query, client time minus server time	592 ms	300 ms
SELECT 1	no change	no change

How is this tested?

• Unit tests
• E2E Tests
• Manually

New unit tests cover the chunked transport under both the C and the pure-Python protocol, a full RPC decoded through the C path, the truncated-503 retry, and the lazy log calls. The unit suite passes on thrift 0.24 and 0.25, and with the C extension disabled. The Thrift e2e tests in test_driver.py, test_complex_types.py, test_variant_types.py and test_parameterized_queries.py pass against a live warehouse. I didn't run the retry or long-running suites, or the UC volume and streaming-PUT tests, which need a volume fixture.

Related Tickets & Documents

N/A

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Looks good overall: the buffered CReadableTransport and the lazy %s logging are both correct, and the existing string/container limits are kept. One medium concern: the buffered reply body stays referenced after decoding, which roughly doubles memory held per connection for large inline results. There is also a minor inconsistency in the exception raised for a truncated reply.

[peco-review-bot]

Verdict: 2 Low

Looks good. Buffering the body and using the accelerated protocol are both correct, and the transport is closed in make_request's finally, which releases the buffer. There are two low-severity notes: peak memory now roughly doubles for large inline replies, and the "C path" test can pass without the C extension ever running.

[peco-review-bot]

✅ No issues identified by the review bot.

[aakash-saravanan-db]

Reviewed response buffering, accelerated protocol compatibility, and lazy logging. One native-parameter compatibility issue is noted inline. The backend suite passed with 179 tests and 95 subtests on each of Thrift 0.24 and 0.25.

[peco-review-bot]

Verdict: 2 Low

The change looks correct. I found two low-severity issues: the C encoder is disabled through a private thrift attribute, and the test guarding that only works when fastbinary is built. The chunked read/cstringio_refill logic correctly handles short reads, EOF, fields larger than a chunk, and switching between the pure-Python and C decode paths, and the lazy %s logging keeps the same output.

[peco-review-bot]

✅ No issues identified by the review bot.