Skip to content

Commit 86b92d3

Browse files
ai: apply changes for #1007 (2 review threads)
Addresses: - #4419073409 at src/databricks/sql/result_set.py:216 - #4419073413 at tests/e2e/test_driver.py:997 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent 3cddc9a commit 86b92d3

4 files changed

Lines changed: 88 additions & 16 deletions

File tree

‎src/databricks/sql/result_set.py‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -196,20 +196,34 @@ def _row_buffer_advance(self, n: Optional[int]) -> Tuple[int, int]:
196196
return start, length
197197

198198
def _row_buffer_take_rows(self, n: Optional[int] = None) -> List[Row]:
199-
"""Pop up to ``n`` (all if None) converted rows from the buffer."""
199+
"""Pop up to ``n`` (all if None) converted rows from the buffer.
200+
201+
If the requested range contains a ``_FailedRow``, only the rows up to and
202+
including the first failed row are consumed and its error is raised. Rows
203+
after it stay in the buffer for the next fetch. The good rows before it in
204+
the same call are consumed with the failed fetch (a fetch either returns
205+
all of its rows or raises), so ``rownumber`` stays aligned with what is
206+
left in the buffer.
207+
"""
200208
if self._row_buffer_count() == 0:
201209
return []
202210
rows = self._row_buffer_rows
203-
start, length = self._row_buffer_advance(n)
211+
available = self._row_buffer_count()
212+
length = available if n is None else max(0, min(n, available))
213+
start = self._row_buffer_pos
214+
failed: Optional[_FailedRow] = None
215+
for i in range(start, start + length):
216+
row = rows[i]
217+
if isinstance(row, _FailedRow):
218+
failed = row
219+
length = i - start + 1
220+
break
221+
self._row_buffer_advance(length)
204222
if self._row_buffer_count() == 0:
205223
self._row_buffer_clear()
206-
taken = rows[start : start + length]
207-
for row in taken:
208-
if isinstance(row, _FailedRow):
209-
# The taken rows are consumed either way, matching how a failed
210-
# conversion behaved before fetchone() buffering.
211-
raise row.error.with_traceback(row.traceback)
212-
return taken
224+
if failed is not None:
225+
raise failed.error.with_traceback(failed.traceback)
226+
return rows[start : start + length]
213227

214228
@staticmethod
215229
def _raw_row_count(raw: Any) -> Optional[int]:

‎tests/e2e/test_driver.py‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -992,10 +992,16 @@ def _assert_rows_and_bound(self, rows, calls, arraysize):
992992
assert len(rows) == n
993993
assert [r[0] for r in rows] == list(range(n))
994994
assert all(r[1] == str(r[0]) for r in rows)
995-
# Refills are capped per server batch / CloudFetch chunk, so an exact
996-
# ceil(n/arraysize) bound is layout-dependent. Only distinguish "a few"
997-
# conversions from "~n" (one per row, issue #981).
998-
bound = n // 10
995+
# Each refill converts up to min(arraysize, _FETCHONE_BUFFER_MAX_ROWS)
996+
# rows, but refills are also capped at a server batch / CloudFetch chunk
997+
# boundary, which can add one short refill per boundary. Allow 2x the
998+
# ideal refill count plus a small constant for that slack — tight
999+
# enough to catch tiny refills, not just one conversion per row (#981).
1000+
from databricks.sql.result_set import _FETCHONE_BUFFER_MAX_ROWS
1001+
1002+
per_refill = max(min(arraysize, _FETCHONE_BUFFER_MAX_ROWS), 1)
1003+
ideal = -(-n // per_refill)
1004+
bound = 2 * ideal + 10
9991005
assert calls <= bound, (
10001006
"row conversion called {} times for {} rows (arraysize={}); "
10011007
"expected at most {} (issue #981)".format(calls, n, arraysize, bound)

‎tests/e2e/test_kernel_backend.py‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -206,9 +206,15 @@ def _assert_rows_and_bound(rows, calls):
206206
assert len(rows) == _ROW_BUFFER_N
207207
assert [r[0] for r in rows] == list(range(_ROW_BUFFER_N))
208208
assert all(r[1] == str(r[0]) for r in rows)
209-
# Server batch boundaries add short refills, so an exact ceil(N/arraysize)
210-
# bound is layout-dependent. Only distinguish "a few" from "~N" (issue #981).
211-
bound = _ROW_BUFFER_N // 10
209+
# Each refill converts up to min(arraysize, _FETCHONE_BUFFER_MAX_ROWS) rows;
210+
# server batch boundaries can add one short refill each. Allow 2x the ideal
211+
# refill count plus a small constant — tight enough to catch tiny refills,
212+
# not just one conversion per row (issue #981).
213+
from databricks.sql.result_set import _FETCHONE_BUFFER_MAX_ROWS
214+
215+
per_refill = max(min(_ROW_BUFFER_ARRAYSIZE, _FETCHONE_BUFFER_MAX_ROWS), 1)
216+
ideal = -(-_ROW_BUFFER_N // per_refill)
217+
bound = 2 * ideal + 10
212218
assert calls <= bound, (
213219
f"_convert_arrow_table called {calls} times for {_ROW_BUFFER_N} rows; "
214220
f"expected at most {bound} (issue #981)"

‎tests/unit/test_fetches.py‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,52 @@ def convert_failing_on_2(self_, table):
585585
self.assertTrue(any("1 of 4 row(s)" in m for m in logs.output))
586586
self.assertEqual(rs.rownumber, 10)
587587

588+
def _patch_convert_failing_on_2(self):
589+
from unittest.mock import patch
590+
591+
real_convert = ThriftResultSet._convert_columnar_table
592+
593+
def convert_failing_on_2(self_, table):
594+
rows = real_convert(self_, table)
595+
if any(r[0] == 2 for r in rows):
596+
raise ValueError("bad value")
597+
return rows
598+
599+
return patch.object(
600+
ThriftResultSet,
601+
"_convert_columnar_table",
602+
autospec=True,
603+
side_effect=convert_failing_on_2,
604+
)
605+
606+
def test_fetchmany_over_failed_buffered_row_keeps_rows_after_it(self):
607+
rs = self.make_columnar_result_set(10, arraysize=4)
608+
with self._patch_convert_failing_on_2(), self.assertLogs(
609+
"databricks.sql.result_set", level="WARNING"
610+
):
611+
# Buffer now holds rows 1, 2 (failed), 3.
612+
self.assertEqual(rs.fetchone()[0], 0)
613+
with self.assertRaises(ValueError):
614+
rs.fetchmany(3)
615+
# Consumed up to and including the failed row; row 3 stays buffered.
616+
self.assertEqual(rs.rownumber, 3)
617+
self.assertEqual(rs._row_buffer_count(), 1)
618+
self.assertEqual([r[0] for r in rs.fetchmany(3)], [3, 4, 5])
619+
self.assertEqual([r[0] for r in rs.fetchall()], [6, 7, 8, 9])
620+
self.assertEqual(rs.rownumber, 10)
621+
622+
def test_fetchall_over_failed_buffered_row_keeps_rows_after_it(self):
623+
rs = self.make_columnar_result_set(10, arraysize=4)
624+
with self._patch_convert_failing_on_2(), self.assertLogs(
625+
"databricks.sql.result_set", level="WARNING"
626+
):
627+
self.assertEqual(rs.fetchone()[0], 0)
628+
with self.assertRaises(ValueError):
629+
rs.fetchall()
630+
self.assertEqual(rs.rownumber, 3)
631+
self.assertEqual([r[0] for r in rs.fetchall()], list(range(3, 10)))
632+
self.assertEqual(rs.rownumber, 10)
633+
588634
def test_fetchone_convert_failure_on_every_row_reraises_per_row(self):
589635
from unittest.mock import patch
590636

0 commit comments

Comments
 (0)