sadpandajoe commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r3820988796
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -148,9 +149,20 @@ def run(
) -> list[dict[str, Any]]:
self.validate()
+ template_processor = get_template_processor(self._database)
+ # A templated query has no single execution plan: it expands using
+ # values only available at run time (a dashboard's time range, the
+ # current user, a URL parameter), and different expansions can produce
+ # different plans. Estimating one of them -- here, the emptiest one,
+ # with no such context to expand from -- would report the plan of a
+ # different query than the one that runs. Refusing before the SQL
+ # reaches `SQLScript` also replaces the parse error the raw `{%` would
+ # otherwise trigger, which reads as a typo in a valid query.
+ if template_processor.has_template(self._sql):
Review Comment:
This now rejects templates even when the request already supplies every
value, so SQL Lab loses cost estimates for queries whose rendered SQL is
deterministic. For example, a query using `{{ ds }}` with `{"ds":
"2026-08-20"}` used to render before `EXPLAIN`, while Run still renders the
same input. Could this render fully bound parameters and reserve the refusal
for context-dependent templates?
##########
superset/jinja_context.py:
##########
@@ -797,6 +797,36 @@ def get_context(self) -> dict[str, Any]:
"""
return self._context.copy()
+ def has_template(self, sql: str) -> bool:
+ """Whether the SQL contains anything for ``process_template`` to expand
+
+ Lexed rather than parsed, so that a comment -- which leaves no trace in
+ a parsed template -- still counts, and using this processor's own
+ environment, so that any customized delimiters are honored. Lexing
+ evaluates nothing.
+
+ >>> has_template("SELECT '{{ current_username() }}'")
+ True
+ >>> has_template("{{ dataset(1) }}")
+ True
+ >>> has_template("SELECT '{{1,2},{3,4}}'::int[]")
+ False
+ """
+ try:
+ # The whole stream is consumed before deciding, rather than
stopping
+ # at the first non-`data` token: `'{{1,2},{3,4}}'` opens like a
+ # template and only turns out not to be one further along, where
the
+ # lexer gives up.
+ kinds = {kind for _, kind, _ in self.env.lex(sql)}
+ except TemplateSyntaxError:
+ # Jinja does not recognize this as one of its own constructs, so
+ # there is nothing here it would expand.
+ return False
+
+ # SQL that is nothing but a template produces no `data` token at all,
so
+ # this asks for any other kind rather than for `data` plus another.
+ return bool(kinds - {"data"})
Review Comment:
Custom template processors can override `process_template` with non-Jinja
syntax but inherit this Jinja-only detector. The existing
`CustomPrestoTemplateProcessor` expands `$DATE(...)`; it is all `data` here, so
estimate still passes the raw macro to the engine instead of producing the new
error. Could the processor contract require/implement matching `has_template`
behavior, with a regression test for that processor?
##########
superset/jinja_context.py:
##########
@@ -797,6 +797,36 @@ def get_context(self) -> dict[str, Any]:
"""
return self._context.copy()
+ def has_template(self, sql: str) -> bool:
+ """Whether the SQL contains anything for ``process_template`` to expand
+
+ Lexed rather than parsed, so that a comment -- which leaves no trace in
+ a parsed template -- still counts, and using this processor's own
+ environment, so that any customized delimiters are honored. Lexing
+ evaluates nothing.
+
+ >>> has_template("SELECT '{{ current_username() }}'")
+ True
+ >>> has_template("{{ dataset(1) }}")
+ True
+ >>> has_template("SELECT '{{1,2},{3,4}}'::int[]")
+ False
+ """
+ try:
+ # The whole stream is consumed before deciding, rather than
stopping
+ # at the first non-`data` token: `'{{1,2},{3,4}}'` opens like a
+ # template and only turns out not to be one further along, where
the
+ # lexer gives up.
+ kinds = {kind for _, kind, _ in self.env.lex(sql)}
+ except TemplateSyntaxError:
Review Comment:
A later array-literal lexer error discards an earlier real template and
returns `False`, so this can still estimate raw, non-executable SQL. `SELECT
'{{ current_username() }}', '{{1,2},{3,4}}'::int[]` hits this path: execution
attempts to render the first expression, but estimation sends it through
unrendered. Should detection preserve already-seen template tokens (or
otherwise reject this mixed case)?
--
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]
