Repository navigation
Commit 307c48d
perf(thrift): buffer reply body, use accelerated protocol, log payloads lazily (#1020)
* perf(thrift): buffer reply body, use accelerated protocol, log payloads lazily
THttpClient.read() forwarded every primitive read of the pure-Python thrift
decoder to urllib3, which dominated decode time for wide result schemas and
column-based metadata results. Read the reply body once, expose it as a
CReadableTransport, and decode with TBinaryProtocolAccelerated (falls back to
pure Python when thrift's C extension is unavailable).
unsafe_logger.debug() pre-formatted every request and response with
str.format, stringifying whole payloads (including inline Arrow batches) even
though the logger only has a NullHandler by default. Pass them as %s args.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
* test(thrift): assert the buffered reply is released after make_request
Locks in that the transport's reply buffer is cleared once a successful RPC
returns (make_request closes the transport in its finally block).
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
* style: black-format new thrift transport test
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
* test(thrift): assert the C decoder runs; document reply-buffer trade-off
TBinaryProtocolAccelerated silently falls back to pure Python when thrift's
C extension isn't built, so add a test (skipped without fastbinary) that
asserts the C decoder actually ran on the buffered body. Share reply/request
builders across the transport tests, and note in the transport that the raw
reply is held alongside the decoded objects until close().
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
* test(thrift): check C-encoded requests match pure Python; qualify buffer bound
TBinaryProtocolAccelerated also moves request encoding to C. Add a test
(skipped without fastbinary) that builds OpenSession, ExecuteStatement (with
native parameters and query tags), GetTables and GetColumns requests through
the backend and checks both encoders produce identical bytes. Clarify that the
buffered reply is bounded by maxBytes only for fetches.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
* perf(thrift): read the reply in 1 MiB chunks instead of buffering it whole
Reading the whole body in flush() held a second copy of it during decode
and, on urllib3 1.26, turned a truncated 503/429 body into a ProtocolError
that skipped the status-based retry. The transport now refills from urllib3
in 1 MiB chunks (or a whole field if larger), like TBufferedTransport, so
truncated bodies fail inside decode as on main. Peak heap for a 100 MiB
multi-batch reply drops from ~200 to ~103 MiB at the same speed.
Tests now run the decode paths under both the C and the pure-Python
protocol, and cover the truncated-503 retry.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
* perf(thrift): join refill pieces once instead of concatenating per read
If urllib3 returns short reads for a field larger than a chunk, bytes +=
recopied everything read so far on every read. Collect the pieces and join
them once. Adds a test that decodes a reply from short reads on both protocols.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
* fix(thrift): keep pure-Python request encoding, decode replies in C
TBinaryProtocolAccelerated also moved request encoding to fastbinary, whose
recursion limit rejects a native array parameter nested 62+ levels once it
is wrapped in ExecuteStatement_args. Main's pure-Python encoder accepts it,
and a warehouse runs such queries at depths 62 and 63. Clear the protocol's
_fast_encode so requests serialize as before, while replies still decode in
C. Requests are small, so this has no measurable cost.
Replace the C-vs-Python request byte comparison with a test that sends a
63-level nested parameter through make_request and checks the wire bytes.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
* refactor(thrift): split decode/encode protocols via Client(iprot, oprot)
Instead of clearing thrift's private _fast_encode attribute, pass the
generated client a TBinaryProtocolAccelerated for replies and a plain
TBinaryProtocol for requests. The nested-parameter test now also checks the
protocol types, so it guards the split where fastbinary isn't built.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
---------
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>1 parent 4394c11 commit 307c48d
4 files changed
Lines changed: 367 additions & 7 deletions
File tree
- src/databricks/sql
- auth
- backend
- tests/unit
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
5 | 6 | | |
6 | 7 | | |
7 | 8 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
| 14 | + | |
14 | 15 | | |
15 | 16 | | |
16 | 17 | | |
| |||
22 | 23 | | |
23 | 24 | | |
24 | 25 | | |
25 | | - | |
| 26 | + | |
26 | 27 | | |
27 | 28 | | |
28 | 29 | | |
| |||
94 | 95 | | |
95 | 96 | | |
96 | 97 | | |
| 98 | + | |
97 | 99 | | |
98 | 100 | | |
99 | 101 | | |
| |||
153 | 155 | | |
154 | 156 | | |
155 | 157 | | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
156 | 163 | | |
157 | 164 | | |
158 | | - | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
159 | 190 | | |
160 | 191 | | |
161 | 192 | | |
| |||
206 | 237 | | |
207 | 238 | | |
208 | 239 | | |
| 240 | + | |
209 | 241 | | |
210 | 242 | | |
211 | 243 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
253 | 253 | | |
254 | 254 | | |
255 | 255 | | |
256 | | - | |
| 256 | + | |
| 257 | + | |
257 | 258 | | |
258 | 259 | | |
259 | 260 | | |
260 | 261 | | |
261 | | - | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
262 | 268 | | |
263 | 269 | | |
264 | 270 | | |
| |||
443 | 449 | | |
444 | 450 | | |
445 | 451 | | |
446 | | - | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
447 | 455 | | |
448 | 456 | | |
449 | 457 | | |
| |||
457 | 465 | | |
458 | 466 | | |
459 | 467 | | |
460 | | - | |
| 468 | + | |
461 | 469 | | |
462 | 470 | | |
463 | 471 | | |
| |||
0 commit comments