bito-code-review[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4140859362
##########
tests/unit_tests/db_engine_specs/test_bigquery.py:
##########
@@ -832,6 +869,43 @@ def test_fetch_data_fallback_on_exception(mocker:
MockerFixture) -> None:
assert flask_g.bq_memory_limited_row_count == 2
[email protected]("bounded", [False, True])
[email protected]("failure_stage", ["probe", "second_batch",
"estimate"])
+def test_fetch_data_does_not_discard_sample_after_error(
+ mocker: MockerFixture, bounded: bool, failure_stage: str
+) -> None:
+ """A failure after consuming rows must not return only the cursor
remainder."""
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.db_engine_specs.exceptions import
SupersetDBAPIConnectionError
+ from superset.sql.execution.executor import _LimitedCursor
+
+ _, app = _patch_bq_fetch_deps(mocker)
+ mocker.patch("superset.db_engine_specs.bigquery.has_app_context",
return_value=True)
+ app.config = {"BQ_FETCH_MAX_MB": 1}
+ mocker.patch("superset.db_engine_specs.bigquery._BQ_INITIAL_SAMPLE_ROWS",
2)
+ mocker.patch.object(
+ BigQueryEngineSpec,
+ "get_dbapi_exception_mapping",
+ return_value={OSError: SupersetDBAPIConnectionError},
+ )
+ row = ("x" * (600_000 if failure_stage == "probe" else 300_000),)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Undocumented magic row sizes</b></div>
<div id="fix">
The 600_000/300_000 literals encode the 1 MiB `BQ_FETCH_MAX_MB` budget math
— two ~600 KB rows overflow it (EOF-probe path), ~300 KB rows keep
`remaining_rows > 0` (second-batch path) — but are undocumented. Sibling
`test_fetch_data_eof_probe_preserves_budgeted_rows` documents identical sizing
at line 809; add the same brief comment.
</div>
</div>
<small><i>Code Review Run #59108b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/db_engine_specs/test_bigquery.py:
##########
@@ -832,6 +869,43 @@ def test_fetch_data_fallback_on_exception(mocker:
MockerFixture) -> None:
assert flask_g.bq_memory_limited_row_count == 2
[email protected]("bounded", [False, True])
[email protected]("failure_stage", ["probe", "second_batch",
"estimate"])
+def test_fetch_data_does_not_discard_sample_after_error(
+ mocker: MockerFixture, bounded: bool, failure_stage: str
+) -> None:
+ """A failure after consuming rows must not return only the cursor
remainder."""
+ from superset.db_engine_specs.bigquery import BigQueryEngineSpec
+ from superset.db_engine_specs.exceptions import
SupersetDBAPIConnectionError
+ from superset.sql.execution.executor import _LimitedCursor
+
+ _, app = _patch_bq_fetch_deps(mocker)
+ mocker.patch("superset.db_engine_specs.bigquery.has_app_context",
return_value=True)
+ app.config = {"BQ_FETCH_MAX_MB": 1}
+ mocker.patch("superset.db_engine_specs.bigquery._BQ_INITIAL_SAMPLE_ROWS",
2)
+ mocker.patch.object(
+ BigQueryEngineSpec,
+ "get_dbapi_exception_mapping",
+ return_value={OSError: SupersetDBAPIConnectionError},
+ )
+ row = ("x" * (600_000 if failure_stage == "probe" else 300_000),)
+ cursor = mock.MagicMock()
+ cursor.description = [("n", "STRING", None, None, None, None, None)]
+ cursor.fetchmany.side_effect = [[row, row], OSError("read failed"), [row]]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead side_effect entries</b></div>
<div id="fix">
In the `estimate` parametrization, the patched `sys.getsizeof` raises before
the second `fetchmany`, so the `OSError` entry here is never consumed; the
trailing `[row]` is consumed in no stage (the EOF probe raises at call 2). The
shared `side_effect` list misstates each scenario's call sequence; derive it
from `failure_stage`.
</div>
</div>
<small><i>Code Review Run #59108b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
