giftig commented on code in PR #24913:
URL: https://github.com/apache/superset/pull/24913#discussion_r1287069397
##########
superset/db_engine_specs/trino.py:
##########
@@ -178,7 +191,7 @@ def prepare_cancel_query(cls, query: Query, session:
Session) -> None:
session.commit()
@classmethod
- def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) ->
bool:
+ def cancel_query(cls, cursor: Cursor, query: Query, cancel_query_id: str)
-> bool:
Review Comment:
Is it ok to declare the type as `Cursor` here? It's inheriting from the base
db engine spec where `cursor` is an `Any` so not sure if this creates a
contradiction or if python's type checking rules allows you to extend it and be
more specific. I note in other db engine specs like postgres it's also declared
as `Any`.
##########
superset/db_engine_specs/trino.py:
##########
@@ -188,11 +201,12 @@ def cancel_query(cls, cursor: Any, query: Query,
cancel_query_id: str) -> bool:
:return: True if query cancelled successfully, False otherwise
"""
try:
+ # Can't use cursor.cancel() because
+ # cursor is new object created in sql_lab.cancel_query
Review Comment:
Not exactly related to your change but I've just realised this method claims
to return `True` on success, `False` on failure, but it's actually returning
`None` on success, `False` on failure.
Might be a quick win to add a `return True` line while we're here, to fix
that.
--
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]
