mapledan commented on PR #42785: URL: https://github.com/apache/superset/pull/42785#issuecomment-5770183742
@rusackas You're right, and thank you for checking the code against what I wrote rather than taking the replies at face value. The fixes existed locally but I never pushed them, and I wrote those replies in the past tense as though the code was already up. That's my mistake and it cost you a review pass. They're pushed now, rebased onto master (`d670f8f4d`). What's actually in the three you traced: - **`schema` as a named parameter** — removed entirely rather than renamed. Estimation builds an unpersisted `Query` and passes that, which is where the processor has always read schema from, so the constructor goes back to its master shape and claims no keyword. `catalog` rides the same way, covering sadpandajoe's follow-up about `latest_partition` reading the connection's default. - **`executed_sql` pinning** — in the same commit, on that query. You're right that the concern isn't live: with no `template_params` passed, `sql=` and a pinned `executed_sql` reach the same `process_jinja_sql(rendered, database, None)` call, so the two authorize identical text. I said as much in that thread and kept the change as parity of mechanism with `_validate_rendered_access` rather than have it claim a hole is closed. One small correction to your tracing, since it may matter elsewhere: `process_jinja_sql` does render — `superset/sql/parse.py:2895` and `:2918` — it renders a neutralized copy of the template before parsing it. The reason the concern isn't live is that both call shapes go through that same render with no params, not that rendering is absent. - **eager `gettext()`** — now `lazy_gettext`, resolved with `str()` at each response site, following #44180 which landed on master after this branch's base. Agreed on sadpandajoe taking the next pass. Sorry for the detour. -- 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]
