codeant-ai-for-open-source[bot] commented on code in PR #36872:
URL: https://github.com/apache/superset/pull/36872#discussion_r2653718368
##########
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 = cursor.stats
+ state = info["state"]
+ completed_splits = info["completedSplits"]
+ total_splits = info["totalSplits"]
Review Comment:
**Suggestion:** Reading `cursor.stats` and indexing keys directly can raise
KeyError or TypeError if `stats` is None, missing keys, or contains non-numeric
types; normalize and guard the values before using them in arithmetic to avoid
runtime exceptions. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
info = getattr(cursor, "stats", {}) or {}
state = info.get("state", "UNKNOWN")
try:
completed_splits = int(info.get("completedSplits", 0) or 0)
except Exception:
completed_splits = 0
try:
total_splits = int(info.get("totalSplits", 0) or 0)
except Exception:
total_splits = 0
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Accessing cursor.stats keys without guards can raise KeyError or TypeError
if stats is None or shaped differently. Normalizing with getattr and using .get
+ safe int coercion prevents crashes during progress calculation. The suggested
change improves robustness. Minor nit: the suggestion swallows conversion
errors with broad except, which I'd tighten to ValueError/TypeError, but the
core idea is valid.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/trino.py
**Line:** 224:227
**Comment:**
*Possible Bug: Reading `cursor.stats` and indexing keys directly can
raise KeyError or TypeError if `stats` is None, missing keys, or contains
non-numeric types; normalize and guard the values before using them in
arithmetic to avoid runtime exceptions.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
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)
Review Comment:
**Suggestion:** Accessing app.config["DB_POLL_INTERVAL_SECONDS"] and calling
.get(...) assumes that the config value is a mapping; if the config key holds
an int or other non-dict value this will raise AttributeError at runtime. Use
app.config.get(...) and handle both dict and scalar cases. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
poll_config = app.config.get("DB_POLL_INTERVAL_SECONDS", {})
if isinstance(poll_config, dict):
poll_interval = poll_config.get(cls.engine, 1)
else:
try:
poll_interval = int(poll_config)
except Exception:
poll_interval = 1
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code assumes the config entry is a mapping and directly calls
.get on it. That's a realistic source of runtime AttributeError if an operator
sets DB_POLL_INTERVAL_SECONDS to an int or other scalar. The proposed defensive
handling is reasonable and hardens the code. Note that in upstream Superset
this config is typically a mapping, so this change is defensive rather than
strictly necessary — but it's a harmless and practical improvement.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/trino.py
**Line:** 198:198
**Comment:**
*Possible Bug: Accessing app.config["DB_POLL_INTERVAL_SECONDS"] and
calling .get(...) assumes that the config value is a mapping; if the config key
holds an int or other non-dict value this will raise AttributeError at runtime.
Use app.config.get(...) and handle both dict and scalar cases.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
