codeant-ai-for-open-source[bot] commented on code in PR #36872:
URL: https://github.com/apache/superset/pull/36872#discussion_r2661366472
##########
superset/db_engine_specs/trino.py:
##########
@@ -187,17 +191,45 @@ 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":
Review Comment:
**Suggestion:** The polling loop only exits when state == "FINISHED", but
Trino can end in other terminal states (e.g., FAILED, CANCELED, ERROR); if the
cursor reports such a state the loop will never terminate. Include other
terminal states in the loop condition (or break when encountering them). [logic
error]
**Severity Level:** Minor ⚠️
```suggestion
while state not in ("FINISHED", "FAILED", "CANCELED", "ERROR"):
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Trino can reach terminal states other than "FINISHED" (e.g. FAILED,
CANCELED). Relying only on "FINISHED" risks looping until the thread event or
error is observed; detecting other terminal states here makes the polling loop
exit sooner and more deterministically. It's a logical correctness 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:** 199:199
**Comment:**
*Logic Error: The polling loop only exits when state == "FINISHED", but
Trino can end in other terminal states (e.g., FAILED, CANCELED, ERROR); if the
cursor reports such a state the loop will never terminate. Include other
terminal states in the loop condition (or break when encountering them).
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,45 @@ 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:** Retrieving the poll interval using
app.config["DB_POLL_INTERVAL_SECONDS"].get(...) assumes the config value is a
dict; if the config key is missing or is an int/float this will raise a
KeyError or AttributeError at runtime. Use app.config.get(...) and handle both
dict and numeric types. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
poll_config = app.config.get("DB_POLL_INTERVAL_SECONDS", {})
if isinstance(poll_config, (int, float)):
poll_interval = poll_config
else:
poll_interval = poll_config.get(cls.engine, 1)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current line assumes DB_POLL_INTERVAL_SECONDS is present and a dict-like
object; if the config key is missing this raises KeyError and if it's a numeric
value it would raise AttributeError on .get(). The proposed change safely reads
the config and handles both numeric and mapping shapes, preventing runtime
exceptions. This is a real robustness fix.
</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: Retrieving the poll interval using
app.config["DB_POLL_INTERVAL_SECONDS"].get(...) assumes the config value is a
dict; if the config key is missing or is an int/float this will raise a
KeyError or AttributeError at runtime. Use app.config.get(...) and handle both
dict and numeric types.
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]
