bito-code-review[bot] commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3566668506
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -193,6 +193,67 @@ def mock_serialize_payload(payload, use_msgpack):
)
+def test_execute_sql_statements_mutates_before_split_by_default(
+ mocker: MockerFixture, app
+) -> None:
+ """
+ With the default `MUTATE_AFTER_SPLIT=False`, `execute_sql_statements`
should
+ mutate the whole, un-split query once before splitting it into individual
+ statement blocks, for engines that execute statements individually rather
+ than as one. Regression guard for issue #30169.
+ """
+ query = mocker.MagicMock()
+ query.limit = 1
+ query.database = mocker.MagicMock()
+ query.database.cache_timeout = 100
+ query.status = "RUNNING"
+ query.select_as_cta = False
+ query.database.allow_run_async = True
+ query.database.db_engine_spec.engine = "sqlite"
+ query.database.db_engine_spec.run_multiple_statements_as_one = False
+ query.database.db_engine_spec.allows_sql_comments = True
+
+ mutate_mock = mocker.patch.object(
+ query.database,
+ "mutate_sql_based_on_config",
+ side_effect=lambda sql, **kw: sql,
+ )
+
+ mocker.patch("superset.sql_lab.get_query", return_value=query)
+ mocker.patch("sys.getsizeof", return_value=10000000)
+ mocker.patch(
+ "superset.sql_lab._serialize_payload",
+ side_effect=lambda payload, use_msgpack: "serialized_payload",
+ )
+ mocker.patch("superset.sql_lab.db.session.refresh", return_value=None)
+ mocker.patch("superset.sql_lab.results_backend", return_value=True)
+
+ execute_sql_statements(
+ query_id=1,
+ rendered_query="SELECT 1; SELECT 2;",
+ return_results=True,
+ store_results=True,
+ start_time=None,
+ expand_data=False,
+ log_params={},
+ )
+
+ is_split_values = [
+ call.kwargs.get("is_split") for call in mutate_mock.call_args_list
+ ]
+ # The mutator is called once on the whole, un-split query before
splitting...
+ assert is_split_values[0] is False
+ first_call_sql = mutate_mock.call_args_list[0].args[0]
+ assert "1" in first_call_sql
+ assert "2" in first_call_sql
+ # Both statements are present in a single, un-split call.
+ assert first_call_sql.count("SELECT") == 2
+ # ...and once again per already-split statement (a no-op when
+ # `MUTATE_AFTER_SPLIT=False`, since `is_split=True` won't match the
config).
+ assert all(value is True for value in is_split_values[1:])
+ assert len(is_split_values) == 3
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding the missing mock for `execute_query` is
valid in a general sense, as unmocked external calls can lead to unexpected
behavior. However, based on your testing, the `db_engine_spec` is already a
`MagicMock`, which allows the call to proceed without error. Since the test
successfully exercises the mutator logic as intended, the current
implementation is functionally correct for this test case.
--
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]
