bito-code-review[bot] commented on code in PR #44604:
URL: https://github.com/apache/superset/pull/44604#discussion_r4141002716
##########
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:
<!-- Bito Reply -->
The suggestion to document the magic row sizes is valid and improves code
maintainability by providing context for the 600,000 and 300,000 literals.
Applying this comment ensures consistency with the existing documentation in
the sibling test case.
**tests/unit_tests/db_engine_specs/test_bigquery.py**
```
row = ("x" * (600_000 if failure_stage == "probe" else 300_000),) # 600 KB
rows exceed 1 MiB budget; 300 KB rows leave room
```
##########
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:
<!-- Bito Reply -->
The suggestion provided by the reviewer is valid and improves the test code.
It correctly identifies that the shared `side_effect` list in the
parametrization does not accurately reflect the execution flow for all
`failure_stage` scenarios, as some stages cause the test to fail before
consuming all elements in the list. Applying this change ensures the test logic
precisely matches the expected behavior for each failure scenario.
--
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]
