codeant-ai-for-open-source[bot] commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3578635944
##########
superset/sql_lab.py:
##########
@@ -474,12 +474,55 @@ def execute_sql_statements( # noqa: C901
for statement in parsed_script.statements:
apply_limit(query, statement)
- # some databases (like BigQuery and Kusto) do not persist state across
mmultiple
+ # some databases (like BigQuery and Kusto) do not persist state across
multiple
# statements if they're run separately (especially when using `NullPool`),
so we run
# the query as a single block.
if db_engine_spec.run_multiple_statements_as_one:
- blocks =
[parsed_script.format(comments=db_engine_spec.allows_sql_comments)]
+ if app.config["MUTATE_AFTER_SPLIT"]:
+ # These engines never actually execute statements individually, so
the
+ # per-block mutation call further down (whose `is_split` is always
+ # `False` here) would never fire. Mutate each statement here,
before
+ # joining them into the single block this engine requires, so
+ # `MUTATE_AFTER_SPLIT=True` still applies the mutator per
statement.
+ blocks = [
+ ";\n".join(
+ database.mutate_sql_based_on_config(
+
statement.format(comments=db_engine_spec.allows_sql_comments),
+ is_split=True,
+ )
+ for statement in parsed_script.statements
+ )
+ ]
Review Comment:
**Suggestion:** Add an explicit type annotation for the newly introduced
`blocks` local variable (for example as a list of strings) at its first
assignment so the new branch logic keeps full type-hint coverage. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The new `blocks` local variable is assigned a list of strings in the added
branch without any type annotation, which matches the rule requiring type hints
for newly added Python variables that can be annotated.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8bc76cc422114f61adb3fa9d6b0397bd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8bc76cc422114f61adb3fa9d6b0397bd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/sql_lab.py
**Line:** 487:495
**Comment:**
*Custom Rule: Add an explicit type annotation for the newly introduced
`blocks` local variable (for example as a list of strings) at its first
assignment so the new branch logic keeps full type-hint coverage.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=7cfead9c3aafec5369861a0c3f08d04126d6e8a2174e3f617064958e54fdd7fb&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=7cfead9c3aafec5369861a0c3f08d04126d6e8a2174e3f617064958e54fdd7fb&reaction=dislike'>๐</a>
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -871,6 +871,45 @@ def test_execute_applies_sql_mutator(
mutate_mock.assert_called()
[email protected]("is_split", [True, False])
+def test_execute_sql_with_cursor_forwards_is_split(
+ mocker: MockerFixture,
+ database: Database,
+ app_context: None,
+ mock_db_session: MagicMock,
+ mock_query: MagicMock,
+ is_split: bool,
+) -> None:
+ """
+ `execute_sql_with_cursor` must forward `is_split` to the SQL mutator.
+
+ `Database.mutate_sql_based_on_config` only fires `SQL_QUERY_MUTATOR` when
+ `is_split == MUTATE_AFTER_SPLIT`, so passing the wrong value silently skips
+ mutation (the SQL Lab bug behind issue #30169). This guards the contract.
+ """
+ from superset.sql.execution.executor import execute_sql_with_cursor
+
+ mutate_mock = mocker.patch.object(
+ database, "mutate_sql_based_on_config", side_effect=lambda sql, **kw:
sql
+ )
+ mocker.patch.object(database.db_engine_spec, "execute")
+ mocker.patch.object(database.db_engine_spec, "fetch_data",
return_value=[(1,)])
+ mocker.patch("superset.result_set.SupersetResultSet",
return_value=MagicMock())
+
+ cursor = create_mock_cursor(["id"], data=[(1,)])
Review Comment:
**Suggestion:** Add a type annotation for this newly added cursor variable
since its type is known and can be declared. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a newly added local variable in modified Python code, and it is
annotatable. The code omits a type hint, so it violates the projectโs Python
type-hint requirement.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=acabd384907440b590c282b18ffcbe0e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=acabd384907440b590c282b18ffcbe0e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/sql/execution/test_executor.py
**Line:** 899:899
**Comment:**
*Custom Rule: Add a type annotation for this newly added cursor
variable since its type is known and can be declared.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=006501ff45fe4301f6d4c4f23c37b7850a029846044f1c3eaf915eed597cd60a&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=006501ff45fe4301f6d4c4f23c37b7850a029846044f1c3eaf915eed597cd60a&reaction=dislike'>๐</a>
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -871,6 +871,45 @@ def test_execute_applies_sql_mutator(
mutate_mock.assert_called()
[email protected]("is_split", [True, False])
+def test_execute_sql_with_cursor_forwards_is_split(
+ mocker: MockerFixture,
+ database: Database,
+ app_context: None,
+ mock_db_session: MagicMock,
+ mock_query: MagicMock,
+ is_split: bool,
+) -> None:
+ """
+ `execute_sql_with_cursor` must forward `is_split` to the SQL mutator.
+
+ `Database.mutate_sql_based_on_config` only fires `SQL_QUERY_MUTATOR` when
+ `is_split == MUTATE_AFTER_SPLIT`, so passing the wrong value silently skips
+ mutation (the SQL Lab bug behind issue #30169). This guards the contract.
+ """
+ from superset.sql.execution.executor import execute_sql_with_cursor
+
+ mutate_mock = mocker.patch.object(
+ database, "mutate_sql_based_on_config", side_effect=lambda sql, **kw:
sql
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced
local mock variable to satisfy the projectโs type-hint requirement for
annotatable variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is newly added Python code that introduces a local variable without a
type annotation, and the variable is clearly annotatable as a mock object. That
matches the type-hint rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d0273c48cb924e0bbdbdfe5fae047396&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d0273c48cb924e0bbdbdfe5fae047396&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/sql/execution/test_executor.py
**Line:** 892:894
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced
local mock variable to satisfy the projectโs type-hint requirement for
annotatable variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=d4157c4c670057a7e4f8063e3f2460190adaf90b3fc5bcf957aa91f0172344e0&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=d4157c4c670057a7e4f8063e3f2460190adaf90b3fc5bcf957aa91f0172344e0&reaction=dislike'>๐</a>
--
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]
