bito-code-review[bot] commented on code in PR #36872:
URL: https://github.com/apache/superset/pull/36872#discussion_r2655933608
##########
superset/db_engine_specs/trino.py:
##########
@@ -187,17 +191,48 @@ def handle_cursor(cls, cursor: Cursor, query: Query) ->
None:
db.session.commit() # pylint: disable=consider-using-transaction
- # if query cancelation was requested prior to the handle_cursor call,
but
- # the query was still executed, trigger the actual query cancelation
now
- if query.extra.get(QUERY_EARLY_CANCEL_KEY):
- cls.cancel_query(
- cursor=cursor,
- query=query,
- cancel_query_id=cancel_query_id,
- )
-
super().handle_cursor(cursor=cursor, query=query)
+ state = "QUEUED"
+ progress = 0.0
+ poll_interval = app.config["DB_POLL_INTERVAL_SECONDS"].get(cls.engine,
1)
+ while state != "FINISHED":
+ # Check for errors raised in execute_thread
+ if execute_result is not None and execute_result.get("error"):
+ break
+
+ # Check if execute_event is set (thread completed or error
occurred)
+ if execute_event is not None and execute_event.is_set():
+ # If there's an error, break; otherwise, it's a normal
completion,
+ # so break after the final state update
+ if execute_result and execute_result.get("error"):
+ break
+
+ # if query cancelation was requested prior to the handle_cursor
call, but
+ # the query was still executed, trigger the actual query
cancelation now
+ if query.extra.get(QUERY_EARLY_CANCEL_KEY) or query.status in [
+ QueryStatus.STOPPED,
+ QueryStatus.TIMED_OUT,
+ ]:
+ cls.cancel_query(
+ cursor=cursor,
+ query=query,
+ cancel_query_id=cancel_query_id,
+ )
+ break
+
+ info = getattr(cursor, "stats", {}) or {}
+ state = info.get("state", "UNKNOWN")
+ completed_splits = float(info.get("completedSplits", 0))
+ total_splits = float(info.get("totalSplits"))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Runtime TypeError Risk</b></div>
<div id="fix">
If `totalSplits` is absent from cursor stats (e.g., early query states),
`info.get("totalSplits")` returns None, causing `float(None)` to raise
TypeError. Defaulting to 1 matches the `(total_splits or 1)` logic and test
expectations.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/db_engine_specs/trino.py
+++ superset/db_engine_specs/trino.py
@@ -226,7 +226,7 @@
info = getattr(cursor, "stats", {}) or {}
state = info.get("state", "UNKNOWN")
completed_splits = float(info.get("completedSplits", 0))
- total_splits = float(info.get("totalSplits"))
+ total_splits = float(info.get("totalSplits", 1))
progress = completed_splits / (total_splits or 1)
if progress != query.progress:
```
</div>
</details>
</div>
<small><i>Code Review Run #0a2a3b</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/db_engine_specs/trino.py:
##########
@@ -187,17 +191,48 @@ def handle_cursor(cls, cursor: Cursor, query: Query) ->
None:
db.session.commit() # pylint: disable=consider-using-transaction
- # if query cancelation was requested prior to the handle_cursor call,
but
- # the query was still executed, trigger the actual query cancelation
now
- if query.extra.get(QUERY_EARLY_CANCEL_KEY):
- cls.cancel_query(
- cursor=cursor,
- query=query,
- cancel_query_id=cancel_query_id,
- )
-
super().handle_cursor(cursor=cursor, query=query)
+ state = "QUEUED"
+ progress = 0.0
+ poll_interval = app.config["DB_POLL_INTERVAL_SECONDS"].get(cls.engine,
1)
+ while state != "FINISHED":
+ # Check for errors raised in execute_thread
+ if execute_result is not None and execute_result.get("error"):
+ break
+
+ # Check if execute_event is set (thread completed or error
occurred)
+ if execute_event is not None and execute_event.is_set():
+ # If there's an error, break; otherwise, it's a normal
completion,
+ # so break after the final state update
+ if execute_result and execute_result.get("error"):
+ break
+
+ # if query cancelation was requested prior to the handle_cursor
call, but
+ # the query was still executed, trigger the actual query
cancelation now
+ if query.extra.get(QUERY_EARLY_CANCEL_KEY) or query.status in [
+ QueryStatus.STOPPED,
+ QueryStatus.TIMED_OUT,
+ ]:
+ cls.cancel_query(
+ cursor=cursor,
+ query=query,
+ cancel_query_id=cancel_query_id,
+ )
+ break
+
+ info = getattr(cursor, "stats", {}) or {}
+ state = info.get("state", "UNKNOWN")
+ completed_splits = float(info.get("completedSplits", 0))
+ total_splits = float(info.get("totalSplits"))
+ progress = completed_splits / (total_splits or 1)
+
+ if progress != query.progress:
+ query.progress = progress
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Type Mismatch in Progress Assignment</b></div>
<div id="fix">
The Query.progress column is Integer (0-100), but assigning float (0.0-1.0)
risks SQLAlchemy conversion errors or truncation. Frontend treats it as
percentage, so scale to 0-100.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/db_engine_specs/trino.py
+++ superset/db_engine_specs/trino.py
@@ -228,7 +228,7 @@
progress = completed_splits / (total_splits or 1)
if progress != query.progress:
- query.progress = progress
+ query.progress = int(progress * 100)
db.session.commit() # pylint:
disable=consider-using-transaction
time.sleep(poll_interval)
```
</div>
</details>
</div>
<small><i>Code Review Run #0a2a3b</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]
