mapledan commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r3963632031
##########
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:
You're right, and it made me re-check the premise the whole thing rested on.
The estimate endpoint is SQL Lab-only (QuerySource.SQL_LAB), and SQL Lab has
no dashboard context either — so a context macro renders the same on the
estimate path as it does on Run. Refusing was wrong both for the bound case you
describe and for the unbound one I was worried about.
Nothing is refused now. The gate on template_params is gone, so {{ ds }}
with {"ds": ...} renders and estimates exactly as before. That also turns out
to fix the case I opened the PR for, on its own — a query using only
get_time_filter() declares no parameter, so the gate never rendered it and the
raw {% reached SQLScript().
Worth noting validate() right above already jinja-processes through
process_jinja_sql without consulting template_params, so the two halves of this
command now agree.
##########
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:
Confirmed — your SQL reproduces it.
I tried the direction you suggested first, and keeping the tokens seen
before the
error flips the array literal on its own to `True`: `{{1,2}` emits
`variable_begin`
before the lexer gives up, so "saw a template token" cannot separate the two
cases.
What does separate them is requiring the construct to have *closed*. The
literal is
abandoned unterminated, so it never emits `variable_end`:
| SQL | want | seen-tokens | closed-only |
|---|---|---|---|
| `SELECT '{{1,2},{3,4}}'::int[]` | False | True ❌ | False ✅ |
| `SELECT '{{ current_username() }}', '{{1,2},{3,4}}'::int[]` | True | True
✅ | True ✅ |
Changed to that, with your example as a test case.
Also worth saying that the stakes here dropped along the way: detection now
only
picks the wording of a parse error that is raised either way, so getting it
wrong
costs the better message and nothing else.
--
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]
