codeant-ai-for-open-source[bot] commented on code in PR #42284: URL: https://github.com/apache/superset/pull/42284#discussion_r3707613700
########## 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) Review Comment: **Suggestion:** Adhoc x-axis objects are reduced to `column_name`, discarding their `sqlExpression`, label, and other metadata. For calculated x-axis dimensions that have no physical `column_name`, the rebuilt query silently omits the dimension; when a physical name is present, it queries that column rather than the configured expression. Preserve the complete adhoc column object as the frontend query builder does. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Calculated x-axis dimensions disappear from rebuilt exports. - ⚠️ Exported grouping differs from the saved chart configuration. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7c8b2a774a69473f9f314eebab8b18d6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7c8b2a774a69473f9f314eebab8b18d6&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:** 130:136 **Comment:** *Type Error: Adhoc x-axis objects are reduced to `column_name`, discarding their `sqlExpression`, label, and other metadata. For calculated x-axis dimensions that have no physical `column_name`, the rebuilt query silently omits the dimension; when a physical name is present, it queries that column rather than the configured expression. Preserve the complete adhoc column object as the frontend query builder does. 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=02f5b1e72e1ec83bd5ae4dcfded6f330d0fbc283a65e47a6bb6e3281eb5dd155&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=02f5b1e72e1ec83bd5ae4dcfded6f330d0fbc283a65e47a6bb6e3281eb5dd155&reaction=dislike'>👎</a> ########## superset/tasks/export_dashboard_excel.py: ########## @@ -67,6 +68,12 @@ # image. Operators can override the set via ``EXCEL_EXPORT_TABLE_VIZ_TYPES``. TABLE_VIZ_TYPES = {"table", "pivot_table_v2", "pivot_table"} +# Viz types whose missing query context may be rebuilt from saved form data. +# Conservative: only charts whose data maps faithfully to a single plain query +# (no post-processing, no multi-query fan-out). Every other viz type without a +# saved query context is skipped and listed for the user to re-save in Explore. +REBUILD_VIZ_TYPES = {"table", "big_number_total", "big_number", "pie"} Review Comment: **Suggestion:** The rebuild allowlist includes `pie`, but every pie query adds a contribution post-processing operation in the frontend, regardless of `percent_metrics`. Rebuilt pie queries omit that operation and therefore export raw metric values without the contribution column used by the chart. Either implement the required post-processing or exclude pie from rebuilding. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Pie exports omit the chart's contribution percentages. - ⚠️ Exported pie data does not match displayed chart data. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=26db992090f94330933e55d2874d62bb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=26db992090f94330933e55d2874d62bb&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/tasks/export_dashboard_excel.py **Line:** 75:75 **Comment:** *Incomplete Implementation: The rebuild allowlist includes `pie`, but every pie query adds a contribution post-processing operation in the frontend, regardless of `percent_metrics`. Rebuilt pie queries omit that operation and therefore export raw metric values without the contribution column used by the chart. Either implement the required post-processing or exclude pie from rebuilding. 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=c916036b26adc6e279e3cd4be865165a6a99226bb4b3ee224caf3972e718cfe3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42284&comment_hash=c916036b26adc6e279e3cd4be865165a6a99226bb4b3ee224caf3972e718cfe3&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]
