codeant-ai-for-open-source[bot] commented on code in PR #37521:
URL: https://github.com/apache/superset/pull/37521#discussion_r2736469448
##########
superset/models/helpers.py:
##########
@@ -2800,6 +2800,21 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
col = self.convert_tbl_column_to_sqla_col(
columns_by_name[col], template_processor=template_processor
)
+ elif isinstance(col, str) and columns:
+ # Check if this is a label reference to an adhoc column
+ adhoc_col = next(
+ (
+ c
+ for c in columns
+ if utils.is_adhoc_column(c) and c.get("label") == col
+ ),
+ None,
+ )
+ if adhoc_col:
+ col = self.adhoc_column_to_sqla(
+ col=adhoc_col,
+ template_processor=template_processor,
+ )
Review Comment:
**Suggestion:** When an adhoc column is used in ORDER BY, the code converts
it to a SQLA element but does not mark that grouping may be required; for
queries with aggregates this can produce incorrect SQL or missing GROUP BY
behavior—set `need_groupby = True` after converting an adhoc column used in
ORDER BY. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Aggregated queries can produce SQL group-by errors.
- ⚠️ Sorting by adhoc columns yields incorrect results.
- ⚠️ Table charts and other aggregated charts impacted.
```
</details>
```suggestion
# Adhoc columns used in ORDER BY may require GROUP BY to
be applied
need_groupby = True
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a chart (for example, a Table) that requests aggregated metrics
(e.g., SUM,
COUNT) and also includes an adhoc (custom SQL) column in its columns list.
2. Enable server-side pagination or otherwise cause the backend to receive
an orderby
referencing the adhoc column's label; the request reaches get_sqla_query and
executes the
ORDER BY processing code at superset/models/helpers.py:2803-2817.
3. Unlike other branches for metrics/physical columns (which set
need_groupby = True at
lines near 2790 and earlier), this adhoc-column-by-label branch converts the
adhoc column
into a SQLA element but does not set need_groupby = True. Later in
get_sqla_query the
value of need_groupby controls whether the code treats columns as group-by
expressions
(affecting select_exprs, group_by, and aggregation behavior).
4. Because need_groupby remains False in queries that include aggregates,
the generated
SQL can be missing required GROUP BY clauses or may place the adhoc column
in SELECT
without grouping, causing SQL errors from the DB (e.g., "column must appear
in the GROUP
BY clause") or incorrect/ambiguous aggregation results when sorting by the
adhoc column.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 2818:2818
**Comment:**
*Logic Error: When an adhoc column is used in ORDER BY, the code
converts it to a SQLA element but does not mark that grouping may be required;
for queries with aggregates this can produce incorrect SQL or missing GROUP BY
behavior—set `need_groupby = True` after converting an adhoc column used in
ORDER BY.
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]
