Skip to content

Commit 313aa5f

Browse files
ai: apply changes for #1007 (2 review threads)
Addresses: - #4419434164 at src/databricks/sql/result_set.py:383 - #4419434169 at src/databricks/sql/result_set.py:307 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent 62fb86e commit 313aa5f

2 files changed

Lines changed: 24 additions & 6 deletions

File tree

‎src/databricks/sql/result_set.py‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -302,18 +302,21 @@ def convert_range(lo: int, hi: int, error: Optional[Exception]) -> None:
302302
convert_range(0, n, batch_error)
303303
failed = sum(isinstance(r, _FailedRow) for r in rows)
304304
# One summary line, no traceback: the error itself still reaches the caller
305-
# when the failing row is fetched. The full traceback is only logged at DEBUG.
305+
# when the failing row is fetched. Only the exception type is logged here,
306+
# since conversion error messages often quote the offending result value;
307+
# the message and full traceback are only logged at DEBUG.
306308
# ``log_level`` lets the caller log this at WARNING only once per result set.
307309
logger.log(
308310
log_level,
309311
"fetchone(): failed to convert %d of %d row(s) in a batch; each failing "
310-
"row raises when fetched (%s: %s)",
312+
"row raises when fetched (%s)",
311313
failed,
312314
n,
313315
type(batch_error).__name__,
314-
batch_error,
315316
)
316-
logger.debug("fetchone(): batch conversion error", exc_info=batch_error)
317+
logger.debug(
318+
"fetchone(): batch conversion error: %s", batch_error, exc_info=batch_error
319+
)
317320
return rows
318321

319322
def _cap_to_current_chunk(self, size: int) -> int:
@@ -369,7 +372,11 @@ def _fetchone_buffered(
369372
If conversion yields a different number of rows than the raw chunk held,
370373
``InternalError`` is raised and that whole chunk is dropped (the queue and
371374
``rownumber`` have already moved past it). The result set should be
372-
considered unusable after that error.
375+
considered unusable after that error. The same holds when conversion
376+
raises one of ``_NON_DATA_CONVERSION_ERRORS`` (e.g. ``MemoryError``,
377+
``RecursionError``): the error is re-raised and the whole refill chunk
378+
(up to ``_FETCHONE_BUFFER_MAX_ROWS`` rows, still counted in ``rownumber``)
379+
is lost, so callers should not keep fetching from this result set.
373380
"""
374381
if self._row_buffer_count() == 0:
375382
cap = max(min(self.arraysize, _FETCHONE_BUFFER_MAX_ROWS), 1)
@@ -383,6 +390,9 @@ def _fetchone_buffered(
383390
except _NON_DATA_CONVERSION_ERRORS:
384391
# Resource / interpreter failures say nothing about individual
385392
# rows; retrying smaller slices would only repeat (or worsen) them.
393+
# The queue has already moved past ``raw`` and it is not buffered,
394+
# so the whole refill chunk is lost: as with the drift error below,
395+
# the result set is unusable after this.
386396
raise
387397
except Exception as batch_error:
388398
# The queue has already moved past this batch, so it can't be

‎tests/unit/test_fetches.py‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -738,6 +738,11 @@ def test_fetchone_convert_failure_warns_once_per_result_set(self):
738738
[r.levelno for r in summaries],
739739
[logging.WARNING] + [logging.DEBUG] * 8,
740740
)
741+
# The error message may quote result data, so it must stay out of WARNING.
742+
for r in logs.records:
743+
if r.levelno >= logging.WARNING:
744+
self.assertNotIn("bad value", r.getMessage())
745+
self.assertIn("ValueError", r.getMessage())
741746

742747
def test_fetchone_all_failing_batch_resets_refill_ramp(self):
743748
from unittest.mock import patch
@@ -821,7 +826,10 @@ def convert(self_, table):
821826
self.assertEqual(len(calls), 2 * 20 - 1)
822827
self.assertEqual(sum(1 for c in calls if c == 1), 20)
823828
self.assertTrue(any("20 of 20 row(s)" in m for m in logs.output))
824-
self.assertTrue(any("batch failure" in m for m in logs.output))
829+
# Only the exception type is logged at WARNING; the message (which may quote
830+
# result data) is kept out of it.
831+
self.assertTrue(any("(ValueError)" in m for m in logs.output))
832+
self.assertFalse(any("batch failure" in m for m in logs.output))
825833
self.assertEqual(rs.fetchone()[0], 20)
826834

827835
def test_fetchone_all_failing_max_batch_conversion_count_is_bounded(self):

0 commit comments

Comments
 (0)