bito-code-review[bot] commented on code in PR #36350:
URL: https://github.com/apache/superset/pull/36350#discussion_r2575407769
##########
superset/models/core.py:
##########
@@ -721,7 +725,55 @@ 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`.
+ # Fall back to the normal `execute` call if not available
or if
+ # the engine signature differs.
+ try:
+ if query is not None and hasattr(self.db_engine_spec,
"execute_with_cursor"):
+ logger.debug(
+ "Using db_engine_spec.execute_with_cursor for
query id=%s client_id=%s",
+ getattr(query, "id", None),
+ getattr(query, "client_id", None),
+ )
+ try:
+ # Preferred signature: (cursor, sql, query)
+
self.db_engine_spec.execute_with_cursor(cursor, sql_, query)
+ except TypeError:
+ # Some engine implementations may not accept
the `query`
+ # argument; try the two-arg form as a fallback.
+
self.db_engine_spec.execute_with_cursor(cursor, sql_)
+ else:
+ # Best-effort: some engines can expose a cancel id
prior to
+ # execution via `get_cancel_query_id` — attempt to
persist
+ # that so other processes can cancel the query.
+ try:
+ from superset.constants import QUERY_CANCEL_KEY
+ if query is not None:
+ cancel_query_id =
self.db_engine_spec.get_cancel_query_id(
+ cursor, query
+ )
+ if cancel_query_id is not None:
+
query.set_extra_json_key(QUERY_CANCEL_KEY, cancel_query_id)
+ from superset.extensions import db as
_db
+
+ _db.session.commit()
+ except Exception:
+ logger.debug(
+ "Could not obtain or persist cancel id for
query",
+ exc_info=True,
+ )
+
+ try:
+ # Preferred `execute` signature tries to
accept query kwarg
+ self.db_engine_spec.execute(cursor, sql_,
self, query=query)
+ except TypeError:
+ # Older signatures may not accept the keyword;
fall back
+ self.db_engine_spec.execute(cursor, sql_, self)
+ except Exception:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blind exception catch without specificity</b></div>
<div id="fix">
Avoid catching blind `Exception`. Specify the exception type(s) that should
be caught, such as `DatabaseError` or other specific exceptions.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (DatabaseError, OperationalError):
````
</div>
</details>
</div>
<small><i>Code Review Run #ac6df5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/models/core.py:
##########
@@ -721,7 +725,55 @@ 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`.
+ # Fall back to the normal `execute` call if not available
or if
+ # the engine signature differs.
+ try:
+ if query is not None and hasattr(self.db_engine_spec,
"execute_with_cursor"):
+ logger.debug(
+ "Using db_engine_spec.execute_with_cursor for
query id=%s client_id=%s",
+ getattr(query, "id", None),
+ getattr(query, "client_id", None),
+ )
+ try:
+ # Preferred signature: (cursor, sql, query)
+
self.db_engine_spec.execute_with_cursor(cursor, sql_, query)
+ except TypeError:
+ # Some engine implementations may not accept
the `query`
+ # argument; try the two-arg form as a fallback.
+
self.db_engine_spec.execute_with_cursor(cursor, sql_)
+ else:
+ # Best-effort: some engines can expose a cancel id
prior to
+ # execution via `get_cancel_query_id` — attempt to
persist
+ # that so other processes can cancel the query.
+ try:
+ from superset.constants import QUERY_CANCEL_KEY
+ if query is not None:
+ cancel_query_id =
self.db_engine_spec.get_cancel_query_id(
+ cursor, query
+ )
+ if cancel_query_id is not None:
+
query.set_extra_json_key(QUERY_CANCEL_KEY, cancel_query_id)
+ from superset.extensions import db as
_db
+
+ _db.session.commit()
+ except Exception:
+ logger.debug(
+ "Could not obtain or persist cancel id for
query",
+ exc_info=True,
+ )
+
+ try:
+ # Preferred `execute` signature tries to
accept query kwarg
+ self.db_engine_spec.execute(cursor, sql_,
self, query=query)
+ except TypeError:
+ # Older signatures may not accept the keyword;
fall back
+ self.db_engine_spec.execute(cursor, sql_, self)
+ except Exception:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blind exception catch without specificity</b></div>
<div id="fix">
Avoid catching blind `Exception`. Specify the exception type(s) that should
be caught, such as `TypeError` or other specific exceptions relevant to the
routing logic.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (TypeError, AttributeError):
````
</div>
</details>
</div>
<small><i>Code Review Run #ac6df5</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
