john-bodley commented on code in PR #28357:
URL: https://github.com/apache/superset/pull/28357#discussion_r1591389498
##########
superset/sql_parse.py:
##########
@@ -1582,7 +1582,7 @@ def extract_tables_from_jinja_sql(sql: str, database:
Database) -> set[Table]:
return (
tables
| ParsedQuery(
- sql_statement=processor.process_template(template),
Review Comment:
@toniphan21 I _think_ the current logic (which I added) might be correct,
i.e., we first have to render/sanitize the template in order for it to be
"proper" SQL so that SQLGlot can actually parse it.
In your PR description you mentioned,
> There are 2 ways to extract table set:
>
> 1. Extract any tables referenced within the confines of specific Jinja
macros.
> 2. Parse SQL and get table.
whereas in actuality we need to do both, which is what the current code does.
That said it's interesting that Mypy didn't barf on the fact that the first
argument to `process_template` is of type `nodes.Template` rather than `str`.
[Here's](https://github.com/apache/superset/blob/master/superset/jinja_context.py#L500)
were the argument is processed, where the
[from_string()](https://github.com/pallets/jinja/blob/main/src/jinja2/environment.py#L1091-L1110)
method accepts either a `node.Template` or `str` (albeit being named to only
accept the late).
The TL;DR is I think the code is correct, though the `process_template()`
method could be updated to include a union of `str | nodes.Template`.
--
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]
