codeant-ai-for-open-source[bot] commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3565603308
##########
tests/unit_tests/sql/execution/test_celery_task.py:
##########
@@ -282,9 +282,12 @@ def test_prepare_statement_blocks_single_statement(
"""Test statement block preparation for single statement."""
from superset.sql.execution.celery_task import _prepare_statement_blocks
+ mock_database.mutate_sql_based_on_config = lambda sql, **kw: sql
Review Comment:
**Suggestion:** Replace this inline lambda with a typed helper function so
the SQL mutator callable has explicit parameter and return type hints.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This new test code assigns an inline lambda with no type annotations to a
callable that is being added in the PR. The custom rule requires type hints on
new or modified Python code where they can be added, so this is a real
violation.
</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=23e05c52a8bd47dc9fe5a39f0f22c695&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=23e05c52a8bd47dc9fe5a39f0f22c695&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_celery_task.py
**Line:** 285:285
**Comment:**
*Custom Rule: Replace this inline lambda with a typed helper function
so the SQL mutator callable has explicit parameter and return type hints.
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=b0fe6cb325c492cdd644da3516d331c2b1b67203ebf7008c61834bd3cb121c13&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=b0fe6cb325c492cdd644da3516d331c2b1b67203ebf7008c61834bd3cb121c13&reaction=dislike'>๐</a>
##########
tests/unit_tests/sql/execution/test_celery_task.py:
##########
@@ -309,13 +315,43 @@ def test_prepare_statement_blocks_run_as_one(
from superset.sql.execution.celery_task import _prepare_statement_blocks
mock_database.db_engine_spec.run_multiple_statements_as_one = True
+ mock_database.mutate_sql_based_on_config = lambda sql, **kw: sql
sql = "SELECT * FROM users; SELECT * FROM orders;"
- script, blocks = _prepare_statement_blocks(sql,
mock_database.db_engine_spec)
+ script, blocks = _prepare_statement_blocks(
+ sql, mock_database.db_engine_spec, mock_database
+ )
assert len(blocks) == 1
+def test_prepare_statement_blocks_mutates_before_split_when_configured(
+ app_context: None, mock_database: MagicMock, mocker: MockerFixture
+) -> None:
+ """
+ `MUTATE_AFTER_SPLIT=False` should mutate the whole, un-split query before
+ it gets broken into per-statement blocks, for engines that execute
+ statements individually. Regression guard for issue #30169.
+ """
+ from superset.sql.execution.celery_task import _prepare_statement_blocks
+
+ mocker.patch.dict(current_app.config, {"MUTATE_AFTER_SPLIT": False})
+ mutate_mock = mocker.patch.object(
+ mock_database,
+ "mutate_sql_based_on_config",
+ side_effect=lambda sql, **kw: f"-- mutated\n{sql}",
Review Comment:
**Suggestion:** Use a separately defined typed function for the mock side
effect so input and output types are explicitly annotated. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This added lambda is also untyped and is part of newly introduced test code.
Since the rule flags new or modified Python code that omits type hints where
applicable, this is a valid violation.
</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=553d56e75d77479c81202b0e4a98f4aa&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=553d56e75d77479c81202b0e4a98f4aa&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_celery_task.py
**Line:** 342:342
**Comment:**
*Custom Rule: Use a separately defined typed function for the mock side
effect so input and output types are explicitly annotated.
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=f5c84cdc2ecc33d742ecfc550ab6acad2a8fec933619cd731ef265742d32dab5&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=f5c84cdc2ecc33d742ecfc550ab6acad2a8fec933619cd731ef265742d32dab5&reaction=dislike'>๐</a>
##########
superset/sql/execution/celery_task.py:
##########
@@ -139,6 +143,19 @@ def _prepare_statement_blocks(
if db_engine_spec.run_multiple_statements_as_one:
blocks =
[parsed_script.format(comments=db_engine_spec.allows_sql_comments)]
else:
+ if not app.config["MUTATE_AFTER_SPLIT"]:
+ # `MUTATE_AFTER_SPLIT=False` means the mutator should see the
whole,
+ # un-split query, but this engine executes statements individually.
+ # Mutate the whole block up front and re-parse it, so the
per-statement
+ # split below (and the later per-statement mutation call in
+ # `execute_sql_with_cursor`, which is a no-op here since its
+ # `is_split=True` no longer matches the config) operate on the
+ # already-mutated SQL.
+ mutated_sql = database.mutate_sql_based_on_config(
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced
local variable so the new code fully complies with required type hints.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The new local variable `mutated_sql` is introduced without a type hint even
though its value is clearly a string, so this is a real violation of the Python
type-hint rule for newly added code.
</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=59d955ada1854a4dbd5c33c5c04b9a45&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=59d955ada1854a4dbd5c33c5c04b9a45&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/execution/celery_task.py
**Line:** 154:154
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced
local variable so the new code fully complies with required type hints.
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=2d9d2c98006dd6f154d5be0ca9d084bca6991d652ead951e3aad6005a03d0ff9&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=2d9d2c98006dd6f154d5be0ca9d084bca6991d652ead951e3aad6005a03d0ff9&reaction=dislike'>๐</a>
##########
tests/unit_tests/models/core_test.py:
##########
@@ -262,6 +262,44 @@ def test_table_column_database() -> None:
assert TableColumn(database=database).database is database
[email protected](
+ "is_split,mutate_after_split,expect_mutated",
+ [
+ # A split-out statement is mutated only when the mutator is meant to
run
+ # after the split, and an un-split block only when it runs before.
+ (True, True, True),
+ (True, False, False),
+ (False, False, True),
+ (False, True, False),
+ ],
+)
+def test_mutate_sql_based_on_config_respects_is_split(
+ app_context: None,
+ mocker: MockerFixture,
+ is_split: bool,
+ mutate_after_split: bool,
+ expect_mutated: bool,
+) -> None:
+ """
+ `mutate_sql_based_on_config` fires `SQL_QUERY_MUTATOR` only when the call
+ site's `is_split` matches the `MUTATE_AFTER_SPLIT` config. Regression guard
+ for issue #30169, where SQL Lab always passed the default `is_split=False`
+ and so never mutated when `MUTATE_AFTER_SPLIT=True`.
+ """
+ database = Database(database_name="db", sqlalchemy_uri="sqlite://")
+ mocker.patch.dict(
+ current_app.config,
+ {
+ "SQL_QUERY_MUTATOR": lambda sql, **kwargs: f"-- mutated\n{sql}",
Review Comment:
**Suggestion:** Replace the inline mutator lambda with a named callable that
includes explicit type hints for its parameters and return value. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The new lambda introduces a callable without type hints for its parameters
or return value, which matches the Python type-hint rule for newly added code.
</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=bd46bc2074704029858e3d0f07bd1235&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=bd46bc2074704029858e3d0f07bd1235&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/models/core_test.py
**Line:** 293:293
**Comment:**
*Custom Rule: Replace the inline mutator lambda with a named callable
that includes explicit type hints for its parameters and return value.
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=045d1724aefbbf5b5b57ce8b8bfab946526eabe82bb3c334a945d9cde9f8ef10&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=045d1724aefbbf5b5b57ce8b8bfab946526eabe82bb3c334a945d9cde9f8ef10&reaction=dislike'>๐</a>
##########
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:
Review Comment:
**Suggestion:** Add a type annotation for the `app` parameter in this newly
introduced test function signature. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
The new test function introduces an unannotated `app` parameter, which
violates the Python type-hint requirement for new or modified code.
</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=238d8869f8e44570a1f57c3500afb698&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=238d8869f8e44570a1f57c3500afb698&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_lab_test.py
**Line:** 196:198
**Comment:**
*Custom Rule: Add a type annotation for the `app` parameter in this
newly introduced test function signature.
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=7c69b0508b087d62ecfd468b13a0921fb735f1c0aff4c30e26cef99dd72c4902&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41127&comment_hash=7c69b0508b087d62ecfd468b13a0921fb735f1c0aff4c30e26cef99dd72c4902&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]
