mapledan commented on PR #42785: URL: https://github.com/apache/superset/pull/42785#issuecomment-5594084566
Thanks @rusackas, and sorry for the long gap — I took it back to draft because the change had moved away from what the description said, and because I wanted to be honest with myself about how narrow it actually is. Rebased onto master as one commit, and the three threads are answered individually and marked resolved. What changed since @sadpandajoe's review: the first version refused to estimate any templated query. That was wrong, and his first comment is what turned it around — when the request supplies the parameters, the rendered SQL is exactly what Run would execute, so the estimate is honest and I was taking it away. The estimate endpoint is also SQL Lab-only, where neither path has a dashboard context, so a context macro renders the same for an estimate as for a run. Nothing is refused now. What is left is one gate: `run()` rendered Jinja only when `template_params` was non-empty, while `SqlQueryRenderImpl.render`, `validate_sql`, and `process_jinja_sql` as called from this command's own `validate()` all render regardless. Removing it makes the four agree, and fixes the reported query on its own. On scope, since it is fair to ask: reaching this needs two feature flags that are off by default plus a per-database `extra`, on an engine that implements cost estimation. So I would frame this as a consistency fix rather than a widely-hit bug — the upside is small, and so is the risk. I've rewritten the title and description to lead with that rather than with the error message, which is where the old description had drifted. Ready for another look whenever you have time. -- 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]
