This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new b1185d8c05c fix(sql): preserve optimizer hints when formatting
semicolon-terminated statements with trailing comments (#43565)
b1185d8c05c is described below
commit b1185d8c05c3e08611159cfeb3e6f130c15c0685
Author: Francesco.Castaldi <[email protected]>
AuthorDate: Wed Sep 9 19:44:01 2026 +0200
fix(sql): preserve optimizer hints when formatting semicolon-terminated
statements with trailing comments (#43565)
Co-authored-by: FrancescoCastaldi <[email protected]>
---
superset/sql/parse.py | 66 +++++++++++++++++++++++++++++++---
tests/unit_tests/sql/parse_tests.py | 71 +++++++++++++++++++++++++++++++------
2 files changed, 121 insertions(+), 16 deletions(-)
diff --git a/superset/sql/parse.py b/superset/sql/parse.py
index 73106258da0..d0caba97aca 100644
--- a/superset/sql/parse.py
+++ b/superset/sql/parse.py
@@ -735,6 +735,66 @@ class BaseSQLStatement(Generic[InternalRepresentation]):
return self.format()
+_SELECT_TRAILING_CLAUSES: tuple[str, ...] = (
+ "options",
+ "settings",
+ "format",
+ "locks",
+ "offset",
+ "limit",
+ "sort",
+ "cluster",
+ "distribute",
+ "order",
+ "windows",
+ "qualify",
+ "having",
+ "group",
+ "where",
+ "joins",
+ "laterals",
+ "from",
+ "into",
+ "expressions",
+)
+
+
+def _get_select_trailing_child(node: exp.Select) -> exp.Expression | None:
+ for clause_name in _SELECT_TRAILING_CLAUSES:
+ val = node.args.get(clause_name)
+ if isinstance(val, list) and val:
+ return _find_last_token_node(val[-1])
+ if isinstance(val, exp.Expression):
+ return _find_last_token_node(val)
+ return None
+
+
+def _find_last_token_node(node: exp.Expression) -> exp.Expression:
+ """
+ Find the last token/leaf node in SQL generation order to attach trailing
comments.
+
+ Avoids optimizer hints (exp.Hint) and non-trailing subtrees to prevent
injecting
+ trailing comments inside optimizer hint blocks (e.g. /*+ SET_VAR(...) */).
+ """
+ if isinstance(node, exp.Select):
+ if trailing := _get_select_trailing_child(node):
+ return trailing
+
+ children: list[exp.Expression] = []
+ for k, v in node.args.items():
+ if k in ("hint", "comments"):
+ continue
+ if isinstance(v, exp.Expression):
+ children.append(v)
+ elif isinstance(v, list):
+ children.extend(item for item in v if isinstance(item,
exp.Expression))
+
+ if children:
+ return _find_last_token_node(children[-1])
+
+ return node
+
+
class SQLStatement(BaseSQLStatement[exp.Expression]):
"""
A SQL statement.
@@ -932,11 +992,7 @@ class SQLStatement(BaseSQLStatement[exp.Expression]):
# statement; move them back to the last token in the last real
statement
if len(statements) > 1 and isinstance(statements[-1], exp.Semicolon):
last_statement = statements.pop()
- target = statements[-1]
- for node in statements[-1].walk():
- if hasattr(node, "comments"): # pragma: no cover
- target = node
-
+ target = _find_last_token_node(statements[-1])
target.comments = target.comments or []
target.comments.extend(last_statement.comments)
diff --git a/tests/unit_tests/sql/parse_tests.py
b/tests/unit_tests/sql/parse_tests.py
index 085450aa7c3..d2d38b96038 100644
--- a/tests/unit_tests/sql/parse_tests.py
+++ b/tests/unit_tests/sql/parse_tests.py
@@ -29,6 +29,8 @@ from superset.jinja_context import JinjaTemplateProcessor
from superset.sql.parse import (
_check_script_length,
_count_weighted_table_references,
+ _find_last_token_node,
+ _get_select_trailing_child,
BaseSQLStatement,
count_referenced_tables,
CTASMethod,
@@ -1358,20 +1360,12 @@ LIMIT 100
assert "increase timeout for large scans" in formatted[hint_end:]
[email protected](
- reason=(
- "#38189 is not fully fixed: a `;`-terminated statement still hits "
- "the comment-relocation branch and corrupts the hint block. Only "
- "the no-semicolon form from the original repro was fixed."
- ),
- strict=True,
-)
def test_sqlscript_format_preserves_optimizer_hint_block_with_semicolon() ->
None:
"""
Same as `test_sqlscript_format_preserves_optimizer_hint_block`, but with
- a terminating `;` on the statement -- this still reproduces #38189: the
- trailing `--` comment gets injected inside the `/*+ SET_VAR(...) */`
- hint block, corrupting it for StarRocks/MySQL-style engines.
+ a terminating `;` on the statement -- verifies #38189 fix so that trailing
+ `--` comments land after the statement rather than injected into the
+ `/*+ SET_VAR(...) */` hint block for StarRocks/MySQL-style engines.
"""
sql = """SELECT /*+ SET_VAR(query_timeout = 3000) */ col1, col2
FROM my_table
@@ -1388,6 +1382,61 @@ LIMIT 100;
assert "increase timeout for large scans" in formatted[hint_end:]
+def test_sqlscript_format_preserves_optimizer_hint_with_cte_and_semicolon() ->
None:
+ """
+ Ensure optimizer hints with CTEs and trailing comments survive formatting
intact.
+ """
+ sql = """WITH cte AS (SELECT 1 AS id)
+SELECT /*+ SET_VAR(query_timeout = 3000) */ id
+FROM cte
+WHERE id = 1;
+
+-- trailing explanation comment"""
+ statement = SQLScript(sql, "mysql").statements[0]
+ formatted = statement.format()
+
+ hint = "/*+ SET_VAR(query_timeout = 3000) */"
+ assert hint in formatted
+ assert "SET_VAR(query_timeout /*" not in formatted
+ hint_end = formatted.index(hint) + len(hint)
+ assert "trailing explanation comment" in formatted[hint_end:]
+
+
+def test_find_last_token_node_branches() -> None:
+ """
+ Directly test all branches of _find_last_token_node and
_get_select_trailing_child.
+ """
+ # 1. Empty select returns None from _get_select_trailing_child
+ # and falls back to node
+ empty_select = exp.Select()
+ assert _get_select_trailing_child(empty_select) is None
+ assert _find_last_token_node(empty_select) is empty_select
+
+ # 2. Select with list clause vs single Expression clause
+ select_with_exprs = exp.Select(expressions=[exp.Literal.number(1)])
+ assert _get_select_trailing_child(select_with_exprs) ==
exp.Literal.number(1)
+
+ select_with_where = exp.Select(where=exp.Where(this=exp.Literal.number(2)))
+ assert _get_select_trailing_child(select_with_where) ==
exp.Literal.number(2)
+
+ # 3. Node with hint or comments in args is skipped during child traversal
+ col_with_comment = exp.Column(this="foo", comments=["my comment"])
+ assert _find_last_token_node(col_with_comment) is not None
+
+ table_with_hint = exp.Table(
+ this="bar", hint=exp.Hint(expressions=[exp.var("HINT")])
+ )
+ assert _find_last_token_node(table_with_hint) is not None
+
+ # 4. Non-select node with list of expressions
+ tup = exp.Tuple(expressions=[exp.Literal.number(1), exp.Literal.number(2)])
+ assert _find_last_token_node(tup) == exp.Literal.number(2)
+
+ # 5. Leaf node with no children returns itself
+ lit = exp.Literal.number(42)
+ assert _find_last_token_node(lit) is lit
+
+
@pytest.mark.parametrize(
"sql, engine, expected",
[
