fix(sqllab): reject cost estimation of a templated query instead of a syntax error - #42785
fix(sqllab): reject cost estimation of a templated query instead of a syntax error#42785mapledan wants to merge 2 commits into
Conversation
… syntax error
QueryEstimationCommand.run() only renders Jinja when template_params is
non-empty, but a whole class of template functions needs no declared
parameter at all (get_time_filter(), current_username(), url_param()).
A query using one of them skips rendering entirely and the raw {%/{{/{#
reaches SQLScript(), which raises a generic parse error -- "Perhaps
there was a misspelling or a typo" for a query that is perfectly valid.
Rather than rendering unconditionally to avoid that error, refuse the
estimate: a templated query has no single execution plan. What it
expands to is decided 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, with nothing to signal the difference.
Whether SQL carries a template is asked of the template processor, which
owns that knowledge and answers with its own environment, so customized
delimiters are honored and NoOpTemplateProcessor correctly reports none
when ENABLE_TEMPLATE_PROCESSING is off. It lexes rather than parses, so
that a comment -- which leaves no trace in a parsed template -- counts,
and so that SQL merely containing braces, such as the PostgreSQL nested
array literal '{{1,2},{3,4}}', does not.
dosubot
Bot
added
global:jinja
Code Review Agent Run #aab734Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The issue is that To resolve this, you should check if there are any tokens other than return any(kind != "data" for _, kind, _ in self.env.lex(sql))This change ensures that if any token is found that is not classified as superset/jinja_context.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42785 +/- ##
==========================================
- Coverage 65.79% 65.78% -0.01%
==========================================
Files 2841 2842 +1
Lines 162082 162117 +35
Branches 37145 37148 +3
==========================================
+ Hits 106638 106656 +18
- Misses 53380 53395 +15
- Partials 2064 2066 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`kinds > {"data"}` is a strict superset test, so it required a `data`
token to be present alongside the template ones. SQL that is nothing but
a template -- `{{ dataset(1) }}` -- lexes without any `data` token, so
has_template() returned False for it and the raw Jinja went on to
SQLScript(), producing exactly the misleading syntax error this change
set out to replace.
Asking for any kind other than `data` instead. The whole token stream is
still consumed before deciding, which any() would not do: `'{{1,2},{3,4}}'`
opens like a template and only turns out not to be one further along.
SUMMARY
QueryEstimationCommand.run()renders Jinja only whentemplate_paramsis non-empty. Butget_time_filter(),current_username()and friends need no declared parameter, and SQL Lab always POSTstemplate_params: {}to/api/v1/sqllab/estimate/— so rendering is skipped, the raw{%reachesSQLScript(), and the user gets Issue 1003, "there is a syntax error in the SQL query, perhaps there was a misspelling or a typo", for a query that runs fine in SQL Lab.Rendering unconditionally would silence the error but estimate the wrong thing: a template expands at run time, and with no dashboard in play
get_time_filter()yields no filter at all, so the reported cost would be for a query missing its time predicate. The query is refused instead, with an explanation.Whether SQL carries a template is asked of the template processor (new
has_template()), which lexes with its own environment — soENABLE_TEMPLATE_PROCESSINGbeing off, and SQL that merely contains braces such as'{{1,2},{3,4}}', both behave correctly.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
A valid query is reported as a typo.
After
The reason is stated, and what to do about it.
TESTING INSTRUCTIONS
ESTIMATE_QUERY_COSTandENABLE_TEMPLATE_PROCESSINGinFEATURE_FLAGS."cost_estimate_enabled": trueto a Postgres database'sextra.Error parsing near '{%' at line 2:2. After: the explanation above.SELECT 1still estimates normally.SELECT '{{1,2},{3,4}}'::int[]still estimates normally.Unit tests:
pytest tests/unit_tests/commands/sql_lab/test_estimate.py tests/unit_tests/jinja_context_test.pyADDITIONAL INFORMATION