bito-code-review[bot] commented on code in PR #44552:
URL: https://github.com/apache/superset/pull/44552#discussion_r4118876694
##########
superset-frontend/src/explore/types.ts:
##########
@@ -61,6 +61,8 @@ export interface ChartState {
latestQueryFormData: LatestQueryFormData;
sliceFormData: QueryFormData | null;
queryController: AbortController | null;
+ /** Client-generated id of the in-flight query, used to cancel it
server-side. */
+ latestQueryId?: string;
Review Comment:
<!-- Bito Reply -->
The reasoning regarding the lifecycle of the query identifier is sound.
Since the UI state transition effectively prevents the stop action from being
triggered after a query has finished, the identifier does not persist in a way
that would cause functional issues. Given that no other part of the application
currently reads this field, avoiding the addition of cleanup logic is an
acceptable approach to keep the codebase lean.
**superset-frontend/src/explore/types.ts**
```
/** Client-generated id of the in-flight query, used to cancel it
server-side. */
latestQueryId?: string;
```
##########
superset/common/query_context_processor.py:
##########
@@ -305,7 +306,15 @@ def get_df_payload_result(
)
)
- query_result = self.get_query_result(query_obj)
+ # Make the warehouse query cancellable by its owner for the
span
+ # of its execution, so Explore's Stop button kills the query
+ # rather than just abandoning the response. No-op for engines
+ # without cancel support, and for requests carrying no
client_id.
+ with cancellable_chart_query(
+ self._query_context.client_id,
+ getattr(self._qc_datasource, "database", None),
+ ):
+ query_result = self.get_query_result(query_obj)
annotation_data = self.get_annotation_data(query_obj)
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to include `self.get_annotation_data(query_obj)`
within the `cancellable_chart_query` context manager is technically sound in
its intent to make the annotation query cancellable. However, as you noted, the
current implementation of the context manager relies on a fixed database target
resolved at open time. Simply wrapping the annotation call would lead to
misattribution if the annotation layer points to a different database, as the
capture sink would still be scoped to the outer chart's database.
To address this correctly, the context manager would need to support
resolving the database per capture, or `cancellable_chart_query` would need to
be updated to handle dynamic database targets. Given the complexity and the
risk of misattributing cancel IDs, your decision to treat this as a non-trivial
follow-up rather than a simple one-line fix is appropriate.
--
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]
