codeant-ai-for-open-source[bot] commented on code in PR #42284:
URL: https://github.com/apache/superset/pull/42284#discussion_r3691934958


##########
superset/common/form_data_query_context.py:
##########
@@ -0,0 +1,290 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Synthesize a query context from a chart's saved form data (``params``).
+
+A chart's ``query_context`` is normally generated client-side by each viz
+plugin's ``buildQuery`` and only persisted when the chart is (re-)saved in
+Explore. Charts that predate that behavior keep their ``params`` (form data) 
but
+carry no ``query_context``, so server-side consumers that need to run the query
+(e.g. the dashboard Excel export) have nothing to execute.
+
+This module rebuilds a best-effort query context from the form data — columns,
+metrics, filters (including free-form SQL and the time range), ordering and 
time
+grain — mirroring the shared parts of the viz plugins' ``buildQuery``. It does
+**not** reproduce plugin post-processing (pivot, contribution/percent
+transforms, rolling/forecast) or multi-query fan-out, so callers must restrict 
it
+to viz types whose data maps faithfully to a single plain query.
+
+The mirrored logic lives on the frontend in
+``superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts`` (query mode,
+ordering), ``superset-frontend/packages/superset-ui-core/src/query/`` (field
+extraction, ``processFilters``). There is no automated tripwire tying the two
+across the language boundary; the per-helper pointers below must be kept in 
sync
+when that frontend logic changes.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from superset.utils import json
+
+
+def adhoc_filters_to_query_filters(
+    adhoc_filters: list[dict[str, Any]],
+    where_only: bool = False,
+) -> list[dict[str, Any]]:
+    """
+    Convert ``SIMPLE`` adhoc filters into QueryObject filter clauses.
+
+    Adhoc filters use ``{subject, operator, comparator}`` while a query object
+    expects ``{col, op, val}``; free-form ``SQL`` filters have no ``{col, op,
+    val}`` equivalent and are handled separately (see
+    :func:`freeform_where_having`).
+
+    By default all ``SIMPLE`` filters are converted (the behavior the MCP
+    compile/preview path relies on). Pass ``where_only=True`` to convert only
+    ``WHERE``-clause filters, matching the frontend's ``processFilters``
+    (``superset-ui-core/src/query/processFilters.ts``) — the dashboard export 
uses
+    this so it applies the same rows the chart shows and does not additionally
+    filter on ``SIMPLE`` ``HAVING`` clauses.
+    """
+    result: list[dict[str, Any]] = []
+    for flt in adhoc_filters or []:
+        if flt.get("expressionType") != "SIMPLE":
+            continue
+        if where_only and (flt.get("clause") or "WHERE").upper() != "WHERE":
+            continue
+        result.append(
+            {
+                "col": flt.get("subject"),
+                "op": flt.get("operator"),
+                "val": flt.get("comparator"),
+            }
+        )
+    return result
+
+
+def freeform_where_having(form_data: dict[str, Any]) -> dict[str, str]:
+    """
+    Collect free-form SQL predicates into a query ``extras`` mapping.
+
+    Mirrors ``processFilters`` on the frontend
+    (``superset-ui-core/src/query/processFilters.ts``): ``SQL`` adhoc filters 
(and
+    a legacy top-level ``where``) join into ``extras.where`` / 
``extras.having`` by
+    clause, so a chart restricted by a custom SQL predicate exports the same 
rows
+    it displays instead of the full, unrestricted result.
+    """
+    where: list[str] = []
+    having: list[str] = []
+    if form_data.get("where"):
+        where.append(form_data["where"])
+    for flt in form_data.get("adhoc_filters") or []:
+        if flt.get("expressionType") == "SQL" and flt.get("sqlExpression"):
+            clause = (flt.get("clause") or "WHERE").upper()
+            (having if clause == "HAVING" else 
where).append(flt["sqlExpression"])
+
+    extras: dict[str, str] = {}
+    if where:
+        extras["where"] = " AND ".join(f"({clause})" for clause in where)
+    if having:
+        extras["having"] = " AND ".join(f"({clause})" for clause in having)

Review Comment:
   **Suggestion:** SQL expressions containing a trailing `--` comment are 
wrapped directly in parentheses without the newline sanitization used by the 
frontend. The comment can consume the generated closing parenthesis, producing 
invalid SQL or changing the predicate's structure during export. [logic error]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ Eligible legacy chart export can fail during query execution.
   - ⚠️ Charts with SQL comments produce invalid predicates.
   - ⚠️ Frontend and rebuilt exports handle comments differently.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=06b6e371748d4dbb8205b6af262e4ecd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=06b6e371748d4dbb8205b6af262e4ecd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/common/form_data_query_context.py
   **Line:** 102:106
   **Comment:**
        *Logic Error: SQL expressions containing a trailing `--` comment are 
wrapped directly in parentheses without the newline sanitization used by the 
frontend. The comment can consume the generated closing parenthesis, producing 
invalid SQL or changing the predicate's structure during export.
   
   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.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=dea73d9ee077e7dd4ded08a7a20abc5ab2ec630adfadaee452b551753e6db0c6&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=dea73d9ee077e7dd4ded08a7a20abc5ab2ec630adfadaee452b551753e6db0c6&reaction=dislike'>👎</a>



##########
superset/common/form_data_query_context.py:
##########
@@ -0,0 +1,290 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Synthesize a query context from a chart's saved form data (``params``).
+
+A chart's ``query_context`` is normally generated client-side by each viz
+plugin's ``buildQuery`` and only persisted when the chart is (re-)saved in
+Explore. Charts that predate that behavior keep their ``params`` (form data) 
but
+carry no ``query_context``, so server-side consumers that need to run the query
+(e.g. the dashboard Excel export) have nothing to execute.
+
+This module rebuilds a best-effort query context from the form data — columns,
+metrics, filters (including free-form SQL and the time range), ordering and 
time
+grain — mirroring the shared parts of the viz plugins' ``buildQuery``. It does
+**not** reproduce plugin post-processing (pivot, contribution/percent
+transforms, rolling/forecast) or multi-query fan-out, so callers must restrict 
it
+to viz types whose data maps faithfully to a single plain query.
+
+The mirrored logic lives on the frontend in
+``superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts`` (query mode,
+ordering), ``superset-frontend/packages/superset-ui-core/src/query/`` (field
+extraction, ``processFilters``). There is no automated tripwire tying the two
+across the language boundary; the per-helper pointers below must be kept in 
sync
+when that frontend logic changes.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from superset.utils import json
+
+
+def adhoc_filters_to_query_filters(
+    adhoc_filters: list[dict[str, Any]],
+    where_only: bool = False,
+) -> list[dict[str, Any]]:
+    """
+    Convert ``SIMPLE`` adhoc filters into QueryObject filter clauses.
+
+    Adhoc filters use ``{subject, operator, comparator}`` while a query object
+    expects ``{col, op, val}``; free-form ``SQL`` filters have no ``{col, op,
+    val}`` equivalent and are handled separately (see
+    :func:`freeform_where_having`).
+
+    By default all ``SIMPLE`` filters are converted (the behavior the MCP
+    compile/preview path relies on). Pass ``where_only=True`` to convert only
+    ``WHERE``-clause filters, matching the frontend's ``processFilters``
+    (``superset-ui-core/src/query/processFilters.ts``) — the dashboard export 
uses
+    this so it applies the same rows the chart shows and does not additionally
+    filter on ``SIMPLE`` ``HAVING`` clauses.
+    """
+    result: list[dict[str, Any]] = []
+    for flt in adhoc_filters or []:
+        if flt.get("expressionType") != "SIMPLE":
+            continue
+        if where_only and (flt.get("clause") or "WHERE").upper() != "WHERE":
+            continue
+        result.append(
+            {
+                "col": flt.get("subject"),
+                "op": flt.get("operator"),
+                "val": flt.get("comparator"),
+            }
+        )
+    return result
+
+
+def freeform_where_having(form_data: dict[str, Any]) -> dict[str, str]:
+    """

Review Comment:
   **Suggestion:** Aggregate table form data can retain a non-empty `columns` 
value from an earlier raw-mode configuration. This branch prefers that stale 
list over `groupby`, unlike the frontend aggregate query builder, so rebuilt 
exports can aggregate by the wrong dimensions or omit the chart's actual 
grouping. Select `groupby` when the query is aggregate mode rather than using 
raw `columns` there. [incorrect condition logic]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ Legacy aggregate table exports use wrong dimensions.
   - ⚠️ Exported aggregates differ from Explore results.
   - ⚠️ Row limits can select incorrect grouped rows.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=887c23ad63c84a09a2f317b3abf65d8e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=887c23ad63c84a09a2f317b3abf65d8e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/common/form_data_query_context.py
   **Line:** 84:84
   **Comment:**
        *Incorrect Condition Logic: Aggregate table form data can retain a 
non-empty `columns` value from an earlier raw-mode configuration. This branch 
prefers that stale list over `groupby`, unlike the frontend aggregate query 
builder, so rebuilt exports can aggregate by the wrong dimensions or omit the 
chart's actual grouping. Select `groupby` when the query is aggregate mode 
rather than using raw `columns` there.
   
   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.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=82beb693d56cddb66ab1de6f025af3612d43103bc8e6637c62dddb5ec8b3a3c8&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=82beb693d56cddb66ab1de6f025af3612d43103bc8e6637c62dddb5ec8b3a3c8&reaction=dislike'>👎</a>



##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -39,26 +39,14 @@
 
 
 def _build_query_columns(form_data: Dict[str, Any]) -> list[str]:
-    """Build query columns list from form_data, including both x_axis and 
groupby."""
-    # Table charts in raw mode use all_columns or columns
-    all_columns = form_data.get("all_columns", [])
-    raw_columns_field = form_data.get("columns", [])
-    if form_data.get("query_mode") == "raw" and (all_columns or 
raw_columns_field):
-        return list(all_columns or raw_columns_field)
-
-    x_axis_config = form_data.get("x_axis")
-    groupby_columns: list[str] = form_data.get("groupby") or []
-    raw_columns: list[str] = form_data.get("columns") or []
-
-    columns = raw_columns.copy() if "columns" in form_data else 
groupby_columns.copy()
-    if x_axis_config and isinstance(x_axis_config, str):
-        if x_axis_config not in columns:
-            columns.insert(0, x_axis_config)
-    elif x_axis_config and isinstance(x_axis_config, dict):
-        col_name = x_axis_config.get("column_name")
-        if col_name and col_name not in columns:
-            columns.insert(0, col_name)
-    return columns
+    """Build query columns list from form_data, including both x_axis and 
groupby.
+
+    Delegates to the shared builder so the MCP and dashboard-export paths stay 
in
+    sync (single source of truth).
+    """
+    from superset.common.form_data_query_context import columns_from_form_data
+
+    return columns_from_form_data(form_data)

Review Comment:
   **Suggestion:** The shared helper now replaces the preview path's 
chart-aware extraction but only reads `groupby`, `columns`, and `x_axis`. The 
MCP preview compiler supports pivot dimensions in 
`groupbyRows`/`groupbyColumns` and secondary dimensions in `groupby_b`; those 
charts now lose required columns and generate previews from an incomplete 
query. Preserve the chart-specific dimensions or extend the shared helper to 
handle these shapes. [incomplete implementation]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ MCP pivot previews omit required dimensions.
   - ❌ Mixed-series previews omit secondary grouping.
   - ⚠️ Preview data no longer matches requested chart configuration.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=82da61d9126a40269c310399fce474b2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=82da61d9126a40269c310399fce474b2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/mcp_service/chart/preview_utils.py
   **Line:** 47:49
   **Comment:**
        *Incomplete Implementation: The shared helper now replaces the preview 
path's chart-aware extraction but only reads `groupby`, `columns`, and 
`x_axis`. The MCP preview compiler supports pivot dimensions in 
`groupbyRows`/`groupbyColumns` and secondary dimensions in `groupby_b`; those 
charts now lose required columns and generate previews from an incomplete 
query. Preserve the chart-specific dimensions or extend the shared helper to 
handle these shapes.
   
   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.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=e1063ff91c147cd6794077f9cdbb9ab6b98e79c8111f0abe282e960d3edd0c53&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=e1063ff91c147cd6794077f9cdbb9ab6b98e79c8111f0abe282e960d3edd0c53&reaction=dislike'>👎</a>



##########
superset/common/form_data_query_context.py:
##########
@@ -0,0 +1,290 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Synthesize a query context from a chart's saved form data (``params``).
+
+A chart's ``query_context`` is normally generated client-side by each viz
+plugin's ``buildQuery`` and only persisted when the chart is (re-)saved in
+Explore. Charts that predate that behavior keep their ``params`` (form data) 
but
+carry no ``query_context``, so server-side consumers that need to run the query
+(e.g. the dashboard Excel export) have nothing to execute.
+
+This module rebuilds a best-effort query context from the form data — columns,
+metrics, filters (including free-form SQL and the time range), ordering and 
time
+grain — mirroring the shared parts of the viz plugins' ``buildQuery``. It does
+**not** reproduce plugin post-processing (pivot, contribution/percent
+transforms, rolling/forecast) or multi-query fan-out, so callers must restrict 
it
+to viz types whose data maps faithfully to a single plain query.
+
+The mirrored logic lives on the frontend in
+``superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts`` (query mode,
+ordering), ``superset-frontend/packages/superset-ui-core/src/query/`` (field
+extraction, ``processFilters``). There is no automated tripwire tying the two
+across the language boundary; the per-helper pointers below must be kept in 
sync
+when that frontend logic changes.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from superset.utils import json
+
+
+def adhoc_filters_to_query_filters(
+    adhoc_filters: list[dict[str, Any]],
+    where_only: bool = False,
+) -> list[dict[str, Any]]:
+    """
+    Convert ``SIMPLE`` adhoc filters into QueryObject filter clauses.
+
+    Adhoc filters use ``{subject, operator, comparator}`` while a query object
+    expects ``{col, op, val}``; free-form ``SQL`` filters have no ``{col, op,
+    val}`` equivalent and are handled separately (see
+    :func:`freeform_where_having`).
+
+    By default all ``SIMPLE`` filters are converted (the behavior the MCP
+    compile/preview path relies on). Pass ``where_only=True`` to convert only
+    ``WHERE``-clause filters, matching the frontend's ``processFilters``
+    (``superset-ui-core/src/query/processFilters.ts``) — the dashboard export 
uses
+    this so it applies the same rows the chart shows and does not additionally
+    filter on ``SIMPLE`` ``HAVING`` clauses.
+    """
+    result: list[dict[str, Any]] = []
+    for flt in adhoc_filters or []:
+        if flt.get("expressionType") != "SIMPLE":
+            continue
+        if where_only and (flt.get("clause") or "WHERE").upper() != "WHERE":
+            continue
+        result.append(
+            {
+                "col": flt.get("subject"),
+                "op": flt.get("operator"),
+                "val": flt.get("comparator"),
+            }
+        )
+    return result
+
+
+def freeform_where_having(form_data: dict[str, Any]) -> dict[str, str]:
+    """
+    Collect free-form SQL predicates into a query ``extras`` mapping.
+
+    Mirrors ``processFilters`` on the frontend
+    (``superset-ui-core/src/query/processFilters.ts``): ``SQL`` adhoc filters 
(and
+    a legacy top-level ``where``) join into ``extras.where`` / 
``extras.having`` by
+    clause, so a chart restricted by a custom SQL predicate exports the same 
rows
+    it displays instead of the full, unrestricted result.
+    """
+    where: list[str] = []
+    having: list[str] = []
+    if form_data.get("where"):
+        where.append(form_data["where"])
+    for flt in form_data.get("adhoc_filters") or []:
+        if flt.get("expressionType") == "SQL" and flt.get("sqlExpression"):
+            clause = (flt.get("clause") or "WHERE").upper()
+            (having if clause == "HAVING" else 
where).append(flt["sqlExpression"])
+
+    extras: dict[str, str] = {}
+    if where:
+        extras["where"] = " AND ".join(f"({clause})" for clause in where)
+    if having:
+        extras["having"] = " AND ".join(f"({clause})" for clause in having)
+    return extras
+
+
+def columns_from_form_data(form_data: dict[str, Any]) -> list[Any]:
+    """
+    Derive the query's grouping/raw columns from form data.
+
+    Handles raw-mode tables (``all_columns``/``columns``), an ``x_axis`` 
(string
+    or adhoc column), and ``groupby`` dimensions, de-duplicating while 
preserving
+    order.
+    """
+    if form_data.get("query_mode") == "raw" and (
+        form_data.get("all_columns") or form_data.get("columns")
+    ):
+        return list(form_data.get("all_columns") or form_data.get("columns") 
or [])
+
+    groupby_columns: list[Any] = form_data.get("groupby") or []
+    raw_columns: list[Any] = form_data.get("columns") or []
+    # Prefer explicit raw columns only when they are actually present; a stale
+    # empty ``columns: []`` key must not shadow the group-by dimensions (which
+    # would silently drop the grouping and change the aggregation).
+    columns = raw_columns.copy() if raw_columns else groupby_columns.copy()
+
+    x_axis = form_data.get("x_axis")
+    if isinstance(x_axis, str) and x_axis and x_axis not in columns:
+        columns.insert(0, x_axis)
+    elif isinstance(x_axis, dict):
+        col_name = x_axis.get("column_name")
+        if col_name and col_name not in columns:
+            columns.insert(0, col_name)
+    return columns
+
+
+def is_raw_query_mode(form_data: dict[str, Any]) -> bool:
+    """
+    Whether the chart runs in raw (non-aggregated) mode, mirroring the 
frontend's
+    ``getQueryMode`` (``plugin-chart-table/src/buildQuery.ts``): an explicit
+    ``query_mode`` wins, otherwise the presence of ``all_columns`` implies raw 
mode.
+    """
+    if mode := form_data.get("query_mode"):
+        return mode == "raw"
+    return bool(form_data.get("all_columns"))
+
+
+def orderby_from_form_data(
+    form_data: dict[str, Any], metrics: list[Any], viz_type: str | None = None
+) -> list[list[Any]]:
+    """
+    Derive ordering so a ``row_limit`` returns the chart's top-N, not an
+    arbitrary N.
+
+    Raw-mode tables order by ``order_by_cols`` (stored as JSON ``[col, asc]``
+    pairs). Aggregate charts order by the configured sort metric
+    (``timeseries_limit_metric``, or the first metric when ``sort_by_metric`` 
is
+    set), otherwise fall back to the first metric descending — matching the
+    table/pie ``buildQuery`` defaults.
+
+    ``order_by_cols`` is a raw-mode-only control (``resetOnHide: false`` in the
+    plugin control panels), so an aggregate chart can carry a stale value from 
a
+    previous raw-mode configuration. Aggregate mode must ignore it, mirroring 
the
+    frontend, where ``plugin-chart-table/src/buildQuery.ts:136-145`` overrides
+    ``orderby`` with the sort metric (``order_by_cols`` reaches ``orderby`` 
only
+    via the alias in ``extractQueryFields.ts``, then gets overwritten in 
aggregate
+    mode).
+    """
+    if is_raw_query_mode(form_data):
+        parsed: list[list[Any]] = []
+        for col in form_data.get("order_by_cols") or []:
+            if isinstance(col, str):
+                try:
+                    col = json.loads(col)
+                except (TypeError, ValueError):
+                    continue
+            parsed.append(col)
+        return parsed
+
+    if not metrics:
+        return []
+
+    sort_metric = form_data.get("timeseries_limit_metric") or (
+        metrics[0] if form_data.get("sort_by_metric") else None
+    )
+    if sort_metric is not None:

Review Comment:
   **Suggestion:** `timeseries_limit_metric` can be either a metric or a 
one-element list, but the frontend normalizes it with `ensureIsArray(...)[0]`. 
Passing the raw list through creates an invalid nested `orderby` metric shape 
for saved charts using the list representation, which can fail query validation 
or produce incorrect ordering. [type error]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ Legacy table export can fail query validation.
   - ⚠️ Row-limited exports can use invalid ordering.
   - ⚠️ Exported top-N rows may differ from Explore.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fa6828a690434bc2a1b93e679c9b6f71&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fa6828a690434bc2a1b93e679c9b6f71&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/common/form_data_query_context.py
   **Line:** 186:189
   **Comment:**
        *Type Error: `timeseries_limit_metric` can be either a metric or a 
one-element list, but the frontend normalizes it with `ensureIsArray(...)[0]`. 
Passing the raw list through creates an invalid nested `orderby` metric shape 
for saved charts using the list representation, which can fail query validation 
or produce incorrect ordering.
   
   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.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=ac5edf1db093d12b8f67658af9a0d1abf3fa23a27a1c57794197276e502f39c2&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=ac5edf1db093d12b8f67658af9a0d1abf3fa23a27a1c57794197276e502f39c2&reaction=dislike'>👎</a>



-- 
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]

Reply via email to