Skip to content

Commit 2163819

Browse files
vuanhphungIsaac
andcommitted
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>
1 parent 44077a0 commit 2163819

2 files changed

Lines changed: 27 additions & 6 deletions

File tree

‎src/databricks/sql/auth/thrift_http_client.py‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -175,15 +175,17 @@ def cstringio_buf(self):
175175

176176
def cstringio_refill(self, partialread, reqlen):
177177
# As in TBufferedTransport: read one more chunk, or all of a larger field.
178-
data = partialread
178+
parts, have = [partialread], len(partialread)
179179
if reqlen < self._READ_CHUNK_SIZE:
180-
data += self.__resp.read(self._READ_CHUNK_SIZE)
181-
while len(data) < reqlen:
182-
chunk = self.__resp.read(reqlen - len(data))
180+
parts.append(self.__resp.read(self._READ_CHUNK_SIZE))
181+
have += len(parts[-1])
182+
while have < reqlen:
183+
chunk = self.__resp.read(reqlen - have)
183184
if not chunk:
184185
raise EOFError()
185-
data += chunk
186-
self.__rbuf = BytesIO(data)
186+
parts.append(chunk)
187+
have += len(chunk)
188+
self.__rbuf = BytesIO(b"".join(parts))
187189
return self.__rbuf
188190

189191
def isOpen(self):

‎tests/unit/test_thrift_backend.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,25 @@ def test_http_client_reply_is_decoded_by_thrift_c_extension(self):
503503
self.assertEqual(result.results.arrowBatches[0].batch, batch)
504504
protocol._fast_decode.assert_called()
505505

506+
def test_http_client_decodes_reply_from_short_urllib3_reads(self):
507+
"""urllib3 may return fewer bytes than asked for; a field spanning many
508+
short reads must still be assembled correctly."""
509+
small = [bytes([i % 256]) * 1000 for i in range(300)]
510+
big = b"y" * (4 * 1024 * 1024)
511+
reply = self._fetch_results_reply(small + [big])
512+
for protocol_class in self._protocol_classes():
513+
with self.subTest(protocol=protocol_class.__name__):
514+
http_client, mock_resp = self._http_client_with_reply(b"")
515+
stream = BytesIO(reply)
516+
mock_resp.read.side_effect = lambda amt: stream.read(min(amt, 65536))
517+
_, client = self._protocol_client(http_client, protocol_class)
518+
519+
result = client.FetchResults(self._fetch_results_req())
520+
521+
self.assertEqual(
522+
[b.batch for b in result.results.arrowBatches], small + [big]
523+
)
524+
506525
def test_http_client_truncated_reply_raises_eof(self):
507526
reply = self._fetch_results_reply([b"x" * 100])
508527
for protocol_class in self._protocol_classes():

0 commit comments

Comments
 (0)