rusackas commented on PR #36350: URL: https://github.com/apache/superset/pull/36350#issuecomment-4773331404
Thanks @X-arshiya-X, this is going after a real and long-standing annoyance, so I appreciate you picking it up. GitHub shows conflicts, so it needs a rebase on current `master` before we can dig in. I'll see if I can help get it closer to merging. The part I want to ask about is creating a SQL Lab `Query` row on every chart render to get a `client_id`. That's a notable side effect (extra rows, ownership/cleanup) for what's effectively a render path. Is there a lighter way to thread the cancel id through without minting a persisted query each time? The other thing giving me hesitation here is the `try/except` + `except TypeError` signature-sniffing in `models/core.py` and `models/helpers.py` (calling `execute_with_cursor`, falling back, then guessing at the `execute(..., query=...)` kwarg). Probing engine-spec signatures at runtime and swallowing the failures could hide real errors. Could the engine-spec interface change explicitly instead? And there are no tests on a change touching the query lifecycle across 14 files. A couple tests pinning down the cancel-actually-fires behavior would let us re-review with a lot more confidence. Thanks! -- 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]
