codeant-ai-for-open-source[bot] commented on code in PR #38742: URL: https://github.com/apache/superset/pull/38742#discussion_r2959753535
########## tests/unit_tests/sql/test_firebolt_dialect.py: ########## @@ -0,0 +1,58 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for Firebolt dialect support in sqlglot.""" + +from superset.sql.parse import SQLScript + + +def test_firebolt_exclude_syntax() -> None: + """Test that Firebolt EXCLUDE syntax is preserved (not transformed to EXCEPT).""" + sql = "SELECT g.* EXCLUDE (source_file_timestamp) FROM public.games g" + script = SQLScript(sql, "firebolt") + + generated = script.format() + assert "EXCLUDE" in generated Review Comment: **Suggestion:** The test only checks that `EXCLUDE` appears somewhere in the formatted SQL, so it can still pass if the formatter corrupts the star-exclude expression (for example, changing selected columns or moving the clause). Assert the full `g.* EXCLUDE (...)` fragment to validate the intended behavior. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ CI may miss malformed Firebolt star-exclude output. - ❌ SQL Lab formatter may send wrong Firebolt projection. ``` </details> ```suggestion assert "g.* EXCLUDE (source_file_timestamp)" in generated ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Trigger SQL formatting flow used in production via `SqlLabRestApi.format_sql()` at `superset/sqllab/api.py:196-205,264`, which calls `SQLScript(...).format()`. 2. Firebolt formatting uses dialect generator at `superset/sql/dialects/firebolt.py:56-61` (`STAR_EXCEPT = "EXCLUDE"`), the exact path this PR protects. 3. Current test `test_firebolt_exclude_syntax` at `tests/unit_tests/sql/test_firebolt_dialect.py:23-30` only checks `"EXCLUDE"` presence and `"EXCEPT"` absence. 4. If a future formatter regression keeps `"EXCLUDE"` but changes the star-exclude fragment, this test still passes, so CI misses a real Firebolt SQL rewrite bug. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/sql/test_firebolt_dialect.py **Line:** 29:29 **Comment:** *Logic Error: The test only checks that `EXCLUDE` appears somewhere in the formatted SQL, so it can still pass if the formatter corrupts the star-exclude expression (for example, changing selected columns or moving the clause). Assert the full `g.* EXCLUDE (...)` fragment to validate the intended behavior. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38742&comment_hash=25da69809598608835153f81f9a13683bfa2b3d14b77015151481629557104b2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38742&comment_hash=25da69809598608835153f81f9a13683bfa2b3d14b77015151481629557104b2&reaction=dislike'>👎</a> ########## tests/unit_tests/sql/test_firebolt_dialect.py: ########## @@ -0,0 +1,58 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for Firebolt dialect support in sqlglot.""" + +from superset.sql.parse import SQLScript + + +def test_firebolt_exclude_syntax() -> None: + """Test that Firebolt EXCLUDE syntax is preserved (not transformed to EXCEPT).""" + sql = "SELECT g.* EXCLUDE (source_file_timestamp) FROM public.games g" + script = SQLScript(sql, "firebolt") + + generated = script.format() + assert "EXCLUDE" in generated + assert "EXCEPT" not in generated + + +def test_firebolt_exclude_multiple_columns() -> None: + """Test EXCLUDE with multiple columns.""" + sql = "SELECT * EXCLUDE (col1, col2, col3) FROM my_table" + script = SQLScript(sql, "firebolt") + + generated = script.format() + assert "EXCLUDE" in generated Review Comment: **Suggestion:** This test claims to validate multiple excluded columns, but it only checks keyword presence and can pass even if one or more excluded columns are dropped or reordered incorrectly. Assert the full multi-column exclude fragment to catch real regressions. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Multi-column EXCLUDE regressions can bypass unit tests. - ❌ Firebolt queries may include unintended sensitive columns. ``` </details> ```suggestion assert "* EXCLUDE (col1, col2, col3)" in generated ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Use Firebolt SQL formatting path (`SQLScript.format`) reached by `superset/sqllab/api.py:264` and execution/render paths found in `superset/sql/execution/executor.py:236,356` and `superset/sqllab/query_render.py:85`. 2. Multi-column behavior is validated only by `test_firebolt_exclude_multiple_columns` at `tests/unit_tests/sql/test_firebolt_dialect.py:33-40`. 3. That test currently asserts only keyword presence (`"EXCLUDE"`) and absence (`"EXCEPT"`), not the full excluded column list. 4. A regression that drops/reorders excluded columns while still printing `"EXCLUDE"` would pass this test, allowing incorrect Firebolt SQL generation to ship. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/sql/test_firebolt_dialect.py **Line:** 39:39 **Comment:** *Logic Error: This test claims to validate multiple excluded columns, but it only checks keyword presence and can pass even if one or more excluded columns are dropped or reordered incorrectly. Assert the full multi-column exclude fragment to catch real regressions. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38742&comment_hash=0a7e1ffc15b5bb6d81239805b5b2b68e1cb64ec7e6371a5c3fc7c543caead382&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38742&comment_hash=0a7e1ffc15b5bb6d81239805b5b2b68e1cb64ec7e6371a5c3fc7c543caead382&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]
