sha174n commented on PR #36350: URL: https://github.com/apache/superset/pull/36350#issuecomment-5368855838
Nice work wiring the stop button through to real backend cancellation. A few things before it can go in: (1) superset/explorables/base.py imports Query from superset.migrations.shared.catalogs at runtime, which is the migration-scoped model, not superset.models.sql_lab.Query, and it pulls the migrations package into the core import graph; please move it under if TYPE_CHECKING: and import from superset.models.sql_lab. (2) get_df_payload persists and commits a Query row on every chart cache-miss, but these rows never get a terminal status and are only hidden from Query History by the sql IS NOT NULL filter, so on busy dashboards the query table grows unbounded plus an extra write per render; could these get a lifecycle (terminal status + cleanup) or a lighter tracking approach? (3) the diff carries a lot of unrelated reformatting; a rebase on latest master (which also clears the current conflict) plus dropping the unrelated churn would make this much easier to review. -- 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]
