codeant-ai-for-open-source[bot] commented on code in PR #40138:
URL: https://github.com/apache/superset/pull/40138#discussion_r3245270572
##########
tests/unit_tests/sql/parse_tests.py:
##########
@@ -1164,6 +1164,51 @@ def test_has_mutation(engine: str, sql: str, expected:
bool) -> None:
assert SQLScript(sql, engine).has_mutation() == expected
[email protected](
+ "engine",
+ ["oracle", "postgresql", "trino", "presto", "hive", "base"],
+)
+def test_with_clause_containing_union_is_not_mutating(engine: str) -> None:
+ """
+ Regression for #25659: a SELECT with a WITH clause whose CTEs contain
+ UNION (or UNION ALL) must not be classified as mutating, on any dialect.
+
+ The original bug surfaced on Oracle, where saving a virtual dataset built
+ from such a query failed with "Only `SELECT` statements are allowed". The
+ parser was misclassifying the WITH+UNION construct as DML.
+
+ Multiple dialects are exercised because the bug arose from sqlglot's
+ per-dialect AST shape — Oracle's representation of the same query may
+ differ from Postgres/Trino, and a fix that only touches one dialect
+ leaves the others exposed to the same regression.
+ """
+ sql = """
+ WITH set1 AS (SELECT 1 AS n UNION SELECT 2),
+ set2 AS (SELECT * FROM set1)
+ SELECT * FROM set2
+ """
+ assert not SQLScript(sql, engine).has_mutation(), (
+ f"WITH+UNION misclassified as mutating on {engine!r}; "
+ "this would block the query from being saved as a virtual dataset."
+ )
+
+
+def test_with_clause_containing_union_all_is_not_mutating_oracle() -> None:
+ """
+ Companion to test_with_clause_containing_union_is_not_mutating: the
+ original bug report (#25659) used the exact Oracle-flavored shape below
+ (``SYSDATE FROM DUAL`` is the Oracle no-op for "now"). Pinning the
+ verbatim repro guards against a future dialect-specific regression that
+ a generic ``SELECT 1`` test might miss.
+ """
+ sql = """
+ WITH SET1 AS (SELECT SYSDATE FROM DUAL UNION SELECT SYSDATE FROM DUAL),
Review Comment:
**Suggestion:** The test named for `UNION ALL` never uses `UNION ALL`; it
uses plain `UNION`. This leaves the Oracle `WITH ... UNION ALL ...` regression
path untested, so CI can pass even if `UNION ALL` is still misclassified as
mutating. Update this SQL to use `UNION ALL` so the test actually validates the
intended bug scenario. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Oracle virtual dataset save rejects SELECT with UNION ALL.
- ⚠️ Regression tests miss UNION ALL mutating classification bug.
- ⚠️ CI stays green despite Oracle UNION ALL regression.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `tests/unit_tests/sql/parse_tests.py` and locate
`test_with_clause_containing_union_all_is_not_mutating_oracle` (added around
lines
1196–1209 in the PR diff; confirmed in the current file by `def
test_with_clause_containing_union_all_is_not_mutating_oracle()` at offset
block starting
line 7 from the `Read` tool).
2. Within that test, inspect the SQL string assigned to `sql`; per the PR
diff line 1205
and the current file (`Read` of `tests/unit_tests/sql/parse_tests.py` lines
15–18), it is
`WITH SET1 AS (SELECT SYSDATE FROM DUAL UNION SELECT SYSDATE FROM DUAL),` —
note the
keyword is `UNION`, not `UNION ALL`, despite the test name and docstring
claiming to pin a
UNION ALL repro.
3. In `superset/sql/parse.py` at the `SQLScript` class (found via Grep and
read with
`Read`, around class definition offset line 1 and `has_mutation` at offset
lines 46–52),
`SQLScript.has_mutation()` returns `any(statement.is_mutating() for
statement in
self.statements)`, so this test is the regression guard for how
`has_mutation()`
classifies the `WITH ... UNION ALL ...` Oracle query.
4. In `superset/connectors/sqla/utils.py` (lines 120–139 from `Read`),
`parsed_script.has_mutation()` is used to enforce `"Only \`SELECT\`
statements are
allowed"` when creating virtual datasets; a regression that incorrectly
marks `WITH ...
UNION ALL ...` as mutating but correctly treats `WITH ... UNION ...` as
non-mutating would
still block real Oracle queries while
`test_with_clause_containing_union_all_is_not_mutating_oracle` passes,
because it never
exercises `UNION ALL` in its SQL.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fsql%2Fparse_tests.py%0A%2A%2ALine%3A%2A%2A%201205%3A1205%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20test%20named%20for%20%60UNION%20ALL%60%20never%20uses%20%60UNION%20ALL%60%3B%20it%20uses%20plain%20%60UNION%60.%20This%20leaves%20the%20Oracle%20%60WITH%20...%20UNION%20ALL%20...%60%20regression%20path%20untested%2C%20so%20CI%20can%20pass%20even%20if%20%60UNION%20ALL%60%20is%20still%20misclassified%20as%20mutating.%20Update%20this%20SQL%20to%20use%20%60UNION%20ALL%60%20so%20the%20test%20actually%20validates%20the%20intended%20bug%20scenario.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%
20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fsql%2Fparse_tests.py%0A%2A%2ALine%3A%2A%2A%201205%3A1205%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20test%20named%20for%20%60UNION%20ALL%60%20never%20uses%20%60UNION%20ALL%60%3B%20it%20uses%20plain%20%60UNION%60.%20This%20leaves%20the%20Oracle%20%60WITH%20...%20UNION%20ALL%20...%60%20regression%20path%20untested%2C%20so%20CI%20can%20pass%20even%20if%20%60UNION%20ALL%60%20is%20still%20misclassified%20as%20mutating.%20Update%20this%20SQL%20to%20use%20%60UNION%20ALL%60%20so%20t
he%20test%20actually%20validates%20the%20intended%20bug%20scenario.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/parse_tests.py
**Line:** 1205:1205
**Comment:**
*Incomplete Implementation: The test named for `UNION ALL` never uses
`UNION ALL`; it uses plain `UNION`. This leaves the Oracle `WITH ... UNION ALL
...` regression path untested, so CI can pass even if `UNION ALL` is still
misclassified as mutating. Update this SQL to use `UNION ALL` so the test
actually validates the intended bug scenario.
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%2F40138&comment_hash=3aafe4254078748db38c9edf48b22b857b970b6fc683638307de8c09d28baa66&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40138&comment_hash=3aafe4254078748db38c9edf48b22b857b970b6fc683638307de8c09d28baa66&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]
