rusackas commented on code in PR #41125:
URL: https://github.com/apache/superset/pull/41125#discussion_r3463888717
##########
superset/sql/parse.py:
##########
@@ -1840,16 +1840,35 @@ def process_jinja_sql(
def sanitize_clause(clause: str, engine: str) -> str:
"""
- Make sure the SQL clause is valid.
+ Validate a SQL clause and return it unchanged.
+
+ The clause is parsed to ensure it is a single, well-formed statement. We
+ intentionally return the *original* text rather than a re-rendered version:
+ round-tripping user SQL through SQLGlot's dialect generator can silently
+ alter semantics. For example, the Postgres dialect (borrowed by several
+ engines) rewrites ``ROUND(AVG(x), n)`` to ``ROUND(CAST(AVG(x) AS DECIMAL),
+ n)``, which rounds the value to an integer before the explicit ``ROUND`` on
+ engines whose unqualified ``DECIMAL`` defaults to scale 0 (see #36113).
+
+ Comments are the one exception: a trailing line comment can comment out
+ surrounding SQL once the clause is embedded into a larger query (e.g.
+ wrapped in parentheses), so any clause that contains comments is
re-rendered
+ to normalize them into a safe form. A trailing statement terminator is
+ likewise stripped, since callers embed the clause inside a larger fragment
+ (``WHERE (...)``) where a stray ``;`` would produce invalid SQL.
"""
try:
statement = SQLStatement(clause, engine)
+ parsed = statement._parsed # pylint: disable=protected-access
+ if not any(node.comments for node in parsed.walk()):
+ return clause.rstrip().rstrip(";").rstrip()
Review Comment:
You'\''re right, the comment path was leaking the engine rewrite back in.
Switched the re-render to the base dialect (`_normalized_generator(None, ...)`)
so comments still normalize but #36113 stays fixed.
##########
tests/unit_tests/sql/parse_tests.py:
##########
@@ -3208,6 +3213,31 @@ def test_sanitize_clause(sql: str, expected: str |
Exception, engine: str) -> No
sanitize_clause(sql, engine)
[email protected](
+ "engine",
+ ["postgresql", "redshift", "cockroachdb", "netezza", "hana", "base",
"mysql"],
+)
+def test_sanitize_clause_preserves_aggregation_semantics(engine: str) -> None:
+ """
+ Regression test for https://github.com/apache/superset/issues/36113.
+
+ `sanitize_clause` must not silently rewrite a user-authored expression. The
+ Postgres SQLGlot dialect (which several engines borrow) rewrites
+ ``ROUND(AVG(x), n)`` to ``ROUND(CAST(AVG(x) AS DECIMAL), n)`` at generation
+ time. On engines whose unqualified ``DECIMAL`` defaults to scale 0 (e.g.
+ Redshift, Netezza) the injected cast rounds the aggregate to an integer
+ *before* the explicit ``ROUND``, producing wrong results.
+
+ The clause must be returned unchanged regardless of the engine dialect.
+ """
+ clause = "ROUND(AVG(col), 4)"
+ sanitized = sanitize_clause(clause, engine)
+ assert "CAST" not in sanitized.upper(), (
+ f"sanitize_clause injected a cast for engine {engine!r}: {sanitized!r}"
+ )
+ assert sanitized == clause
Review Comment:
Added `test_sanitize_clause_preserves_aggregation_semantics_with_comment`
covering the comment path across the same engine set. Confirmed it goes red
without the base-dialect change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
