bito-code-review[bot] commented on code in PR #36350:
URL: https://github.com/apache/superset/pull/36350#discussion_r2575209729
##########
superset/connectors/sqla/models.py:
##########
@@ -1637,16 +1637,22 @@ def _get_top_groups(
return or_(*groups)
- def query(self, query_obj: QueryObjectDict) -> QueryResult:
+ def query(self, query_obj: QueryObjectDict, query: 'Query' | None = None)
-> QueryResult:
"""
Executes the query for SqlaTable with additional column ordering logic.
This overrides ExploreMixin.query() to add SqlaTable-specific behavior
for handling column_order from extras.
"""
- # Get the base result from ExploreMixin
- # (explicitly, not super() which would hit BaseDatasource first)
- result = ExploreMixin.query(self, query_obj)
+ # Get the base result from ExploreMixin, forwarding optional Query
model
+ try:
+ print(
+ f"[SqlaTable.query] dataset_id={getattr(self, 'id', None)}
forwarding query_id={getattr(query, 'id', None) if query else None}
client_id={getattr(query, 'client_id', None) if query else None}"
+ )
+ except Exception:
+ logger.debug("Could not print SqlaTable.query debug info",
exc_info=True)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Remove debug print statement</b></div>
<div id="fix">
Remove the `print()` statement on line 1649. Use proper logging instead via
the existing `logger` object for debug output.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
try:
logger.debug(
f"[SqlaTable.query] dataset_id={getattr(self, 'id', None)}
forwarding query_id={getattr(query, 'id', None) if query else None}
client_id={getattr(query, 'client_id', None) if query else None}"
)
except Exception:
logger.debug("Could not log SqlaTable.query debug info",
exc_info=True)
````
</div>
</details>
</div>
<small><i>Code Review Run #9e4459</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/charts/schemas.py:
##########
@@ -1415,6 +1415,18 @@ class ChartDataQueryContextSchema(Schema):
form_data = fields.Raw(allow_none=True, required=False)
+ client_id = fields.String(
+ allow_none=True,
+ required=False,
+ metadata={"description": "Client ID for tracking the query execution"},
+ )
+
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate field definition</b></div>
<div id="fix">
The client_id field is defined twice in the ChartDataQueryContextSchema
class, causing the second definition to overwrite the first. This duplication
may lead to confusion and incorrect metadata in the schema. It looks like the
second definition with the more detailed description should be kept.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
````
</div>
</details>
</div>
<small><i>Code Review Run #9e4459</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/common/query_context_processor.py:
##########
@@ -116,7 +117,55 @@ def get_df_payload(
)
)
- query_result = self.get_query_result(query_obj)
+ # Create a persisted Query row for chart queries so we can
expose a
+ # client_id and allow cancellation later. This mirrors SQL
Lab's
+ # behavior; it is best-effort and only created if the
datasource has
+ # an underlying database.
+ query_model = None
+ try:
+ from uuid import uuid4
+
+ from superset.extensions import db as _db
+ from superset.models.sql_lab import Query as SqlLabQuery
+ from superset.utils.core import get_user_id
+
+ # Use client_id if provided by the client (e.g., frontend)
+ provided_client_id = (
+ self._query_context.cache_values.get("client_id")
+ if isinstance(self._query_context.cache_values, dict)
+ else None
+ )
+
+ if hasattr(self._qc_datasource, "database") and getattr(
+ self._qc_datasource, "database", None
+ ) is not None:
+ client_id = provided_client_id or uuid4().hex[:11]
+
+ # If a Query with this client_id already exists, reuse
it.
+ query_model = _db.session.query(SqlLabQuery).filter_by(
+ client_id=client_id
+ ).one_or_none()
+
+ if not query_model:
+ query_model = SqlLabQuery(
+ client_id=client_id,
+ database_id=self._qc_datasource.database.id,
+ user_id=get_user_id(),
+ )
+ _db.session.add(query_model)
+ _db.session.commit()
+ # Emit debug info to help trace lifecycle of the
persisted Query
+ try:
+ print(
+ f"[query_context_processor] Query model
created/reused: id={getattr(query_model, 'id', None)} "
+ f"client_id={getattr(query_model, 'client_id',
None)} database_id={getattr(query_model, 'database_id', None)}"
+ )
+ except Exception:
+ logger.debug("Could not print query_model debug
info", exc_info=True)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blind exception catch without specificity</b></div>
<div id="fix">
Replace bare `except Exception:` with a more specific exception type to
avoid catching unexpected errors.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (AttributeError, TypeError, ValueError):
logger.debug("Could not print query_model debug
info", exc_info=True)
````
</div>
</details>
</div>
<small><i>Code Review Run #9e4459</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]
