[PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["amaannawab923 (via GitHub)" <gi...@apache.org>]

amaannawab923 opened a new pull request, #36136:
URL: https://github.com/apache/superset/pull/36136

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["alexandrusoare (via GitHub)" <gi...@apache.org>]

alexandrusoare commented on code in PR #36136:
URL: https://github.com/apache/superset/pull/36136#discussion_r2533754428


##########
superset/common/query_object.py:
##########
@@ -336,6 +336,13 @@ def _is_valid_date_range(self, date_range: str) -> bool:
 
     def _sanitize_filters(self) -> None:
         from superset.jinja_context import get_template_processor
+        from superset.utils.sql_sanitizer import (
+            is_ag_grid_viz_type,
+            sanitize_sql_with_sqlglot,
+        )
+
+        viz_type = self.extras.get("viz_type")
+        use_sqlglot_escaping = is_ag_grid_viz_type(viz_type)

Review Comment:
   I think sanitizing should be done regardless, is there a negative effect if we sanitize the queries regardless the viz_type?



##########
superset-frontend/packages/superset-ui-core/src/query/types/Query.ts:
##########
@@ -74,6 +74,9 @@ export type QueryObjectExtras = Partial<{
   instant_time_comparison_range?: string;
 
   time_compare?: string;
+
+  /** Visualization type for escape sanitisation */
+  viz_type?: string;
 }>;

Review Comment:
   I think we shouldn't check the viz type, we should sanitize the queries any way



##########
tests/unit_tests/utils/sql_sanitizer_escaping_test.py:
##########
@@ -0,0 +1,458 @@
+# 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.
+
+"""
+Test cases to verify SQLGlot properly escapes SQL strings for different databases.
+
+Focus: Ensure single quotes, backslashes, and special characters are escaped
+correctly for each database dialect.
+"""
+
+import pytest
+
+from superset.utils.sql_sanitizer import sanitize_sql_with_sqlglot
+
+
+class TestSQLEscapingByDatabase:
+    """Test SQL string escaping across different database backends."""
+
+    # Test data with problematic characters
+    TEST_CASES = [
+        # (input_sql, description)
+        ("name = 'O'Connor'", "Single quote in string"),
+        ("name = 'O''Connor'", "Pre-escaped single quote"),
+        ("path = 'C:\\Users\\test'", "Backslash in path"),
+        ("query = '100% complete'", "Percent sign"),
+        ("text = 'Hello_World'", "Underscore (LIKE wildcard)"),
+        ("data = 'test\"quote'", "Double quote in string"),
+        ("value = 'line1\\nline2'", "Newline escape sequence"),
+        ("pattern = '%test%'", "LIKE pattern with wildcards"),
+    ]
+
+    def test_postgres_single_quote_escaping(self):
+        """PostgreSQL should use SQL-92 standard: double single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"PostgreSQL - Input: {sql}")
+        print(f"PostgreSQL - Output: {result}")
+        assert "O''Connor" in result or "O'Connor" in result.replace("''", "'")
+        assert "name" in result
+
+    def test_mysql_single_quote_escaping(self):
+        """MySQL should handle single quotes (either '' or \\')."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "mysql")
+        print(f"MySQL - Input: {sql}")
+        print(f"MySQL - Output: {result}")
+        assert "O''Connor" in result or "O\\'Connor" in result or "O'Connor" in result
+        assert "name" in result
+
+    def test_databricks_single_quote_escaping(self):
+        """Databricks should use backslash escaping for single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "databricks")
+        print(f"Databricks - Input: {sql}")
+        print(f"Databricks - Output: {result}")
+        assert "name" in result
+        assert "Connor" in result
+
+    def test_sqlite_single_quote_escaping(self):
+        """SQLite uses SQL-92 standard: double single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "sqlite")
+        print(f"SQLite - Input: {sql}")
+        print(f"SQLite - Output: {result}")
+        assert "O''Connor" in result or "O'Connor" in result
+        assert "name" in result
+
+    def test_snowflake_single_quote_escaping(self):
+        """Snowflake uses backslash escaping for single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "snowflake")
+        print(f"Snowflake - Input: {sql}")
+        print(f"Snowflake - Output: {result}")
+        assert "O''Connor" in result or "O'Connor" in result or "O\\'Connor" in result
+        assert "name" in result
+
+    def test_bigquery_single_quote_escaping(self):
+        """BigQuery should handle single quotes properly."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "bigquery")
+        print(f"BigQuery - Input: {sql}")
+        print(f"BigQuery - Output: {result}")
+        assert "Connor" in result
+        assert "name" in result
+
+    def test_mssql_single_quote_escaping(self):
+        """SQL Server uses SQL-92 standard: double single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "mssql")
+        print(f"MSSQL - Input: {sql}")
+        print(f"MSSQL - Output: {result}")
+        assert "O''Connor" in result or "O'Connor" in result
+        assert "name" in result
+
+
+class TestBackslashEscaping:
+    """Test backslash handling across databases."""
+
+    def test_postgres_backslash_in_path(self):
+        """PostgreSQL: backslashes in strings."""
+        sql = "path = 'C:\\\\Users\\\\test'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"PostgreSQL backslash - Input: {sql}")
+        print(f"PostgreSQL backslash - Output: {result}")
+        # Should preserve or properly escape backslashes
+        assert "path" in result
+        assert "Users" in result or "C:" in result
+
+    def test_mysql_backslash_in_path(self):
+        """MySQL treats backslash as escape character."""
+        sql = "path = 'C:\\\\Users\\\\test'"
+        result = sanitize_sql_with_sqlglot(sql, "mysql")
+        print(f"MySQL backslash - Input: {sql}")
+        print(f"MySQL backslash - Output: {result}")
+        # MySQL needs double backslashes
+        assert "path" in result
+
+    def test_databricks_backslash_in_path(self):
+        """Databricks uses backslash as escape character."""
+        sql = "path = 'C:\\\\Users\\\\test'"
+        result = sanitize_sql_with_sqlglot(sql, "databricks")
+        print(f"Databricks backslash - Input: {sql}")
+        print(f"Databricks backslash - Output: {result}")
+        assert "path" in result
+
+
+class TestComplexSQLExpressions:
+    """Test complex SQL expressions with multiple special characters."""
+
+    def test_compound_filter_with_quotes(self):
+        """Test compound filters with quoted strings."""
+        sql = "(name = 'O''Connor' AND status = 'active')"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Compound filter - Input: {sql}")
+        print(f"Compound filter - Output: {result}")
+        assert "Connor" in result
+        assert "active" in result
+        assert "AND" in result
+
+    def test_ilike_with_special_chars(self):
+        """Test ILIKE patterns with special characters."""
+        sql = "name ILIKE '%O''Connor%'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"ILIKE pattern - Input: {sql}")
+        print(f"ILIKE pattern - Output: {result}")
+        assert "Connor" in result
+        assert "ILIKE" in result or "ilike" in result.lower()
+
+    def test_in_clause_with_quoted_strings(self):
+        """Test IN clause with multiple quoted strings."""
+        sql = "name IN ('O''Connor', 'D''Angelo', 'McDonald''s')"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"IN clause - Input: {sql}")
+        print(f"IN clause - Output: {result}")
+        assert "Connor" in result
+        assert "Angelo" in result
+        assert "McDonald" in result
+        assert "IN" in result
+
+    def test_mixed_operators_with_strings(self):
+        """Test mixed operators with string literals."""
+        sql = "(price > 100 AND name ILIKE '%test%' AND status = 'active')"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Mixed operators - Input: {sql}")
+        print(f"Mixed operators - Output: {result}")
+        assert "price" in result
+        assert "100" in result
+        assert "test" in result
+        assert "active" in result
+
+
+class TestSQLInjectionPrevention:
+    """Test that SQL injection attempts are handled safely."""
+
+    def test_injection_attempt_with_quotes(self):
+        """Test SQL injection attempt using quotes."""
+        sql = "name = 'test'; DROP TABLE users; --'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Injection attempt - Input: {sql}")
+        print(f"Injection attempt - Output: {result}")
+        assert "name" in result
+        assert "test" in result
+        assert "DROP" not in result
+
+    def test_injection_with_escaped_quotes(self):
+        """Test injection attempt with pre-escaped quotes."""
+        sql = "name = 'test''; DELETE FROM users; --'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Escaped injection - Input: {sql}")
+        print(f"Escaped injection - Output: {result}")
+        assert isinstance(result, str)
+
+    def test_legitimate_quote_in_string(self):
+        """Test that legitimate quotes are preserved correctly."""
+        sql = "description = 'It''s a test'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Legitimate quote - Input: {sql}")
+        print(f"Legitimate quote - Output: {result}")
+        assert "test" in result
+        assert "It" in result
+
+
+class TestDangerousConstructsBlocked:
+    """Test that dangerous SQL constructs are blocked with ValueError."""
+
+    def test_subquery_in_in_clause_blocked(self):
+        """Subquery in IN clause should raise ValueError."""
+        sql = "id IN (SELECT id FROM users)"
+        with pytest.raises(ValueError, match="Subqueries are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_subquery_in_where_blocked(self):
+        """Subquery in comparison should raise ValueError."""
+        sql = "price > (SELECT AVG(price) FROM products)"
+        with pytest.raises(ValueError, match="Subqueries are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_exists_subquery_blocked(self):
+        """EXISTS with subquery should raise ValueError."""
+        sql = "id = 1 AND EXISTS (SELECT 1 FROM users WHERE users.id = orders.user_id)"
+        with pytest.raises(ValueError, match="Subqueries are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_union_injection_blocked(self):
+        """UNION set operation should raise ValueError."""
+        sql = "1=1 UNION SELECT * FROM users"
+        with pytest.raises(ValueError, match="Set operations are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_union_all_blocked(self):
+        """UNION ALL set operation should raise ValueError."""
+        sql = "status='active' UNION ALL SELECT password FROM users"
+        with pytest.raises(ValueError, match="Set operations are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_intersect_blocked(self):
+        """INTERSECT set operation should raise ValueError."""
+        sql = "id=1 INTERSECT SELECT id FROM admins"
+        with pytest.raises(ValueError, match="Set operations are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_except_blocked(self):
+        """EXCEPT set operation should raise ValueError."""
+        sql = "id=1 EXCEPT SELECT id FROM banned"
+        with pytest.raises(ValueError, match="Set operations are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_ddl_commands_stripped(self):
+        """DDL commands after semicolon are stripped by SQLGlot parse_one."""
+        sql = "1=1; DROP TABLE users"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "DROP" not in result
+        assert "1 = 1" in result
+
+    def test_delete_command_stripped(self):
+        """DELETE command after semicolon is stripped."""
+        sql = "1=1; DELETE FROM users"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "DELETE" not in result
+        assert "1 = 1" in result
+
+    def test_insert_command_stripped(self):
+        """INSERT command after semicolon is stripped."""
+        sql = "1=1; INSERT INTO users VALUES (1, 'admin')"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "INSERT" not in result
+        assert "1 = 1" in result
+
+    def test_update_command_stripped(self):
+        """UPDATE command after semicolon is stripped."""
+        sql = "1=1; UPDATE users SET admin=true"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "UPDATE" not in result
+        assert "1 = 1" in result
+
+    def test_create_table_command_stripped(self):
+        """CREATE TABLE command after semicolon is stripped."""
+        sql = "1=1; CREATE TABLE hacked (id INT)"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "CREATE" not in result
+        assert "1 = 1" in result
+
+    def test_validation_can_be_disabled(self):
+        """When validate_structure=False, subqueries are allowed."""
+        sql = "id IN (SELECT id FROM users)"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql", validate_structure=False)
+        assert "SELECT" in result
+        assert "users" in result
+
+
+class TestDatabaseSpecificBehavior:
+    """Test database-specific SQL behaviors."""
+
+    def test_ilike_postgres_vs_mysql(self):
+        """ILIKE is PostgreSQL-specific, MySQL uses LIKE."""
+        sql = "name ILIKE '%test%'"
+
+        pg_result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"PostgreSQL ILIKE: {pg_result}")
+        # PostgreSQL should keep ILIKE
+        assert "ILIKE" in pg_result or "ilike" in pg_result.lower()
+
+        mysql_result = sanitize_sql_with_sqlglot(sql, "mysql")
+        print(f"MySQL ILIKE: {mysql_result}")
+        # MySQL might convert to LIKE or keep ILIKE depending on SQLGlot version
+        assert "LIKE" in mysql_result or "like" in mysql_result.lower()
+
+    def test_boolean_literals_across_dbs(self):
+        """Test boolean literals across databases."""
+        sql = "active = TRUE AND deleted = FALSE"
+
+        pg_result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"PostgreSQL boolean: {pg_result}")
+        assert "active" in pg_result
+        assert "deleted" in pg_result
+
+        mysql_result = sanitize_sql_with_sqlglot(sql, "mysql")
+        print(f"MySQL boolean: {mysql_result}")
+        assert "active" in mysql_result
+        assert "deleted" in mysql_result
+
+    def test_identifier_quoting(self):
+        """Test that column names with special chars are handled."""
+        sql = "\"column-name\" = 'value'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Identifier quoting - Input: {sql}")
+        print(f"Identifier quoting - Output: {result}")
+        assert "value" in result
+
+
+class TestRealWorldAGGridScenarios:
+    """Test real-world scenarios from AG Grid filters."""

Review Comment:
   Maybe we shouldn t focus on plugin specific tests, maybe rephrasing it would help, also I am not sure if the print statements are necessary in the tests



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["betodealmeida (via GitHub)" <gi...@apache.org>]

betodealmeida commented on code in PR #36136:
URL: https://github.com/apache/superset/pull/36136#discussion_r2557849698


##########
tests/unit_tests/sql/transpile_to_dialect_test.py:
##########
@@ -0,0 +1,347 @@
+# 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 transpile_to_dialect function in superset/sql/parse.py
+"""
+
+import pytest
+
+from superset.sql.parse import transpile_to_dialect
+
+
+@pytest.mark.parametrize(
+    "sql,dialect,expected",
+    [
+        # PostgreSQL - SQL-92 standard double single quotes
+        ("name = 'O''Hara'", "postgresql", "name = 'O''Hara'"),
+        # MySQL - SQL-92 standard double single quotes
+        ("name = 'O''Hara'", "mysql", "name = 'O''Hara'"),
+        # SQLite - SQL-92 standard double single quotes
+        ("name = 'O''Hara'", "sqlite", "name = 'O''Hara'"),
+        # Snowflake - backslash escaping
+        ("name = 'O''Hara'", "snowflake", "name = 'O\\'Hara'"),

Review Comment:
   Small nit, you can use `r"` strings to make this easier to read:
   ```suggestion
           ("name = 'O''Hara'", "snowflake", r"name = 'O\'Hara'"),
   ```



##########
tests/unit_tests/sql/transpile_to_dialect_test.py:
##########
@@ -0,0 +1,347 @@
+# 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 transpile_to_dialect function in superset/sql/parse.py
+"""
+
+import pytest
+
+from superset.sql.parse import transpile_to_dialect
+
+
+@pytest.mark.parametrize(
+    "sql,dialect,expected",
+    [
+        # PostgreSQL - SQL-92 standard double single quotes
+        ("name = 'O''Hara'", "postgresql", "name = 'O''Hara'"),
+        # MySQL - SQL-92 standard double single quotes
+        ("name = 'O''Hara'", "mysql", "name = 'O''Hara'"),
+        # SQLite - SQL-92 standard double single quotes
+        ("name = 'O''Hara'", "sqlite", "name = 'O''Hara'"),
+        # Snowflake - backslash escaping
+        ("name = 'O''Hara'", "snowflake", "name = 'O\\'Hara'"),

Review Comment:
   Small nit, you can use `r"` strings here and in other places to make it easier to read:
   ```suggestion
           ("name = 'O''Hara'", "snowflake", r"name = 'O\'Hara'"),
   ```



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["amaannawab923 (via GitHub)" <gi...@apache.org>]

amaannawab923 commented on code in PR #36136:
URL: https://github.com/apache/superset/pull/36136#discussion_r2533765399


##########
superset-frontend/packages/superset-ui-core/src/query/types/Query.ts:
##########
@@ -74,6 +74,9 @@ export type QueryObjectExtras = Partial<{
   instant_time_comparison_range?: string;
 
   time_compare?: string;
+
+  /** Visualization type for escape sanitisation */
+  viz_type?: string;
 }>;

Review Comment:
   Yes true , but just wanted to reduce the blast radius , if any kind of issue arises from this functionality , it wont propogate to other charts



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["betodealmeida (via GitHub)" <gi...@apache.org>]

betodealmeida commented on code in PR #36136:
URL: https://github.com/apache/superset/pull/36136#discussion_r2550694488


##########
tests/unit_tests/sql/transpile_to_dialect_test.py:
##########
@@ -0,0 +1,271 @@
+# 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 transpile_to_dialect function in superset/sql/parse.py
+"""
+
+import pytest
+
+from superset.sql.parse import transpile_to_dialect
+
+
+def test_postgres_single_quote_escaping():

Review Comment:
   Let's simplify these tests by (1) using parameters and (2) checking the whole value of `result`. Something like:
   
   ```python
   @pytest.mark.parametrize(
       "sql, dialect, expected",
       [
           ("name = 'O''Hara'", "postgresql", "name = 'O''Hara'"),
           ("name = 'O''Hara'", "databricks", "name = 'O\'Hara'"),
           ...
       ],
   )
   def test_single_quote_escaping(sql: str, dialect: str, expected: str) -> None:
       assert transpile_to_dialect(sql, dialect) == expected
   ```



##########
tests/unit_tests/sql/transpile_to_dialect_test.py:
##########
@@ -0,0 +1,271 @@
+# 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 transpile_to_dialect function in superset/sql/parse.py
+"""
+
+import pytest
+
+from superset.sql.parse import transpile_to_dialect
+
+
+def test_postgres_single_quote_escaping():
+    """PostgreSQL keeps SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_databricks_single_quote_escaping():
+    """Databricks uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "databricks")
+    assert "name" in result
+    # Databricks uses backslash escaping
+    assert "O\\'Hara" in result or "O''Hara" in result
+
+
+def test_mysql_single_quote_escaping():
+    """MySQL uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "mysql")
+    assert "name" in result
+    # MySQL may use backslash or double-quote escaping
+    assert (
+        "Connor" in result.replace("O''Hara", "Connor").replace("O\\'Hara", "Connor")
+        or "Hara" in result
+    )
+
+
+def test_sqlite_single_quote_escaping():
+    """SQLite uses SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "sqlite")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_snowflake_single_quote_escaping():
+    """Snowflake handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "snowflake")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_bigquery_single_quote_escaping():
+    """BigQuery handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "bigquery")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_compound_filter_with_quotes():
+    """Test compound filters with quoted strings."""
+    sql = "(name = 'O''Hara' AND status = 'active')"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "active" in result
+    assert "AND" in result
+
+
+def test_like_with_special_chars():
+    """Test LIKE patterns with special characters."""
+    sql = "name LIKE '%O''Hara%'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "LIKE" in result
+
+
+def test_ilike_case_insensitive():
+    """Test ILIKE for case-insensitive matching (PostgreSQL syntax)."""
+    sql = "name ILIKE '%O''Hara%'"
+
+    # PostgreSQL keeps ILIKE and '' escaping
+    pg_result = transpile_to_dialect(sql, "postgresql")
+    assert "ILIKE" in pg_result
+    assert "Hara" in pg_result
+
+    # MySQL converts ILIKE to LIKE (MySQL LIKE is case-insensitive by default)
+    mysql_result = transpile_to_dialect(sql, "mysql")
+    assert "LIKE" in mysql_result
+    assert "Hara" in mysql_result
+
+    # Databricks supports ILIKE
+    db_result = transpile_to_dialect(sql, "databricks")
+    assert "ILIKE" in db_result or "LIKE" in db_result
+    assert "Hara" in db_result

Review Comment:
   Same thing here, let's use `@pytest.mark.parametrize` to break these down into individual tests.



##########
tests/unit_tests/sql/transpile_to_dialect_test.py:
##########
@@ -0,0 +1,271 @@
+# 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 transpile_to_dialect function in superset/sql/parse.py
+"""
+
+import pytest
+
+from superset.sql.parse import transpile_to_dialect
+
+
+def test_postgres_single_quote_escaping():
+    """PostgreSQL keeps SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_databricks_single_quote_escaping():
+    """Databricks uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "databricks")
+    assert "name" in result
+    # Databricks uses backslash escaping
+    assert "O\\'Hara" in result or "O''Hara" in result
+
+
+def test_mysql_single_quote_escaping():
+    """MySQL uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "mysql")
+    assert "name" in result
+    # MySQL may use backslash or double-quote escaping
+    assert (
+        "Connor" in result.replace("O''Hara", "Connor").replace("O\\'Hara", "Connor")
+        or "Hara" in result
+    )

Review Comment:
   Even though MySQL accepts both when parsing, the generated SQL will always use single quotes and not backslash.



##########
tests/unit_tests/sql/transpile_to_dialect_test.py:
##########
@@ -0,0 +1,271 @@
+# 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 transpile_to_dialect function in superset/sql/parse.py
+"""
+
+import pytest
+
+from superset.sql.parse import transpile_to_dialect
+
+
+def test_postgres_single_quote_escaping():
+    """PostgreSQL keeps SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_databricks_single_quote_escaping():
+    """Databricks uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "databricks")
+    assert "name" in result
+    # Databricks uses backslash escaping
+    assert "O\\'Hara" in result or "O''Hara" in result
+
+
+def test_mysql_single_quote_escaping():
+    """MySQL uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "mysql")
+    assert "name" in result
+    # MySQL may use backslash or double-quote escaping
+    assert (
+        "Connor" in result.replace("O''Hara", "Connor").replace("O\\'Hara", "Connor")
+        or "Hara" in result
+    )
+
+
+def test_sqlite_single_quote_escaping():
+    """SQLite uses SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "sqlite")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_snowflake_single_quote_escaping():
+    """Snowflake handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "snowflake")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_bigquery_single_quote_escaping():
+    """BigQuery handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "bigquery")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_compound_filter_with_quotes():
+    """Test compound filters with quoted strings."""
+    sql = "(name = 'O''Hara' AND status = 'active')"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "active" in result
+    assert "AND" in result

Review Comment:
   It's better to test for the behavior we want... if `result` was `Hara active AND` this test would pass, even though the result is wrong. So let's do something like this here and in all tests in this file:
   
   ```suggestion
       assert transpile_to_dialect(sql, "postgresql") == "(name = 'O''Hara' AND status = 'active')"
   ```
   
   That is, check the actual generated string, not just if substrings are present.



##########
superset/sql/parse.py:
##########
@@ -1503,3 +1503,31 @@ def sanitize_clause(clause: str, engine: str) -> str:
         )
     except SupersetParseError as ex:
         raise QueryClauseValidationException(f"Invalid SQL clause: {clause}") from ex
+
+
+def transpile_to_dialect(sql: str, target_engine: str) -> str:
+    """
+    Transpile SQL to the target database dialect using SQLGlot.

Review Comment:
   ```suggestion
       Transpile SQL from "generic SQL" to the target database dialect using SQLGlot.
   ```



##########
tests/unit_tests/sql/transpile_to_dialect_test.py:
##########
@@ -0,0 +1,271 @@
+# 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 transpile_to_dialect function in superset/sql/parse.py
+"""
+
+import pytest
+
+from superset.sql.parse import transpile_to_dialect
+
+
+def test_postgres_single_quote_escaping():
+    """PostgreSQL keeps SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_databricks_single_quote_escaping():
+    """Databricks uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "databricks")
+    assert "name" in result
+    # Databricks uses backslash escaping
+    assert "O\\'Hara" in result or "O''Hara" in result
+
+
+def test_mysql_single_quote_escaping():
+    """MySQL uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "mysql")
+    assert "name" in result
+    # MySQL may use backslash or double-quote escaping
+    assert (
+        "Connor" in result.replace("O''Hara", "Connor").replace("O\\'Hara", "Connor")
+        or "Hara" in result
+    )
+
+
+def test_sqlite_single_quote_escaping():
+    """SQLite uses SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "sqlite")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_snowflake_single_quote_escaping():
+    """Snowflake handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "snowflake")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_bigquery_single_quote_escaping():
+    """BigQuery handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "bigquery")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_compound_filter_with_quotes():
+    """Test compound filters with quoted strings."""
+    sql = "(name = 'O''Hara' AND status = 'active')"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "active" in result
+    assert "AND" in result
+
+
+def test_like_with_special_chars():
+    """Test LIKE patterns with special characters."""
+    sql = "name LIKE '%O''Hara%'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "LIKE" in result
+
+
+def test_ilike_case_insensitive():
+    """Test ILIKE for case-insensitive matching (PostgreSQL syntax)."""
+    sql = "name ILIKE '%O''Hara%'"
+
+    # PostgreSQL keeps ILIKE and '' escaping
+    pg_result = transpile_to_dialect(sql, "postgresql")
+    assert "ILIKE" in pg_result
+    assert "Hara" in pg_result
+
+    # MySQL converts ILIKE to LIKE (MySQL LIKE is case-insensitive by default)
+    mysql_result = transpile_to_dialect(sql, "mysql")
+    assert "LIKE" in mysql_result
+    assert "Hara" in mysql_result
+
+    # Databricks supports ILIKE
+    db_result = transpile_to_dialect(sql, "databricks")
+    assert "ILIKE" in db_result or "LIKE" in db_result
+    assert "Hara" in db_result
+
+
+def test_ilike_transpilation_across_databases():
+    """Test ILIKE transpilation to various database dialects."""
+    sql = "name ILIKE '%test%'"
+
+    # PostgreSQL: keeps ILIKE
+    pg_result = transpile_to_dialect(sql, "postgresql")
+    assert "ILIKE" in pg_result
+
+    # MySQL: ILIKE → LIKE
+    mysql_result = transpile_to_dialect(sql, "mysql")
+    assert "LIKE" in mysql_result
+
+
+def test_in_clause_with_quoted_strings():
+    """Test IN clause with multiple quoted strings."""
+    sql = "name IN ('O''Hara', 'D''Angelo', 'McDonald''s')"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "Angelo" in result
+    assert "McDonald" in result
+    assert "IN" in result
+
+
+def test_mixed_operators_with_strings():
+    """Test mixed operators with string literals."""
+    sql = "(price > 100 AND name LIKE '%test%' AND status = 'active')"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "price" in result
+    assert "100" in result
+    assert "test" in result
+    assert "active" in result
+
+
+def test_number_comparison():
+    """Test number comparisons are preserved."""
+    sql = "price > 100 AND quantity <= 50"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "price > 100" in result
+    assert "quantity <= 50" in result
+
+
+def test_date_comparison():
+    """Test date comparisons."""
+    sql = "created_at > '2024-01-01'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "created_at" in result
+    assert "2024-01-01" in result
+
+
+def test_between_clause():
+    """Test BETWEEN clause."""
+    sql = "price BETWEEN 10 AND 100"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "price" in result
+    assert "BETWEEN" in result
+    assert "10" in result
+    assert "100" in result
+
+
+def test_is_null():
+    """Test IS NULL clause."""
+    sql = "name IS NULL"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "name IS NULL" in result
+
+
+def test_is_not_null():
+    """Test IS NOT NULL clause (SQLGlot may normalize to NOT ... IS NULL)."""
+    sql = "name IS NOT NULL"
+    result = transpile_to_dialect(sql, "postgresql")
+    # SQLGlot normalizes "IS NOT NULL" to "NOT ... IS NULL"
+    assert "name" in result
+    assert "NOT" in result
+    assert "NULL" in result
+
+
+def test_or_condition():
+    """Test OR condition."""
+    sql = "status = 'active' OR status = 'pending'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "active" in result
+    assert "pending" in result
+    assert "OR" in result
+
+
+def test_nested_conditions():
+    """Test nested parentheses conditions."""
+    sql = "((a > 1 AND b < 2) OR (c = 3))"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "a > 1" in result
+    assert "b < 2" in result
+    assert "c = 3" in result
+
+
+def test_escaping_consistency_across_databases():
+    """Verify transpilation works for each database type."""
+    test_sql = "name = 'O''Hara'"
+
+    databases = [
+        "postgresql",
+        "mysql",
+        "sqlite",
+        "snowflake",
+        "bigquery",
+        "mssql",
+        "databricks",
+        "presto",
+        "trino",
+    ]
+
+    for db in databases:

Review Comment:
   Another good use case for `@pytest.mark.parametrize`.



##########
superset/sql/parse.py:
##########
@@ -1503,3 +1503,31 @@ def sanitize_clause(clause: str, engine: str) -> str:
         )
     except SupersetParseError as ex:
         raise QueryClauseValidationException(f"Invalid SQL clause: {clause}") from ex
+
+
+def transpile_to_dialect(sql: str, target_engine: str) -> str:
+    """
+    Transpile SQL to the target database dialect using SQLGlot.
+
+    If the target engine is not in SQLGLOT_DIALECTS, returns the SQL as-is.
+    """
+    target_dialect = SQLGLOT_DIALECTS.get(target_engine)
+
+    # If no dialect mapping exists, return as-is
+    if target_dialect is None:
+        return sql
+
+    try:
+        parsed = sqlglot.parse_one(sql, dialect="postgres")

Review Comment:
   Let's parse using the generic dialect here, since the use of "postgres" could be confusing:
   
   ```suggestion
           # use the generic parser
           parsed = sqlglot.parse_one(sql, sqlglot.dialects.dialect.Dialect)
   ```



##########
tests/unit_tests/sql/transpile_to_dialect_test.py:
##########
@@ -0,0 +1,271 @@
+# 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 transpile_to_dialect function in superset/sql/parse.py
+"""
+
+import pytest
+
+from superset.sql.parse import transpile_to_dialect
+
+
+def test_postgres_single_quote_escaping():
+    """PostgreSQL keeps SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_databricks_single_quote_escaping():
+    """Databricks uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "databricks")
+    assert "name" in result
+    # Databricks uses backslash escaping
+    assert "O\\'Hara" in result or "O''Hara" in result
+
+
+def test_mysql_single_quote_escaping():
+    """MySQL uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "mysql")
+    assert "name" in result
+    # MySQL may use backslash or double-quote escaping
+    assert (
+        "Connor" in result.replace("O''Hara", "Connor").replace("O\\'Hara", "Connor")
+        or "Hara" in result
+    )
+
+
+def test_sqlite_single_quote_escaping():
+    """SQLite uses SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "sqlite")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_snowflake_single_quote_escaping():
+    """Snowflake handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "snowflake")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_bigquery_single_quote_escaping():
+    """BigQuery handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "bigquery")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_compound_filter_with_quotes():
+    """Test compound filters with quoted strings."""
+    sql = "(name = 'O''Hara' AND status = 'active')"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "active" in result
+    assert "AND" in result
+
+
+def test_like_with_special_chars():
+    """Test LIKE patterns with special characters."""
+    sql = "name LIKE '%O''Hara%'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "LIKE" in result
+
+
+def test_ilike_case_insensitive():
+    """Test ILIKE for case-insensitive matching (PostgreSQL syntax)."""
+    sql = "name ILIKE '%O''Hara%'"
+
+    # PostgreSQL keeps ILIKE and '' escaping
+    pg_result = transpile_to_dialect(sql, "postgresql")
+    assert "ILIKE" in pg_result
+    assert "Hara" in pg_result
+
+    # MySQL converts ILIKE to LIKE (MySQL LIKE is case-insensitive by default)
+    mysql_result = transpile_to_dialect(sql, "mysql")
+    assert "LIKE" in mysql_result
+    assert "Hara" in mysql_result
+
+    # Databricks supports ILIKE
+    db_result = transpile_to_dialect(sql, "databricks")
+    assert "ILIKE" in db_result or "LIKE" in db_result
+    assert "Hara" in db_result
+
+
+def test_ilike_transpilation_across_databases():

Review Comment:
   This one too.



##########
tests/unit_tests/sql/transpile_to_dialect_test.py:
##########
@@ -0,0 +1,271 @@
+# 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 transpile_to_dialect function in superset/sql/parse.py
+"""
+
+import pytest
+
+from superset.sql.parse import transpile_to_dialect
+
+
+def test_postgres_single_quote_escaping():
+    """PostgreSQL keeps SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_databricks_single_quote_escaping():
+    """Databricks uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "databricks")
+    assert "name" in result
+    # Databricks uses backslash escaping
+    assert "O\\'Hara" in result or "O''Hara" in result
+
+
+def test_mysql_single_quote_escaping():
+    """MySQL uses backslash escaping for single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "mysql")
+    assert "name" in result
+    # MySQL may use backslash or double-quote escaping
+    assert (
+        "Connor" in result.replace("O''Hara", "Connor").replace("O\\'Hara", "Connor")
+        or "Hara" in result
+    )
+
+
+def test_sqlite_single_quote_escaping():
+    """SQLite uses SQL-92 standard: double single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "sqlite")
+    assert "O''Hara" in result
+    assert "name" in result
+
+
+def test_snowflake_single_quote_escaping():
+    """Snowflake handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "snowflake")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_bigquery_single_quote_escaping():
+    """BigQuery handles single quotes."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "bigquery")
+    assert "name" in result
+    assert "Hara" in result
+
+
+def test_compound_filter_with_quotes():
+    """Test compound filters with quoted strings."""
+    sql = "(name = 'O''Hara' AND status = 'active')"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "active" in result
+    assert "AND" in result
+
+
+def test_like_with_special_chars():
+    """Test LIKE patterns with special characters."""
+    sql = "name LIKE '%O''Hara%'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "LIKE" in result
+
+
+def test_ilike_case_insensitive():
+    """Test ILIKE for case-insensitive matching (PostgreSQL syntax)."""
+    sql = "name ILIKE '%O''Hara%'"
+
+    # PostgreSQL keeps ILIKE and '' escaping
+    pg_result = transpile_to_dialect(sql, "postgresql")
+    assert "ILIKE" in pg_result
+    assert "Hara" in pg_result
+
+    # MySQL converts ILIKE to LIKE (MySQL LIKE is case-insensitive by default)
+    mysql_result = transpile_to_dialect(sql, "mysql")
+    assert "LIKE" in mysql_result
+    assert "Hara" in mysql_result
+
+    # Databricks supports ILIKE
+    db_result = transpile_to_dialect(sql, "databricks")
+    assert "ILIKE" in db_result or "LIKE" in db_result
+    assert "Hara" in db_result
+
+
+def test_ilike_transpilation_across_databases():
+    """Test ILIKE transpilation to various database dialects."""
+    sql = "name ILIKE '%test%'"
+
+    # PostgreSQL: keeps ILIKE
+    pg_result = transpile_to_dialect(sql, "postgresql")
+    assert "ILIKE" in pg_result
+
+    # MySQL: ILIKE → LIKE
+    mysql_result = transpile_to_dialect(sql, "mysql")
+    assert "LIKE" in mysql_result
+
+
+def test_in_clause_with_quoted_strings():
+    """Test IN clause with multiple quoted strings."""
+    sql = "name IN ('O''Hara', 'D''Angelo', 'McDonald''s')"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "Hara" in result
+    assert "Angelo" in result
+    assert "McDonald" in result
+    assert "IN" in result
+
+
+def test_mixed_operators_with_strings():
+    """Test mixed operators with string literals."""
+    sql = "(price > 100 AND name LIKE '%test%' AND status = 'active')"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "price" in result
+    assert "100" in result
+    assert "test" in result
+    assert "active" in result
+
+
+def test_number_comparison():
+    """Test number comparisons are preserved."""
+    sql = "price > 100 AND quantity <= 50"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "price > 100" in result
+    assert "quantity <= 50" in result
+
+
+def test_date_comparison():
+    """Test date comparisons."""
+    sql = "created_at > '2024-01-01'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "created_at" in result
+    assert "2024-01-01" in result
+
+
+def test_between_clause():
+    """Test BETWEEN clause."""
+    sql = "price BETWEEN 10 AND 100"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "price" in result
+    assert "BETWEEN" in result
+    assert "10" in result
+    assert "100" in result
+
+
+def test_is_null():
+    """Test IS NULL clause."""
+    sql = "name IS NULL"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "name IS NULL" in result
+
+
+def test_is_not_null():
+    """Test IS NOT NULL clause (SQLGlot may normalize to NOT ... IS NULL)."""
+    sql = "name IS NOT NULL"
+    result = transpile_to_dialect(sql, "postgresql")
+    # SQLGlot normalizes "IS NOT NULL" to "NOT ... IS NULL"
+    assert "name" in result
+    assert "NOT" in result
+    assert "NULL" in result
+
+
+def test_or_condition():
+    """Test OR condition."""
+    sql = "status = 'active' OR status = 'pending'"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "active" in result
+    assert "pending" in result
+    assert "OR" in result
+
+
+def test_nested_conditions():
+    """Test nested parentheses conditions."""
+    sql = "((a > 1 AND b < 2) OR (c = 3))"
+    result = transpile_to_dialect(sql, "postgresql")
+    assert "a > 1" in result
+    assert "b < 2" in result
+    assert "c = 3" in result
+
+
+def test_escaping_consistency_across_databases():
+    """Verify transpilation works for each database type."""
+    test_sql = "name = 'O''Hara'"
+
+    databases = [
+        "postgresql",
+        "mysql",
+        "sqlite",
+        "snowflake",
+        "bigquery",
+        "mssql",
+        "databricks",
+        "presto",
+        "trino",
+    ]
+
+    for db in databases:
+        result = transpile_to_dialect(test_sql, db)
+        assert "name" in result, f"{db}: Missing column name"
+        assert "Hara" in result, f"{db}: Missing value"
+
+
+def test_invalid_sql_raises_exception():
+    """Test that invalid SQL raises QueryClauseValidationException."""
+    from superset.exceptions import QueryClauseValidationException
+
+    with pytest.raises(QueryClauseValidationException):
+        transpile_to_dialect("INVALID SQL !!!", "postgresql")
+
+
+def test_empty_sql():
+    """Test that empty SQL raises exception."""
+    from superset.exceptions import QueryClauseValidationException
+
+    with pytest.raises(QueryClauseValidationException):
+        transpile_to_dialect("", "postgresql")
+
+
+def test_unknown_engine_returns_sql_as_is():
+    """Test that unknown engine returns SQL unchanged (SQL-92 compatible)."""
+    sql = "name = 'O''Hara'"
+    result = transpile_to_dialect(sql, "unknown_database_engine")
+    assert result == sql
+
+
+def test_engine_not_in_dialect_map():
+    """Test engines not in SQLGLOT_DIALECTS return SQL as-is."""
+    sql = "status == 'active' AND count > 10"
+    # These engines are commented out in SQLGLOT_DIALECTS
+    unknown_engines = [
+        "crate",
+        "databend",
+        "db2",
+        "denodo",
+        "dynamodb",
+        "elasticsearch",
+    ]
+
+    for engine in unknown_engines:

Review Comment:
   Combine this test with `test_escaping_consistency_across_databases`.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["geido (via GitHub)" <gi...@apache.org>]

geido merged PR #36136:
URL: https://github.com/apache/superset/pull/36136


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["codecov[bot] (via GitHub)" <gi...@apache.org>]

codecov[bot] commented on PR #36136:
URL: https://github.com/apache/superset/pull/36136#issuecomment-3541237460

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/36136?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `31.11111%` with `31 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 48.11%. Comparing base ([`76d897e`](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c937359`](https://app.codecov.io/gh/apache/superset/commit/c937359593f290d695eb2e1c12526ea5ad35de24?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 2899 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/36136?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [superset/utils/sql\_sanitizer.py](https://app.codecov.io/gh/apache/superset/pull/36136?src=pr&el=tree&filepath=superset%2Futils%2Fsql_sanitizer.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvc3FsX3Nhbml0aXplci5weQ==) | 28.20% | [28 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/36136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [superset/common/query\_object.py](https://app.codecov.io/gh/apache/superset/pull/36136?src=pr&el=tree&filepath=superset%2Fcommon%2Fquery_object.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | 50.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/36136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #36136       +/-   ##
   ===========================================
   - Coverage   60.48%   48.11%   -12.38%     
   ===========================================
     Files        1931      630     -1301     
     Lines       76236    46122    -30114     
     Branches     8568     5002     -3566     
   ===========================================
   - Hits        46114    22193    -23921     
   + Misses      28017    23299     -4718     
   + Partials     2105      630     -1475     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/36136/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/36136/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.77% <0.00%> (-5.38%)` | :arrow_down: |
   | [javascript](https://app.codecov.io/gh/apache/superset/pull/36136/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/36136/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.43% <31.11%> (-6.38%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/36136/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.11% <31.11%> (-15.40%)` | :arrow_down: |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/36136/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.36%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/36136?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["betodealmeida (via GitHub)" <gi...@apache.org>]

betodealmeida commented on code in PR #36136:
URL: https://github.com/apache/superset/pull/36136#discussion_r2539631633


##########
superset/utils/sql_sanitizer.py:
##########
@@ -0,0 +1,95 @@
+# 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.
+
+from __future__ import annotations
+
+from typing import Any
+
+import sqlglot
+from sqlglot import Dialects, exp
+from sqlglot.errors import ParseError
+
+from superset.sql.parse import SQLGLOT_DIALECTS
+
+
+def get_sqlglot_dialect(database_backend: str) -> Any:
+    """Map Superset database backend name to SQLGlot dialect."""
+    return SQLGLOT_DIALECTS.get(database_backend, Dialects.DIALECT)
+
+
+def sanitize_sql_with_sqlglot(
+    raw_sql: str,
+    database_backend: str,
+    validate_structure: bool = True,
+) -> str:
+    """
+    Sanitize a raw SQL WHERE/HAVING clause using SQLGlot.
+
+    Parses the SQL into an AST and regenerates it with proper
+    escaping for the target database dialect.
+
+    Raises ValueError if SQL contains disallowed constructs
+    (subqueries, DDL/DML commands, set operations).
+    """
+    if not raw_sql or not raw_sql.strip():
+        return ""
+
+    dialect = get_sqlglot_dialect(database_backend)
+
+    try:
+        parsed = sqlglot.parse_one(raw_sql, dialect=dialect)
+
+        if validate_structure:
+            _validate_sql_structure(parsed)
+
+        return parsed.sql(dialect=dialect)
+
+    except ParseError:
+        return raw_sql
+    except ValueError:
+        raise
+    except Exception:  # pylint: disable=broad-except
+        return raw_sql
+
+
+def _validate_sql_structure(parsed: exp.Expression) -> None:

Review Comment:
   We already have logic in `superset/sql/parse.py` to check for this.



##########
superset/utils/sql_sanitizer.py:
##########
@@ -0,0 +1,95 @@
+# 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.
+
+from __future__ import annotations
+
+from typing import Any
+
+import sqlglot
+from sqlglot import Dialects, exp
+from sqlglot.errors import ParseError

Review Comment:
   One of the goals of moving to `sqlglot` was consolidating all SQL parsing under a couple classes (`SQLScript` and `SQLStatements`), so that if we ever want to migrate to a different parsing library we would only have to replace those two classes. Because of that I added lint rules that prevent `sqlglot` from being imported outside of `superset/sql/`, I'm not sure why the rule is no longer triggering.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["amaannawab923 (via GitHub)" <gi...@apache.org>]

amaannawab923 closed pull request #36136: feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses
URL: https://github.com/apache/superset/pull/36136


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["amaannawab923 (via GitHub)" <gi...@apache.org>]

amaannawab923 commented on code in PR #36136:
URL: https://github.com/apache/superset/pull/36136#discussion_r2533768904


##########
tests/unit_tests/utils/sql_sanitizer_escaping_test.py:
##########
@@ -0,0 +1,458 @@
+# 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.
+
+"""
+Test cases to verify SQLGlot properly escapes SQL strings for different databases.
+
+Focus: Ensure single quotes, backslashes, and special characters are escaped
+correctly for each database dialect.
+"""
+
+import pytest
+
+from superset.utils.sql_sanitizer import sanitize_sql_with_sqlglot
+
+
+class TestSQLEscapingByDatabase:
+    """Test SQL string escaping across different database backends."""
+
+    # Test data with problematic characters
+    TEST_CASES = [
+        # (input_sql, description)
+        ("name = 'O'Connor'", "Single quote in string"),
+        ("name = 'O''Connor'", "Pre-escaped single quote"),
+        ("path = 'C:\\Users\\test'", "Backslash in path"),
+        ("query = '100% complete'", "Percent sign"),
+        ("text = 'Hello_World'", "Underscore (LIKE wildcard)"),
+        ("data = 'test\"quote'", "Double quote in string"),
+        ("value = 'line1\\nline2'", "Newline escape sequence"),
+        ("pattern = '%test%'", "LIKE pattern with wildcards"),
+    ]
+
+    def test_postgres_single_quote_escaping(self):
+        """PostgreSQL should use SQL-92 standard: double single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"PostgreSQL - Input: {sql}")
+        print(f"PostgreSQL - Output: {result}")
+        assert "O''Connor" in result or "O'Connor" in result.replace("''", "'")
+        assert "name" in result
+
+    def test_mysql_single_quote_escaping(self):
+        """MySQL should handle single quotes (either '' or \\')."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "mysql")
+        print(f"MySQL - Input: {sql}")
+        print(f"MySQL - Output: {result}")
+        assert "O''Connor" in result or "O\\'Connor" in result or "O'Connor" in result
+        assert "name" in result
+
+    def test_databricks_single_quote_escaping(self):
+        """Databricks should use backslash escaping for single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "databricks")
+        print(f"Databricks - Input: {sql}")
+        print(f"Databricks - Output: {result}")
+        assert "name" in result
+        assert "Connor" in result
+
+    def test_sqlite_single_quote_escaping(self):
+        """SQLite uses SQL-92 standard: double single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "sqlite")
+        print(f"SQLite - Input: {sql}")
+        print(f"SQLite - Output: {result}")
+        assert "O''Connor" in result or "O'Connor" in result
+        assert "name" in result
+
+    def test_snowflake_single_quote_escaping(self):
+        """Snowflake uses backslash escaping for single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "snowflake")
+        print(f"Snowflake - Input: {sql}")
+        print(f"Snowflake - Output: {result}")
+        assert "O''Connor" in result or "O'Connor" in result or "O\\'Connor" in result
+        assert "name" in result
+
+    def test_bigquery_single_quote_escaping(self):
+        """BigQuery should handle single quotes properly."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "bigquery")
+        print(f"BigQuery - Input: {sql}")
+        print(f"BigQuery - Output: {result}")
+        assert "Connor" in result
+        assert "name" in result
+
+    def test_mssql_single_quote_escaping(self):
+        """SQL Server uses SQL-92 standard: double single quotes."""
+        sql = "name = 'O''Connor'"
+        result = sanitize_sql_with_sqlglot(sql, "mssql")
+        print(f"MSSQL - Input: {sql}")
+        print(f"MSSQL - Output: {result}")
+        assert "O''Connor" in result or "O'Connor" in result
+        assert "name" in result
+
+
+class TestBackslashEscaping:
+    """Test backslash handling across databases."""
+
+    def test_postgres_backslash_in_path(self):
+        """PostgreSQL: backslashes in strings."""
+        sql = "path = 'C:\\\\Users\\\\test'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"PostgreSQL backslash - Input: {sql}")
+        print(f"PostgreSQL backslash - Output: {result}")
+        # Should preserve or properly escape backslashes
+        assert "path" in result
+        assert "Users" in result or "C:" in result
+
+    def test_mysql_backslash_in_path(self):
+        """MySQL treats backslash as escape character."""
+        sql = "path = 'C:\\\\Users\\\\test'"
+        result = sanitize_sql_with_sqlglot(sql, "mysql")
+        print(f"MySQL backslash - Input: {sql}")
+        print(f"MySQL backslash - Output: {result}")
+        # MySQL needs double backslashes
+        assert "path" in result
+
+    def test_databricks_backslash_in_path(self):
+        """Databricks uses backslash as escape character."""
+        sql = "path = 'C:\\\\Users\\\\test'"
+        result = sanitize_sql_with_sqlglot(sql, "databricks")
+        print(f"Databricks backslash - Input: {sql}")
+        print(f"Databricks backslash - Output: {result}")
+        assert "path" in result
+
+
+class TestComplexSQLExpressions:
+    """Test complex SQL expressions with multiple special characters."""
+
+    def test_compound_filter_with_quotes(self):
+        """Test compound filters with quoted strings."""
+        sql = "(name = 'O''Connor' AND status = 'active')"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Compound filter - Input: {sql}")
+        print(f"Compound filter - Output: {result}")
+        assert "Connor" in result
+        assert "active" in result
+        assert "AND" in result
+
+    def test_ilike_with_special_chars(self):
+        """Test ILIKE patterns with special characters."""
+        sql = "name ILIKE '%O''Connor%'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"ILIKE pattern - Input: {sql}")
+        print(f"ILIKE pattern - Output: {result}")
+        assert "Connor" in result
+        assert "ILIKE" in result or "ilike" in result.lower()
+
+    def test_in_clause_with_quoted_strings(self):
+        """Test IN clause with multiple quoted strings."""
+        sql = "name IN ('O''Connor', 'D''Angelo', 'McDonald''s')"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"IN clause - Input: {sql}")
+        print(f"IN clause - Output: {result}")
+        assert "Connor" in result
+        assert "Angelo" in result
+        assert "McDonald" in result
+        assert "IN" in result
+
+    def test_mixed_operators_with_strings(self):
+        """Test mixed operators with string literals."""
+        sql = "(price > 100 AND name ILIKE '%test%' AND status = 'active')"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Mixed operators - Input: {sql}")
+        print(f"Mixed operators - Output: {result}")
+        assert "price" in result
+        assert "100" in result
+        assert "test" in result
+        assert "active" in result
+
+
+class TestSQLInjectionPrevention:
+    """Test that SQL injection attempts are handled safely."""
+
+    def test_injection_attempt_with_quotes(self):
+        """Test SQL injection attempt using quotes."""
+        sql = "name = 'test'; DROP TABLE users; --'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Injection attempt - Input: {sql}")
+        print(f"Injection attempt - Output: {result}")
+        assert "name" in result
+        assert "test" in result
+        assert "DROP" not in result
+
+    def test_injection_with_escaped_quotes(self):
+        """Test injection attempt with pre-escaped quotes."""
+        sql = "name = 'test''; DELETE FROM users; --'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Escaped injection - Input: {sql}")
+        print(f"Escaped injection - Output: {result}")
+        assert isinstance(result, str)
+
+    def test_legitimate_quote_in_string(self):
+        """Test that legitimate quotes are preserved correctly."""
+        sql = "description = 'It''s a test'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Legitimate quote - Input: {sql}")
+        print(f"Legitimate quote - Output: {result}")
+        assert "test" in result
+        assert "It" in result
+
+
+class TestDangerousConstructsBlocked:
+    """Test that dangerous SQL constructs are blocked with ValueError."""
+
+    def test_subquery_in_in_clause_blocked(self):
+        """Subquery in IN clause should raise ValueError."""
+        sql = "id IN (SELECT id FROM users)"
+        with pytest.raises(ValueError, match="Subqueries are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_subquery_in_where_blocked(self):
+        """Subquery in comparison should raise ValueError."""
+        sql = "price > (SELECT AVG(price) FROM products)"
+        with pytest.raises(ValueError, match="Subqueries are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_exists_subquery_blocked(self):
+        """EXISTS with subquery should raise ValueError."""
+        sql = "id = 1 AND EXISTS (SELECT 1 FROM users WHERE users.id = orders.user_id)"
+        with pytest.raises(ValueError, match="Subqueries are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_union_injection_blocked(self):
+        """UNION set operation should raise ValueError."""
+        sql = "1=1 UNION SELECT * FROM users"
+        with pytest.raises(ValueError, match="Set operations are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_union_all_blocked(self):
+        """UNION ALL set operation should raise ValueError."""
+        sql = "status='active' UNION ALL SELECT password FROM users"
+        with pytest.raises(ValueError, match="Set operations are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_intersect_blocked(self):
+        """INTERSECT set operation should raise ValueError."""
+        sql = "id=1 INTERSECT SELECT id FROM admins"
+        with pytest.raises(ValueError, match="Set operations are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_except_blocked(self):
+        """EXCEPT set operation should raise ValueError."""
+        sql = "id=1 EXCEPT SELECT id FROM banned"
+        with pytest.raises(ValueError, match="Set operations are not allowed"):
+            sanitize_sql_with_sqlglot(sql, "postgresql")
+
+    def test_ddl_commands_stripped(self):
+        """DDL commands after semicolon are stripped by SQLGlot parse_one."""
+        sql = "1=1; DROP TABLE users"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "DROP" not in result
+        assert "1 = 1" in result
+
+    def test_delete_command_stripped(self):
+        """DELETE command after semicolon is stripped."""
+        sql = "1=1; DELETE FROM users"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "DELETE" not in result
+        assert "1 = 1" in result
+
+    def test_insert_command_stripped(self):
+        """INSERT command after semicolon is stripped."""
+        sql = "1=1; INSERT INTO users VALUES (1, 'admin')"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "INSERT" not in result
+        assert "1 = 1" in result
+
+    def test_update_command_stripped(self):
+        """UPDATE command after semicolon is stripped."""
+        sql = "1=1; UPDATE users SET admin=true"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "UPDATE" not in result
+        assert "1 = 1" in result
+
+    def test_create_table_command_stripped(self):
+        """CREATE TABLE command after semicolon is stripped."""
+        sql = "1=1; CREATE TABLE hacked (id INT)"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        assert "CREATE" not in result
+        assert "1 = 1" in result
+
+    def test_validation_can_be_disabled(self):
+        """When validate_structure=False, subqueries are allowed."""
+        sql = "id IN (SELECT id FROM users)"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql", validate_structure=False)
+        assert "SELECT" in result
+        assert "users" in result
+
+
+class TestDatabaseSpecificBehavior:
+    """Test database-specific SQL behaviors."""
+
+    def test_ilike_postgres_vs_mysql(self):
+        """ILIKE is PostgreSQL-specific, MySQL uses LIKE."""
+        sql = "name ILIKE '%test%'"
+
+        pg_result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"PostgreSQL ILIKE: {pg_result}")
+        # PostgreSQL should keep ILIKE
+        assert "ILIKE" in pg_result or "ilike" in pg_result.lower()
+
+        mysql_result = sanitize_sql_with_sqlglot(sql, "mysql")
+        print(f"MySQL ILIKE: {mysql_result}")
+        # MySQL might convert to LIKE or keep ILIKE depending on SQLGlot version
+        assert "LIKE" in mysql_result or "like" in mysql_result.lower()
+
+    def test_boolean_literals_across_dbs(self):
+        """Test boolean literals across databases."""
+        sql = "active = TRUE AND deleted = FALSE"
+
+        pg_result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"PostgreSQL boolean: {pg_result}")
+        assert "active" in pg_result
+        assert "deleted" in pg_result
+
+        mysql_result = sanitize_sql_with_sqlglot(sql, "mysql")
+        print(f"MySQL boolean: {mysql_result}")
+        assert "active" in mysql_result
+        assert "deleted" in mysql_result
+
+    def test_identifier_quoting(self):
+        """Test that column names with special chars are handled."""
+        sql = "\"column-name\" = 'value'"
+        result = sanitize_sql_with_sqlglot(sql, "postgresql")
+        print(f"Identifier quoting - Input: {sql}")
+        print(f"Identifier quoting - Output: {result}")
+        assert "value" in result
+
+
+class TestRealWorldAGGridScenarios:
+    """Test real-world scenarios from AG Grid filters."""

Review Comment:
   Yes the print statements are not necessary , i had put those for debugging
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["amaannawab923 (via GitHub)" <gi...@apache.org>]

amaannawab923 commented on code in PR #36136:
URL: https://github.com/apache/superset/pull/36136#discussion_r2533767495


##########
superset/common/query_object.py:
##########
@@ -336,6 +336,13 @@ def _is_valid_date_range(self, date_range: str) -> bool:
 
     def _sanitize_filters(self) -> None:
         from superset.jinja_context import get_template_processor
+        from superset.utils.sql_sanitizer import (
+            is_ag_grid_viz_type,
+            sanitize_sql_with_sqlglot,
+        )
+
+        viz_type = self.extras.get("viz_type")
+        use_sqlglot_escaping = is_ag_grid_viz_type(viz_type)

Review Comment:
   Nope , shouldnt be a negative effect, Just did it in case if any issue arises and we can remove viz type check in a follow up pr



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #36136:
URL: https://github.com/apache/superset/pull/36136#discussion_r2539250250


##########
superset/utils/sql_sanitizer.py:
##########
@@ -0,0 +1,95 @@
+# 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.
+
+from __future__ import annotations
+
+from typing import Any
+
+import sqlglot
+from sqlglot import Dialects, exp
+from sqlglot.errors import ParseError
+
+from superset.sql.parse import SQLGLOT_DIALECTS
+
+
+def get_sqlglot_dialect(database_backend: str) -> Any:
+    """Map Superset database backend name to SQLGlot dialect."""
+    return SQLGLOT_DIALECTS.get(database_backend, Dialects.DIALECT)
+
+
+def sanitize_sql_with_sqlglot(
+    raw_sql: str,
+    database_backend: str,
+    validate_structure: bool = True,
+) -> str:
+    """
+    Sanitize a raw SQL WHERE/HAVING clause using SQLGlot.
+
+    Parses the SQL into an AST and regenerates it with proper
+    escaping for the target database dialect.
+
+    Raises ValueError if SQL contains disallowed constructs
+    (subqueries, DDL/DML commands, set operations).
+    """
+    if not raw_sql or not raw_sql.strip():
+        return ""
+
+    dialect = get_sqlglot_dialect(database_backend)
+
+    try:
+        parsed = sqlglot.parse_one(raw_sql, dialect=dialect)
+
+        if validate_structure:
+            _validate_sql_structure(parsed)
+
+        return parsed.sql(dialect=dialect)
+
+    except ParseError:

Review Comment:
   When SQLGlot parsing fails with a `ParseError`, the function returns the original, unparsed SQL. This creates a security bypass: malformed SQL that can't be parsed by SQLGlot will skip validation entirely and be returned unchanged, potentially allowing injection attacks through syntax that SQLGlot doesn't understand.
   
   If validation is enabled (`validate_structure=True`), the function should raise an error on ParseError rather than returning the original SQL. The graceful fallback should only apply when validation is disabled:
   
   ```python
   except ParseError:
       if validate_structure:
           raise ValueError("Unable to parse and validate SQL clause") from None
       return raw_sql
   ```
   
   This ensures that when security validation is requested, it either succeeds or fails explicitly, rather than silently bypassing validation.
   ```suggestion
       except ParseError:
           if validate_structure:
               raise ValueError("Unable to parse and validate SQL clause") from None
   ```



##########
superset/utils/sql_sanitizer.py:
##########
@@ -0,0 +1,95 @@
+# 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.
+
+from __future__ import annotations
+
+from typing import Any
+
+import sqlglot
+from sqlglot import Dialects, exp
+from sqlglot.errors import ParseError
+
+from superset.sql.parse import SQLGLOT_DIALECTS
+
+
+def get_sqlglot_dialect(database_backend: str) -> Any:
+    """Map Superset database backend name to SQLGlot dialect."""
+    return SQLGLOT_DIALECTS.get(database_backend, Dialects.DIALECT)
+
+
+def sanitize_sql_with_sqlglot(
+    raw_sql: str,
+    database_backend: str,
+    validate_structure: bool = True,
+) -> str:
+    """
+    Sanitize a raw SQL WHERE/HAVING clause using SQLGlot.
+
+    Parses the SQL into an AST and regenerates it with proper
+    escaping for the target database dialect.
+
+    Raises ValueError if SQL contains disallowed constructs
+    (subqueries, DDL/DML commands, set operations).

Review Comment:
   The docstring states "Raises ValueError if SQL contains disallowed constructs" but doesn't document that the function also returns the original SQL unchanged for ParseError exceptions. This fallback behavior is a critical part of the function's contract and should be documented:
   
   ```python
   """
   Sanitize a raw SQL WHERE/HAVING clause using SQLGlot.
   
   Parses the SQL into an AST and regenerates it with proper
   escaping for the target database dialect.
   
   Args:
       raw_sql: The SQL clause to sanitize
       database_backend: The database backend name (e.g., 'postgresql', 'mysql')
       validate_structure: Whether to validate for dangerous SQL constructs
   
   Returns:
       The sanitized SQL clause. If SQLGlot cannot parse the SQL,
       returns the original SQL unchanged.
   
   Raises:
       ValueError: If SQL contains disallowed constructs (subqueries,
                   set operations, DDL/DML commands) when validate_structure=True
   """
   ```
   ```suggestion
       Args:
           raw_sql: The SQL clause to sanitize.
           database_backend: The database backend name (e.g., 'postgresql', 'mysql').
           validate_structure: Whether to validate for dangerous SQL constructs.
   
       Returns:
           The sanitized SQL clause. If SQLGlot cannot parse the SQL,
           returns the original SQL unchanged.
   
       Raises:
           ValueError: If SQL contains disallowed constructs (subqueries,
               DDL/DML commands, set operations) when validate_structure=True.
   ```



##########
superset/utils/sql_sanitizer.py:
##########
@@ -0,0 +1,95 @@
+# 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.
+
+from __future__ import annotations
+
+from typing import Any
+
+import sqlglot
+from sqlglot import Dialects, exp
+from sqlglot.errors import ParseError
+
+from superset.sql.parse import SQLGLOT_DIALECTS
+
+
+def get_sqlglot_dialect(database_backend: str) -> Any:
+    """Map Superset database backend name to SQLGlot dialect."""
+    return SQLGLOT_DIALECTS.get(database_backend, Dialects.DIALECT)
+
+
+def sanitize_sql_with_sqlglot(
+    raw_sql: str,
+    database_backend: str,
+    validate_structure: bool = True,
+) -> str:
+    """
+    Sanitize a raw SQL WHERE/HAVING clause using SQLGlot.
+
+    Parses the SQL into an AST and regenerates it with proper
+    escaping for the target database dialect.
+
+    Raises ValueError if SQL contains disallowed constructs
+    (subqueries, DDL/DML commands, set operations).
+    """
+    if not raw_sql or not raw_sql.strip():
+        return ""
+
+    dialect = get_sqlglot_dialect(database_backend)
+
+    try:
+        parsed = sqlglot.parse_one(raw_sql, dialect=dialect)
+
+        if validate_structure:
+            _validate_sql_structure(parsed)
+
+        return parsed.sql(dialect=dialect)
+
+    except ParseError:
+        return raw_sql
+    except ValueError:
+        raise
+    except Exception:  # pylint: disable=broad-except

Review Comment:
   The broad `except Exception` clause silently catches and suppresses all unexpected exceptions, returning the original SQL unchanged. This creates a security risk: any unexpected error during validation will cause the function to bypass security checks and return potentially dangerous SQL.
   
   If validation is enabled, unexpected exceptions should be raised rather than silently suppressed:
   
   ```python
   except Exception:  # pylint: disable=broad-except
       if validate_structure:
           raise
       return raw_sql
   ```
   
   This ensures that when security validation is requested, it either succeeds or fails explicitly, rather than silently bypassing validation.
   ```suggestion
       except Exception:  # pylint: disable=broad-except
           if validate_structure:
               raise
   ```



##########
superset/common/query_object.py:
##########
@@ -352,6 +355,15 @@ def _sanitize_filters(self) -> None:
                                 msg=ex.message,
                             )
                         ) from ex
+
+                    if use_sqlglot_escaping:
+                        database_backend = database.db_engine_spec.engine
+                        clause = sanitize_sql_with_sqlglot(
+                            clause,
+                            database_backend,
+                            validate_structure=True,
+                        )

Review Comment:
   The `sanitize_sql_with_sqlglot` function can raise `ValueError` for dangerous SQL constructs (subqueries, set operations, etc.), but this exception is not being caught in the surrounding try-except block. This will cause an unhandled `ValueError` to propagate, bypassing the error handling that converts exceptions to `QueryObjectValidationError`. 
   
   Add a catch for `ValueError` and convert it to `QueryObjectValidationError` like other exceptions in this block:
   
   ```python
   if use_sqlglot_escaping:
       try:
           database_backend = database.db_engine_spec.engine
           clause = sanitize_sql_with_sqlglot(
               clause,
               database_backend,
               validate_structure=True,
           )
       except ValueError as ex:
           raise QueryObjectValidationError(str(ex)) from ex
   ```
   ```suggestion
                           try:
                               clause = sanitize_sql_with_sqlglot(
                                   clause,
                                   database_backend,
                                   validate_structure=True,
                               )
                           except ValueError as ex:
                               raise QueryObjectValidationError(str(ex)) from ex
   ```



##########
superset/utils/sql_sanitizer.py:
##########
@@ -0,0 +1,95 @@
+# 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.
+
+from __future__ import annotations
+
+from typing import Any
+
+import sqlglot
+from sqlglot import Dialects, exp
+from sqlglot.errors import ParseError
+
+from superset.sql.parse import SQLGLOT_DIALECTS
+
+
+def get_sqlglot_dialect(database_backend: str) -> Any:
+    """Map Superset database backend name to SQLGlot dialect."""
+    return SQLGLOT_DIALECTS.get(database_backend, Dialects.DIALECT)
+
+
+def sanitize_sql_with_sqlglot(
+    raw_sql: str,
+    database_backend: str,
+    validate_structure: bool = True,
+) -> str:
+    """
+    Sanitize a raw SQL WHERE/HAVING clause using SQLGlot.
+
+    Parses the SQL into an AST and regenerates it with proper
+    escaping for the target database dialect.
+
+    Raises ValueError if SQL contains disallowed constructs
+    (subqueries, DDL/DML commands, set operations).
+    """
+    if not raw_sql or not raw_sql.strip():
+        return ""
+
+    dialect = get_sqlglot_dialect(database_backend)
+
+    try:
+        parsed = sqlglot.parse_one(raw_sql, dialect=dialect)
+
+        if validate_structure:
+            _validate_sql_structure(parsed)
+
+        return parsed.sql(dialect=dialect)
+
+    except ParseError:
+        return raw_sql
+    except ValueError:
+        raise
+    except Exception:  # pylint: disable=broad-except
+        return raw_sql
+
+
+def _validate_sql_structure(parsed: exp.Expression) -> None:
+    """Validate that parsed SQL doesn't contain dangerous constructs."""
+    disallowed_ddl_types = [
+        exp.Command,
+        exp.Create,
+        exp.Drop,
+        exp.Insert,
+        exp.Update,
+        exp.Delete,
+    ]
+
+    if hasattr(exp, "AlterTable"):
+        disallowed_ddl_types.append(exp.AlterTable)
+    if hasattr(exp, "Truncate"):
+        disallowed_ddl_types.append(exp.Truncate)
+
+    disallowed_ddl_tuple = tuple(disallowed_ddl_types)
+
+    for node in parsed.walk():
+        if isinstance(node, (exp.Subquery, exp.Select)):
+            raise ValueError("Subqueries are not allowed in filter clauses")
+
+        if isinstance(node, disallowed_ddl_tuple):
+            raise ValueError("SQL commands are not allowed in filter clauses")

Review Comment:
   The error message "SQL commands are not allowed in filter clauses" is ambiguous because it doesn't specify what kind of SQL commands. Since this catch block handles DDL/DML operations (DROP, INSERT, UPDATE, DELETE, CREATE, ALTER, TRUNCATE), the error message should be more specific:
   
   ```python
   raise ValueError("DDL/DML commands (CREATE, DROP, INSERT, UPDATE, DELETE, etc.) are not allowed in filter clauses")
   ```
   
   This helps users understand exactly what was blocked.
   ```suggestion
               raise ValueError("DDL/DML commands (CREATE, DROP, INSERT, UPDATE, DELETE, etc.) are not allowed in filter clauses")
   ```



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["amaannawab923 (via GitHub)" <gi...@apache.org>]

amaannawab923 commented on PR #36136:
URL: https://github.com/apache/superset/pull/36136#issuecomment-3549037100

   Thanks for the review @sadpandajoe !!


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["bito-code-review[bot] (via GitHub)" <gi...@apache.org>]

bito-code-review[bot] commented on PR #36136:
URL: https://github.com/apache/superset/pull/36136#issuecomment-3541195671

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] feat(ag-grid): add SQLGlot-based SQL escaping for where and having filter clauses [superset]

["alexandrusoare (via GitHub)" <gi...@apache.org>]

alexandrusoare commented on code in PR #36136:
URL: https://github.com/apache/superset/pull/36136#discussion_r2537833573


##########
superset/common/query_object.py:
##########
@@ -352,6 +355,15 @@ def _sanitize_filters(self) -> None:
                                 msg=ex.message,
                             )
                         ) from ex
+
+                    if use_sqlglot_escaping:
+                        database_backend = database.db_engine_spec.engine
+                        clause = sanitize_sql_with_sqlglot(

Review Comment:
   Can we also handle the case where the `database_backend` is for some reason undefined or None?



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org