sadpandajoe commented on code in PR #36350:
URL: https://github.com/apache/superset/pull/36350#discussion_r3824427279
##########
superset/common/query_context_processor.py:
##########
@@ -249,7 +314,9 @@ def query_cache_key(self, query_obj: QueryObject, **kwargs:
Any) -> str | None:
)
return cache_key
- def get_query_result(self, query_object: QueryObject) -> QueryResult:
+ def get_query_result(
+ self, query_object: QueryObject, query: Query | None = None
Review Comment:
The new tracking row is dropped when a query takes the grouping-sets
fallback: this method calls `_grouping_sets_fallback()` without forwarding
`query`, and each subquery then runs without the row needed to persist its
cancellation key. Should the fallback accept and pass the tracking row through
its datasource calls?
##########
superset/models/core.py:
##########
@@ -929,7 +933,46 @@ def _log_query(sql_: str) -> None:
database=self,
object_ref=__name__,
):
- self.db_engine_spec.execute(cursor, sql_, self)
+ # If a Query model was provided, prefer to call the
engine's
+ # `execute_with_cursor` path so engines that need the
running
+ # cursor (eg Trino) can capture a cancel id via
`handle_cursor`.
+ if query is not None and hasattr(
+ self.db_engine_spec, "execute_with_cursor"
+ ):
+ self.db_engine_spec.execute_with_cursor(cursor, sql_,
query)
Review Comment:
This takes the inherited `execute_with_cursor` path for every engine, but
the base implementation only persists a cancellation key when
`has_query_id_before_execute` is false. For the default case, the new chart row
reaches `cancel_query()` without `QUERY_CANCEL_KEY`, so Stop still cannot
cancel the query. Should this use the pre-execution capture path for engines
that need it rather than selecting the inherited method by `hasattr`?
##########
superset-frontend/src/components/Chart/chartAction.ts:
##########
@@ -544,6 +552,7 @@ export async function getChartDataRequest({
setDataMask,
ownState,
parseMethod,
+ clientId,
Review Comment:
Legacy chart requests return before this client ID is added to their
payload, so those visualizations create no cancellable backend query even
though the reducer records the ID. Clicking Stop only aborts the browser
request for that path. Should the legacy request carry the same identifier, or
should Stop be disabled until that path is supported?
--
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]
