feat(dashboard): handle empty chart query context in Excel export - #42284
Conversation
Code Review Agent Run #f56d0eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42284 +/- ##
==========================================
- Coverage 66.60% 66.29% -0.31%
==========================================
Files 2863 2859 -4
Lines 161721 161430 -291
Branches 37258 37128 -130
==========================================
- Hits 107709 107020 -689
- Misses 51967 52364 +397
- Partials 2045 2046 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #4ba2e4Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
How Has This Been Tested? (real full stack, Docker)Validated end-to-end against the live stack (Flask + Celery worker + Postgres + Redis + MinIO/S3 + Mailpit/SMTP) serving this branch via bind mount. Bring-up (from workspace root, with the S3/SMTP verify overlay): Test case — the natural one: Superset's example dashboards ship charts that have no saved
Before this PR every one of these 8 charts would have been skipped (empty workbook); now the 3 rebuildable ones export with correct data. Non-visual proof:
Screenshots (in |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| groupby_columns: list[Any] = form_data.get("groupby") or [] | ||
| raw_columns: list[Any] = form_data.get("columns") or [] | ||
| columns = raw_columns.copy() if "columns" in form_data else groupby_columns.copy() |
There was a problem hiding this comment.
Suggestion: This selection logic drops groupby dimensions whenever the columns key exists, even if columns is an empty list. Many saved form-data payloads include empty columns by default, so exports can run with missing grouping columns and incorrect results. [logic error]
Severity Level: Critical 🚨
- Exported table sheets lose group-by columns.
- Aggregations no longer grouped as in original charts.
- Dashboard Excel exports show semantically different data.
- Issue affects legacy charts without saved query_context.
- Impacts REBUILD_VIZ_TYPES such as table exports.Steps of Reproduction ✅
1. Create or locate a legacy chart with viz_type "table" whose saved form data (stored in
`Slice.params`) contains a non-empty `groupby` (for example `["country"]`) and an
explicitly present but empty `columns` list (this is the payload read in
`superset/tasks/export_dashboard_excel.py:145` when `_resolve_query_context()` parses
`chart.params`).
2. Add this chart to a dashboard and ensure it has no saved `query_context` (the column is
NULL or an empty/invalid JSON string), so that `_resolve_query_context()` at
`superset/tasks/export_dashboard_excel.py:130-156` will execute the form-data rebuild path
instead of using a saved context.
3. Trigger a dashboard Excel export (the Flask route enqueues the Celery task in
`superset/tasks/export_dashboard_excel.py`, which calls `_build_workbook()` at
`superset/tasks/export_dashboard_excel.py:244` and iterates charts via
`get_charts_in_layout_order(dashboard)` at line 261).
4. For this table chart, `_build_workbook()` calls `_resolve_query_context()` (line 269),
which calls `build_query_context_from_form_data()` at
`superset/common/form_data_query_context.py:92`; inside it, `columns_from_form_data()` at
line 65 executes the logic at lines 78–80: because the `columns` key is present (even
though it is an empty list), `columns = raw_columns.copy()` sets `columns` to `[]` and
discards the non-empty `groupby_columns`, so the rebuilt query context has no grouping
columns, and the exported Excel sheet aggregates metrics without the expected group-by
dimension, producing different results than the chart in Explore.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/form_data_query_context.py
**Line:** 78:80
**Comment:**
*Logic Error: This selection logic drops `groupby` dimensions whenever the `columns` key exists, even if `columns` is an empty list. Many saved form-data payloads include empty `columns` by default, so exports can run with missing grouping columns and incorrect results.
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 fixThere was a problem hiding this comment.
Good catch — valid latent bug. Fixed in de7dfc1: columns_from_form_data now prefers raw columns only when non-empty, otherwise falls back to groupby, so a stale columns: [] no longer drops the grouping. Added a unit test.
| "columns": columns, | ||
| "metrics": metrics, | ||
| "orderby": form_data.get("orderby") or [], | ||
| "filters": adhoc_filters_to_query_filters(form_data.get("adhoc_filters", [])), |
There was a problem hiding this comment.
Suggestion: The rebuild only maps adhoc_filters and ignores legacy/simple filters stored directly in form data. Charts that still rely on filters will export unfiltered or partially filtered data, producing incorrect Excel output. [logic error]
Severity Level: Critical 🚨
- Excel exports ignore legacy form_data filters.
- Dashboard exports include rows that charts filter out.
- Data consumers see mismatched filtered versus exported data.
- Affects legacy charts without saved query_context.
- Undermines trust in dashboard Excel export accuracy.Steps of Reproduction ✅
1. Locate or create a legacy chart (for example viz_type "table" or "pie") whose saved
form data uses the older `filters` field instead of `adhoc_filters` (for example
`form_data["filters"] = [{"col": "country", "op": "==", "val": "US"}]`) and has no
persisted `query_context`; this form data is what `chart.params` contains and is parsed in
`_resolve_query_context()` at `superset/tasks/export_dashboard_excel.py:145-148`.
2. Add this chart to a dashboard and trigger a dashboard Excel export so that the Celery
task in `superset/tasks/export_dashboard_excel.py` runs `_build_workbook()` at line 244
and iterates charts via `get_charts_in_layout_order(dashboard)` at line 261.
3. For this chart, `_build_workbook()` calls `_resolve_query_context()` at line 269;
because `chart.query_context` is empty, `_resolve_query_context()` falls back to
`build_query_context_from_form_data()` in
`superset/common/form_data_query_context.py:92-125`, passing the parsed `form_data`
dictionary.
4. Inside `build_query_context_from_form_data()`, the query payload is constructed at
lines 115–121; the `filters` entry is set solely from
`adhoc_filters_to_query_filters(form_data.get("adhoc_filters", []))` (line 119), and the
legacy `form_data["filters"]` is never read, so the rebuilt query context has an empty
`filters` list, causing `ChartDataCommand` (invoked downstream in `_write_chart_sheets()`
at `superset/tasks/export_dashboard_excel.py:192-213`) to execute an unfiltered query; the
resulting Excel sheet includes all rows instead of only those matching the original
`filters`, silently diverging from what users see in Explore.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/form_data_query_context.py
**Line:** 119:119
**Comment:**
*Logic Error: The rebuild only maps `adhoc_filters` and ignores legacy/simple `filters` stored directly in form data. Charts that still rely on `filters` will export unfiltered or partially filtered data, producing incorrect Excel output.
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 fixThere was a problem hiding this comment.
Fixed in de7dfc1: build_query_context_from_form_data now also honors legacy simple filters (already in {col, op, val} shape) alongside adhoc_filters, dropping malformed entries. Added a unit test. Legacy charts (the target of this feature) are exactly where a top-level filters field can appear.
|
|
||
| def _rebuild_viz_types() -> set[str]: | ||
| """Viz types eligible for form-data query-context rebuild (config or default).""" | ||
| return current_app.config.get("EXCEL_EXPORT_REBUILD_VIZ_TYPES") or REBUILD_VIZ_TYPES |
There was a problem hiding this comment.
Suggestion: The config fallback logic treats an explicitly configured empty set as falsy and silently re-enables the default rebuild allowlist. This makes it impossible to disable rebuilds via configuration. Check for None explicitly instead of using or so an empty set is respected. [logic error]
Severity Level: Major ⚠️
- Cannot fully disable query-context rebuild via config.
- Operators’ explicit empty allowlist is silently ignored.
- Charts still export with synthesized contexts against expectations.
- Potentially reintroduces silently incomplete or approximate results.
- Reduces trust in Excel export configuration controls.Steps of Reproduction ✅
1. In the Superset configuration file `superset/config.py`, set
`EXCEL_EXPORT_REBUILD_VIZ_TYPES = set()` (or an empty list) to explicitly disable
form-data-based query-context rebuilds for Excel export while keeping other defaults
unchanged; this value is read through `current_app.config` by `_rebuild_viz_types()` in
`superset/tasks/export_dashboard_excel.py:125-127`.
2. Start the Superset application and Celery workers so that the dashboard Excel export
task defined in `superset/tasks/export_dashboard_excel.py` is available and using the
configured `EXCEL_EXPORT_REBUILD_VIZ_TYPES`.
3. Export a dashboard containing a legacy chart (for example viz_type "table") with no
saved `query_context`; during `_build_workbook()` at
`superset/tasks/export_dashboard_excel.py:244-273`, this chart is processed on the data
path (not as an image) so `_resolve_query_context()` at line 130 is invoked.
4. Inside `_resolve_query_context()`, `_rebuild_viz_types()` is called at line 144;
because `_rebuild_viz_types()` currently returns
`current_app.config.get("EXCEL_EXPORT_REBUILD_VIZ_TYPES") or REBUILD_VIZ_TYPES` (line
127), the empty set from the config is treated as falsy and the function returns the
default `REBUILD_VIZ_TYPES` instead, so the chart’s viz_type (for example "table") is
still considered rebuild-eligible and its query context is synthesized from form data,
even though the operator explicitly configured an empty allowlist to disable rebuilds.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/tasks/export_dashboard_excel.py
**Line:** 127:127
**Comment:**
*Logic Error: The config fallback logic treats an explicitly configured empty set as falsy and silently re-enables the default rebuild allowlist. This makes it impossible to disable rebuilds via configuration. Check for `None` explicitly instead of using `or` so an empty set is respected.
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 fixThere was a problem hiding this comment.
Fixed in de7dfc1: _rebuild_viz_types now checks is None explicitly instead of or, so an operator can disable the rebuild with an explicit empty set instead of it silently falling back to the default allowlist. Added a parametrized unit test (None → default, empty set → disabled, override → honored).
Code Review Agent Run #54836dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #fe22c4Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
EnxDev's Review Agent — #42284 · HEAD de7dfc1request changes — the rebuilt query context silently drops the chart's time range, ordering and custom-SQL filters, so allowlisted charts export a different dataset than the chart shows — the exact outcome the PR promises never happens. The three earlier bot findings are genuinely fixed at this HEAD (empty- Note: the 🔴 Functional
🟡 Should-fix
🔵 Nits
🙌 Praise
Findings are code-verified against this HEAD, not runtime-verified (test suite not executed). |
|
Thanks — all findings addressed in 🔴 Functional
🟡 Should-fix
🔵 Nits — restructured the loop to drop the
|
| 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() |
There was a problem hiding this comment.
Suggestion: The function documentation claims columns are de-duplicated, but the implementation only prevents duplicate insertion for x_axis and does not de-duplicate existing groupby/columns entries. This contradiction can produce duplicate selected/grouped columns and inconsistent query output; either actually de-duplicate or correct the contract. [docstring mismatch]
Severity Level: Minor 🧹
- ⚠️ Docstring overstates de-duplication actually implemented.
- ⚠️ Duplicate columns unlikely from normal frontend form_data.Steps of Reproduction ✅
1. Trigger a dashboard Excel export so `_build_workbook`
(`superset/tasks/export_dashboard_excel.py:255`) runs and iterates charts from
`get_charts_in_layout_order`.
2. For a chart without saved `query_context` but with saved `params` (`chart.params`),
`_build_workbook` calls `_resolve_query_context`
(`superset/tasks/export_dashboard_excel.py:137`), which parses `chart.params` and invokes
`build_query_context_from_form_data` in `superset/common/form_data_query_context.py:162`.
3. `build_query_context_from_form_data` calls `columns_from_form_data`
(`superset/common/form_data_query_context.py:94`), which at lines 107–112 simply copies
`form_data["columns"]` or `form_data["groupby"]` into `columns` without de-duplicating
within those lists; it only avoids inserting a duplicate `x_axis` later in the function.
4. In practice, frontend-generated `form_data` for charts does not contain duplicate
entries in `columns` or `groupby`, and there is no backend code that injects duplicates,
so any duplication would require malformed or manually edited `params`; the mismatch is
between the docstring (“de-duplicating while preserving order”) and implementation rather
than a reproducible bug in normal usage.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/form_data_query_context.py
**Line:** 107:112
**Comment:**
*Docstring Mismatch: The function documentation claims columns are de-duplicated, but the implementation only prevents duplicate insertion for `x_axis` and does not de-duplicate existing `groupby`/`columns` entries. This contradiction can produce duplicate selected/grouped columns and inconsistent query output; either actually de-duplicate or correct the contract.
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| ineligible = _chart(20, "Ineligible", viz_type="mixed_timeseries") | ||
| ineligible.query_context = None | ||
| ineligible.params = json.dumps({"groupby": ["x"], "metrics": ["count"]}) | ||
| ineligible.datasource_id = 5 |
There was a problem hiding this comment.
Suggestion: This test is intended to prove skip behavior is driven by an ineligible viz type, but it never sets datasource_type on the chart. If rebuild preconditions require a complete datasource, the chart can be skipped for missing datasource metadata instead, so the test can pass even when allowlist gating is broken. Set a valid datasource type so the only reason to skip is viz ineligibility. [logic error]
Severity Level: Major ⚠️
- ⚠️ Ineligible viz allowlist gating may remain untested.
- ⚠️ Export skip reason obscured by missing datasource_type.
- ⚠️ Tests may pass while rebuild allowlist misconfigured.Steps of Reproduction ✅
1. Run `pytest tests/unit_tests/tasks/test_export_dashboard_excel.py` and focus on
`test_empty_query_context_ineligible_viz_is_skipped` at lines 241–263 in
`tests/unit_tests/tasks/test_export_dashboard_excel.py`.
2. Inside this test, the ineligible chart is set up at lines 247–250: `ineligible =
_chart(20, "Ineligible", viz_type="mixed_timeseries")`, `ineligible.query_context = None`,
`ineligible.params = json.dumps({"groupby": ["x"], "metrics": ["count"]})`, and
`ineligible.datasource_id = 5`; note that `ineligible.datasource_type` is never set.
3. Compare this setup with the rebuild tests for eligible viz types:
`test_empty_query_context_rebuilt_from_form_data_for_eligible_viz` (lines 217–239) and
`test_eligible_viz_skipped_when_form_data_unusable` (lines 273–295), where charts
explicitly set both `datasource_id` and `datasource_type = "table"` before invoking
`_run()`. This indicates the production rebuild logic in
`superset.tasks.export_dashboard_excel` requires complete datasource metadata (both id and
type) as a precondition.
4. If the allowlist gating in `superset.tasks.export_dashboard_excel._rebuild_viz_types()`
(tested at lines 344–360) is accidentally broadened to include `"mixed_timeseries"`, the
ineligible chart in `test_empty_query_context_ineligible_viz_is_skipped` can still be
skipped solely because `datasource_type` is missing. The test will pass, but it will be
exercising the “missing datasource metadata” path instead of the intended “viz type
outside allowlist” path, so a bug in allowlist gating goes undetected.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/tasks/test_export_dashboard_excel.py
**Line:** 247:250
**Comment:**
*Logic Error: This test is intended to prove skip behavior is driven by an ineligible viz type, but it never sets `datasource_type` on the chart. If rebuild preconditions require a complete datasource, the chart can be skipped for missing datasource metadata instead, so the test can pass even when allowlist gating is broken. Set a valid datasource type so the only reason to skip is viz ineligibility.
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
Code Review Agent Run #f00e56Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| if form_data.get("time_grain_sqla"): | ||
| extras["time_grain_sqla"] = form_data["time_grain_sqla"] | ||
|
|
||
| time_range = form_data.get("time_range") or "No filter" |
There was a problem hiding this comment.
The rebuild only reads time_range, but older charts may still use since/until. In that case, the export falls back to "No filter" and could export the full history instead of the chart's configured time range.
Should we fall back to since/until when time_range is missing? It would also be worth adding a regression test for this case.
There was a problem hiding this comment.
Fixed in 53350ae — when time_range is absent the rebuild now falls back to since/until ("{since} : {until}") before defaulting to "No filter", so older charts export their configured range. Added a regression test.
| def adhoc_filters_to_query_filters( | ||
| adhoc_filters: list[dict[str, Any]], | ||
| ) -> 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}``. Only ``SIMPLE`` WHERE-clause filters are | ||
| convertible here; free-form ``SQL`` filters have no ``{col, op, val}`` | ||
| equivalent and are handled separately (see :func:`freeform_where_having`). | ||
| """ | ||
| result: list[dict[str, Any]] = [] | ||
| for flt in adhoc_filters or []: | ||
| if ( | ||
| flt.get("expressionType") == "SIMPLE" | ||
| and (flt.get("clause") or "WHERE").upper() == "WHERE" | ||
| ): | ||
| result.append( | ||
| { | ||
| "col": flt.get("subject"), | ||
| "op": flt.get("operator"), | ||
| "val": flt.get("comparator"), | ||
| } | ||
| ) | ||
| return result |
There was a problem hiding this comment.
adhoc_filters_to_query_filters now only converts SIMPLE filters with a WHERE clause, while the previous chart_utils implementation converted all SIMPLE filters. This means SIMPLE HAVING filters previously handled by mcp_service/chart/compile.py and preview_utils.py are now silently dropped.
Should we preserve the previous behavior here and handle both WHERE and HAVING filters?
There was a problem hiding this comment.
Reverted in 53350ae — adhoc_filters_to_query_filters again converts all SIMPLE filters regardless of clause, so SIMPLE HAVING filters the MCP compile/preview path relied on are no longer dropped. Added a test pinning that a SIMPLE HAVING filter still converts. (Note: the frontend processFilters does drop SIMPLE-HAVING, but preserving the prior shared behavior here is the safer choice and keeps MCP unchanged.)
| 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 |
There was a problem hiding this comment.
columns_from_form_data now checks the truthiness of raw_columns instead of whether "columns" exists in form_data. As a result, columns: [] no longer takes precedence over groupby in the MCP preview/compile path.
This looks like an improvement, but it changes the previous behavior. Could we confirm this is intentional and add an MCP-path test to cover it?
There was a problem hiding this comment.
Confirmed intentional — this fixes a real bug (an earlier reviewer flagged that a stale, present-but-empty columns: [] was silently dropping groupby and changing the aggregation). Now that preview_utils._build_query_columns delegates to the shared columns_from_form_data, the MCP compile/preview path gets the same fix. Added an MCP-path test (test_build_query_columns_empty_columns_key_keeps_groupby) calling preview_utils._build_query_columns({"groupby": ["country"], "columns": []}) and asserting ["country"].
| # Table percent metrics are computed as additional query metrics. | ||
| if viz_type == "table" and form_data.get("percent_metrics"): | ||
| metrics = [*metrics, *form_data["percent_metrics"]] |
There was a problem hiding this comment.
For table charts, percent_metrics are added as regular query metrics without applying the percentage/contribution post-processing.
This means the exported column contains the raw aggregate instead of the "% of total" shown in the chart.
Should we exclude percent_metrics from the rebuild to avoid exporting values that don't match what the user sees in the chart?
There was a problem hiding this comment.
Good call — excluded in 53350ae. Since the rebuild doesn't apply the contribution/percent post-processing, carrying percent_metrics would export raw aggregates under a column the user expects to be a "% of total", i.e. silently-wrong values. Omitting them is the safer choice (consistent with the rest of this rebuild); time_grain_sqla is still carried. Test updated to assert they're excluded.
| if flt.get("expressionType") == "SIMPLE": | ||
| result.append( | ||
| { | ||
| "col": flt.get("subject"), | ||
| "op": flt.get("operator"), | ||
| "val": flt.get("comparator"), | ||
| } |
There was a problem hiding this comment.
Suggestion: A SIMPLE adhoc filter marked with clause HAVING is always appended to filters, which applies it before aggregation as a WHERE predicate. Aggregate conditions such as COUNT(*) > 5 will therefore produce incorrect results or fail in the database. Route HAVING filters to the query's having expression instead of treating every SIMPLE filter as a row-level filter. [logic error]
Severity Level: Major ⚠️
- ❌ Aggregate conditions are applied before chart aggregation.
- ❌ Legacy chart Excel sheets can contain incorrect totals.
- ⚠️ Databases may reject aggregate predicates in WHERE.Steps of Reproduction ✅
1. Create or use a legacy chart whose saved `params` contain a `SIMPLE` adhoc filter with
`clause: "HAVING"` and an aggregate condition, such as a metric count greater than five.
2. Export a dashboard containing that chart through the Excel export task in
`superset/tasks/export_dashboard_excel.py`; `_build_workbook()` at lines 272-291 resolves
the missing context and invokes `_write_chart_sheets()`.
3. `_resolve_query_context()` at lines 148-164 calls
`build_query_context_from_form_data()`, which calls `adhoc_filters_to_query_filters()` at
line 192.
4. `adhoc_filters_to_query_filters()` at lines 55-61 converts the filter into the query's
`filters` list without inspecting its HAVING clause, while `freeform_where_having()` at
lines 79-88 only handles SQL filters. The generated query therefore applies the aggregate
predicate as a row-level WHERE filter, producing incorrect results or a database
validation error.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/form_data_query_context.py
**Line:** 55:61
**Comment:**
*Logic Error: A SIMPLE adhoc filter marked with clause HAVING is always appended to `filters`, which applies it before aggregation as a WHERE predicate. Aggregate conditions such as `COUNT(*) > 5` will therefore produce incorrect results or fail in the database. Route HAVING filters to the query's having expression instead of treating every SIMPLE filter as a row-level filter.
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| if not isinstance(parsed, dict) or not parsed.get("queries"): | ||
| return None | ||
| return parsed |
There was a problem hiding this comment.
Suggestion: _saved_query_context only checks that queries is truthy, so malformed contexts such as {"queries": "invalid"} or other non-list values are accepted and passed to ChartDataCommand. This causes validation or iteration failures to be reported as a generic chart export error instead of being classified as missing query context and listed for remediation. Require queries to be a non-empty list with the expected query-object structure before returning the parsed context. [possible bug]
Severity Level: Major ⚠️
- ❌ Affected chart data sheets are omitted from exports.
- ⚠️ Users receive generic errors instead of remediation guidance.
- ⚠️ Malformed saved contexts bypass missing-context handling.Steps of Reproduction ✅
1. Persist a chart with `query_context` containing valid JSON such as `{"queries":
"invalid"}`; this is accepted by `json.loads()` at line 119 and produces a truthy
`queries` value.
2. Include that chart in a data-mode dashboard Excel export; `_build_workbook()` at lines
272-291 calls `_resolve_query_context()` for the chart.
3. `_saved_query_context()` at lines 122-124 returns the parsed dictionary because it
checks only that `queries` is truthy, not that it is a non-empty list of query objects.
4. `_write_chart_sheets()` receives the malformed payload at line 291 and passes it into
the downstream chart-data execution path, where schema validation or query iteration
fails. The exception is handled as a generic chart export failure instead of being
classified under `email.ERROR_NO_QUERY_CONTEXT` for re-saving.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/tasks/export_dashboard_excel.py
**Line:** 122:124
**Comment:**
*Possible Bug: `_saved_query_context` only checks that `queries` is truthy, so malformed contexts such as `{"queries": "invalid"}` or other non-list values are accepted and passed to `ChartDataCommand`. This causes validation or iteration failures to be reported as a generic chart export error instead of being classified as missing query context and listed for remediation. Require `queries` to be a non-empty list with the expected query-object structure before returning the parsed context.
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
| # Conservative by default: only charts whose data maps faithfully to a single | ||
| # plain query (no post-processing, no multi-query fan-out). Operators can | ||
| # override via ``EXCEL_EXPORT_REBUILD_VIZ_TYPES``. | ||
| REBUILD_VIZ_TYPES = {"table", "big_number_total", "big_number", "pie"} |
There was a problem hiding this comment.
REBUILD_VIZ_TYPES only checks the viz type, but some behaviors that require post-processing or multiple queries depend on the form data.
For example, a table with time_compare requires additional queries/post-processing, and a Big Number with rolling/resampling can also produce values that differ from the raw query result. Rebuilding these directly could therefore export incomplete or incorrect data.
Should we also skip the rebuild when rolling_type, resample_rule, time_compare, or aggregation === "raw" is present?
It would be good to add a regression test for at least the time_compare case.
There was a problem hiding this comment.
Good point — the viz-type allowlist isn't sufficient on its own. Fixed in 808ba6f: _resolve_query_context now also skips the rebuild (→ re-save list) when the form data uses processing the single query can't reproduce — time_compare, rolling_type (ignoring the literal "None"), resample_rule, or aggregation == "raw". Added a parametrized regression test covering all four (including time_compare).
| granularity = form_data.get("granularity") or form_data.get("granularity_sqla") | ||
| if granularity and time_range != "No filter": | ||
| query["granularity"] = granularity | ||
| if form_data.get("row_limit"): | ||
| query["row_limit"] = form_data["row_limit"] |
There was a problem hiding this comment.
For Big Number trendlines, granularity is only set when time_range != "No filter". However, time_grain_sqla is only applied to the column identified by granularity.
This means a Big Number with no time filter can export rows at the raw timestamp precision instead of the configured time grain.
Should we always set granularity to the promoted time column for Big Number trendlines, regardless of time_range?
It would also be good to add a regression test for a Big Number with time_grain_sqla and no time_range.
There was a problem hiding this comment.
Fixed in 808ba6f. When a Big Number trendline promotes its granularity_sqla to the grouping column, the rebuild now also sets granularity regardless of time_range, so time_grain_sqla buckets that column (no more raw-timestamp precision). The time_range != "No filter" gate still applies to the non-promoted cases, so a non-temporal granularity_sqla with no active range isn't forced through date bucketing. Added a regression test (Big Number with time_grain_sqla and no time_range).
| """ | ||
| result: list[dict[str, Any]] = [] | ||
| for flt in adhoc_filters or []: | ||
| if flt.get("expressionType") == "SIMPLE": |
There was a problem hiding this comment.
Restoring support for all SIMPLE filters makes sense for MCP, but it creates a difference between the chart and the export. The chart only applies SIMPLE filters with clause === "WHERE", while the export would also apply HAVING filters, potentially resulting in fewer rows than what the user sees.
Should we filter out HAVING filters in build_query_context_from_form_data while keeping the shared helper unchanged for MCP?
There was a problem hiding this comment.
Done in 808ba6f — exactly your suggestion. Added a where_only arg to the shared adhoc_filters_to_query_filters: it defaults to converting all SIMPLE filters (unchanged for MCP), and build_query_context_from_form_data calls it with where_only=True so the export applies only WHERE-clause SIMPLE filters, matching the chart. Added tests for both the helper (default vs where_only) and the builder (SIMPLE HAVING excluded).
Code Review Agent Run #0a2f4bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #c07fbdActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| :param viz_type: The chart's viz type, used for viz-specific handling. | ||
| :returns: A single-query query-context dict. | ||
| """ | ||
| metrics = list(form_data.get("metrics") or []) |
There was a problem hiding this comment.
In raw-mode table charts, saved form data can still contain stale metrics/groupby values because those controls aren't reset when hidden. The frontend ignores them in raw mode, but the rebuild currently doesn't, so the exported query can differ from what the chart actually runs.
For example, this can add aggregate columns/grouping and even introduce an orderby based on a stale metric, potentially exporting different rows from those shown in the chart.
Could we mirror the frontend behavior here by dropping metrics/groupby in raw mode and handling all_columns consistently with getQueryMode? A regression test using Publishers_With_Most_Titles would cover this well.
There was a problem hiding this comment.
Fixed in 2565811. The rebuild now mirrors getQueryMode (a new is_raw_query_mode: explicit query_mode, else all_columns presence) and in raw mode uses only the selected columns while ignoring the stale metrics/groupby — so no accidental aggregation, grouping, or metric-based orderby. Added regression tests, including one for the Publishers With Most Titles-style raw table (all_columns + stale metrics/groupby → columns only, metrics=[], orderby=[]).
| if not metrics: | ||
| return [] | ||
|
|
||
| order_desc = form_data.get("order_desc", True) |
There was a problem hiding this comment.
order_desc defaults to True here, while the table plugin defaults it to false.
This means a table with timeseries_limit_metric and no saved order_desc can sort ascending in the chart but descending in the export; potentially changing bottom-N into top-N when a row limit is applied.
We can't change the default globally because Pie always sorts by metric descending.
Could we make this viz-specific: default to ascending for Table and descending for Pie?
A regression test for a Table with timeseries_limit_metric and no order_desc would also be useful.
There was a problem hiding this comment.
Good catch. Made it viz-specific in 2565811: orderby_from_form_data now defaults order_desc to False (ascending) for Table and True (descending) for Pie/others, matching the plugins — so a table with timeseries_limit_metric and no saved order_desc sorts ascending in the export too, keeping bottom-N under a row limit. Added a regression test (Table + timeseries_limit_metric, no order_desc → [[metric, True]]).
| # ``percent_metrics`` are intentionally not carried: the chart shows them as a | ||
| # "% of total" produced by contribution post-processing, which this rebuild | ||
| # does not apply, so adding them as plain metrics would export raw aggregates | ||
| # that don't match the chart. |
There was a problem hiding this comment.
Dropping percent_metrics means the export is missing percentage columns that are visible in the chart.
This affects both Table and Pie charts, where contribution post-processing adds these values.
Since the goal is to avoid silently exporting incomplete data, should we treat a non-empty percent_metrics like the other _UNSUPPORTED_PROCESSING_KEYS and require the chart to be re-saved?
There was a problem hiding this comment.
Agreed — silently omitting visible columns is worse than skipping. In 2565811 a non-empty percent_metrics is now treated like the other _UNSUPPORTED_PROCESSING_KEYS: the chart is skipped and listed for re-save rather than rebuilt. Added it to the parametrized skip test.
| 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"]) |
There was a problem hiding this comment.
Suggestion: Unlike the frontend filter processor, this path does not append a newline when a free-form SQL clause contains --. The generated wrapper can therefore turn the closing parenthesis and following SQL into a comment, causing a syntax error or changing the predicate semantics during export. Apply the same clause sanitization as processFilters before placing expressions in extras. [logic error]
Severity Level: Major ⚠️
- ❌ SQL-filtered Excel exports can fail query execution.
- ⚠️ Export predicates can differ from displayed chart results.
- ⚠️ Affected charts are omitted or reported as export errors.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/form_data_query_context.py
**Line:** 86:91
**Comment:**
*Logic Error: Unlike the frontend filter processor, this path does not append a newline when a free-form SQL clause contains `--`. The generated wrapper can therefore turn the closing parenthesis and following SQL into a comment, causing a syntax error or changing the predicate semantics during export. Apply the same clause sanitization as `processFilters` before placing expressions in `extras`.
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| 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) |
There was a problem hiding this comment.
Suggestion: An adhoc x-axis object representing a calculated column is reduced to only column_name, discarding its sqlExpression and other expression metadata. The rebuilt query then references the physical column name instead of the calculated expression, so legacy charts using SQL or calculated x-axis columns can fail or return different data. Preserve the full adhoc column definition, or explicitly reject this form when the builder cannot support it. [type error]
Severity Level: Major ⚠️
- ❌ Calculated-axis chart exports can omit their grouping.
- ⚠️ Exported results can differ from chart visualization.
- ⚠️ SQL-expression axes may cause query failures.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/form_data_query_context.py
**Line:** 121:127
**Comment:**
*Type Error: An adhoc x-axis object representing a calculated column is reduced to only `column_name`, discarding its `sqlExpression` and other expression metadata. The rebuilt query then references the physical column name instead of the calculated expression, so legacy charts using SQL or calculated x-axis columns can fail or return different data. Preserve the full adhoc column definition, or explicitly reject this form when the builder cannot support it.
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| if order_by_cols := form_data.get("order_by_cols") or []: | ||
| parsed: list[list[Any]] = [] | ||
| for col in order_by_cols: | ||
| if isinstance(col, str): | ||
| try: | ||
| col = json.loads(col) | ||
| except (TypeError, ValueError): | ||
| continue | ||
| parsed.append(col) | ||
| return parsed |
There was a problem hiding this comment.
Suggestion: Successfully parsed order_by_cols values are appended without validating that they are two-element [column, ascending] pairs. Values such as null, a scalar, a dictionary, or a one-element list reach ChartDataQueryContextSchema, which expects tuple pairs and rejects the entire chart export instead of treating the malformed legacy ordering as absent. Validate the shape before appending it. [type error]
Severity Level: Major ⚠️
- ❌ Malformed ordering can abort chart Excel export.
- ⚠️ One legacy chart can be skipped from the workbook.
- ⚠️ Recoverable form-data corruption becomes a user-visible error.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/form_data_query_context.py
**Line:** 155:164
**Comment:**
*Type Error: Successfully parsed `order_by_cols` values are appended without validating that they are two-element `[column, ascending]` pairs. Values such as `null`, a scalar, a dictionary, or a one-element list reach `ChartDataQueryContextSchema`, which expects tuple pairs and rejects the entire chart export instead of treating the malformed legacy ordering as absent. Validate the shape before appending it.
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| for flt in form_data.get("filters") or []: | ||
| if isinstance(flt, dict) and flt.get("col") is not None: | ||
| filters.append(flt) |
There was a problem hiding this comment.
Suggestion: Legacy filter entries are accepted whenever they contain a col, even if they lack the required op or val fields. Such malformed entries later reach query-context processing, which indexes the missing filter fields and can raise an exception for the whole chart. Require a complete query-filter shape before appending legacy filters. [error handling]
Severity Level: Major ⚠️
- ❌ Corrupt legacy filters can fail chart export.
- ⚠️ Affected charts may be omitted from workbooks.
- ⚠️ Malformed saved form data reaches query processing.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/form_data_query_context.py
**Line:** 234:236
**Comment:**
*Error Handling: Legacy filter entries are accepted whenever they contain a `col`, even if they lack the required `op` or `val` fields. Such malformed entries later reach query-context processing, which indexes the missing filter fields and can raise an exception for the whole chart. Require a complete query-filter shape before appending legacy filters.
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| query: dict[str, Any] = { | ||
| "columns": columns, | ||
| "metrics": metrics, | ||
| "orderby": orderby_from_form_data(form_data, metrics, viz_type), | ||
| "filters": filters, | ||
| "time_range": time_range, | ||
| } |
There was a problem hiding this comment.
Suggestion: The generic rebuild omits table and other chart controls such as series_limit and series_limit_metric. A chart with a series limit can therefore return arbitrary groups up to row_limit instead of the configured top series, producing materially different export data while still being treated as a successful rebuild. These controls must be translated into the query context or such charts must be skipped. [api mismatch]
Severity Level: Critical 🚨
- ❌ Allowlisted chart exports can contain incorrect groups.
- ⚠️ Top-series and row-limit behavior diverges from charts.
- ⚠️ Exported Excel data may not match displayed chart data.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/common/form_data_query_context.py
**Line:** 248:254
**Comment:**
*Api Mismatch: The generic rebuild omits table and other chart controls such as `series_limit` and `series_limit_metric`. A chart with a series limit can therefore return arbitrary groups up to `row_limit` instead of the configured top series, producing materially different export data while still being treated as a successful rebuild. These controls must be translated into the query context or such charts must be skipped.
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| if chart.viz_type not in _rebuild_viz_types() or chart.datasource_id is None: | ||
| return None | ||
| try: | ||
| form_data = json.loads(chart.params) if chart.params else {} | ||
| except (TypeError, ValueError): | ||
| return None | ||
| if not isinstance(form_data, dict) or not form_data: | ||
| return None | ||
| if _needs_unsupported_processing(form_data): | ||
| return None |
There was a problem hiding this comment.
Suggestion: The default rebuild allowlist permits table charts, but the synthesized context does not reproduce table-specific extra queries such as show_totals or percent-metric calculations. An older allowlisted table using these options will export only the main query and silently omit the totals or derived data shown by the chart. Either detect these settings in _needs_unsupported_processing or include the required queries and post-processing before allowing the rebuild. [incomplete implementation]
Severity Level: Critical 🚨
- ❌ Table Excel exports can omit configured totals rows.
- ⚠️ Exported table data differs from displayed chart output.
- ⚠️ Legacy charts are marked successful despite incomplete results.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/tasks/export_dashboard_excel.py
**Line:** 174:183
**Comment:**
*Incomplete Implementation: The default rebuild allowlist permits table charts, but the synthesized context does not reproduce table-specific extra queries such as `show_totals` or percent-metric calculations. An older allowlisted table using these options will export only the main query and silently omit the totals or derived data shown by the chart. Either detect these settings in `_needs_unsupported_processing` or include the required queries and post-processing before allowing the rebuild.
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
| if order_by_cols := form_data.get("order_by_cols") or []: | ||
| parsed: list[list[Any]] = [] | ||
| for col in order_by_cols: | ||
| if isinstance(col, str): | ||
| try: | ||
| col = json.loads(col) |
There was a problem hiding this comment.
orderby_from_form_data checks order_by_cols in every mode, but this is a raw-mode-only control.
Since its value isn't reset when switching to aggregate mode, a stale order_by_cols can cause the export to use a different ordering from the chart, potentially returning a different top-N.
Could we only use order_by_cols when is_raw_query_mode(form_data) is true? It would also be good to add a regression test for an aggregate table with a stale order_by_cols value.
Code Review Agent Run #f2291fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Address review of the empty query-context rebuild:
- columns_from_form_data no longer lets a stale, explicitly-present-but-
empty `columns: []` key shadow the group-by dimensions (which silently
dropped grouping and changed the aggregation). Prefer raw columns only
when non-empty, else fall back to groupby.
- build_query_context_from_form_data now also honors legacy simple
`filters` (already in QueryObject {col, op, val} shape) in addition to
`adhoc_filters`, so legacy charts export the same filtered data they
show; malformed entries are dropped.
- _rebuild_viz_types checks the config for None explicitly instead of
using `or`, so an operator can disable the rebuild with an empty set
instead of it silently falling back to the default allowlist.
Adds unit tests for each case.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address EnxDev review — the generic rebuild silently dropped several query aspects, so allowlisted charts could export a different dataset than they show: - Ordering: derive `orderby` from order_by_cols (raw) or the sort metric / first-metric-descending (aggregate), so a `row_limit` returns the chart's top-N instead of an arbitrary N. - Time range: set `granularity` (from granularity/granularity_sqla) so `time_range` is actually applied — but only when there is an active range, so a numeric column saved as granularity_sqla with no range isn't forced through date bucketing (verified against a real export). - Custom SQL filters: map `SQL` adhoc filters + legacy top-level `where` into `extras.where`/`extras.having` by clause instead of dropping them. - Table specifics: carry `percent_metrics` into metrics and pass `time_grain_sqla` through `extras`. - Big Number: only promote granularity_sqla to a grouping column for the trendline viz (`big_number`), never `big_number_total`. Also point the MCP chart compile/preview helpers (adhoc_filters_to_query_filters, _build_query_columns) at this shared module so they stop diverging (preview_utils still had the pre-fix `columns` bug), and address nits: drop the `-O`-stripped assert by restructuring the loop, parse the saved query_context once, and correct the shallow-copy comment. Adds unit tests for every derived field plus an export test asserting the full query body reaches ChartDataQueryContextSchema().load. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Exclude percent_metrics from the rebuild: the chart shows them as a "% of total" via contribution post-processing the rebuild can't apply, so adding them as plain metrics would export raw aggregates that don't match the chart. - Restore converting all SIMPLE adhoc filters (not just WHERE-clause) so SIMPLE HAVING filters the MCP compile/preview path relied on are not silently dropped. - Fall back to legacy since/until when time_range is absent, so older charts export their configured range instead of the full history. - Add an MCP-path test pinning that an explicitly empty `columns: []` no longer shadows `groupby` (intentional behavior change), plus tests for the since/until fallback and SIMPLE HAVING conversion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…atch Address further review of the form-data query-context rebuild: - Skip the rebuild when the form data relies on processing the single query can't reproduce: time_compare, rolling_type, resample_rule, or aggregation=raw. Even allowlisted viz types would otherwise export values that differ from the chart. - Big Number trendline: set granularity (so time_grain_sqla buckets the promoted time column) even with no active time_range. - Export applies only WHERE-clause SIMPLE filters (via a new where_only arg), matching the chart, while the shared helper still converts all SIMPLE filters for the MCP path. Adds regression tests for each case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…metrics Further review of the form-data query-context rebuild: - Raw-mode tables: ignore stale metrics/groupby (their controls aren't reset when hidden) and use only the selected columns, mirroring the frontend's getQueryMode (explicit query_mode, else all_columns). - Ordering: default order_desc to False (ascending) for Table sort metrics and True for Pie, matching the plugins — so a row limit keeps the chart's top/bottom-N instead of flipping it. - percent_metrics: skip the chart (re-save) rather than silently omitting the "% of total" columns the user sees, alongside the other unsupported-processing markers. Adds regression tests for each; extracts a helper to keep the builder under the complexity limit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
order_by_cols is a raw-mode-only control (resetOnHide: false), so an aggregate chart can carry a stale value. The form-data query-context rebuild read it in every mode, which could order an aggregate export by stale columns and return a different top-N than the chart shows. Gate it behind is_raw_query_mode so aggregate mode falls back to the metric-based ordering, mirroring the frontend Table buildQuery. Also add frontend-drift pointers naming the mirrored buildQuery / extractQueryFields / processFilters sources, and a regression test for an aggregate table with a stale order_by_cols. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the EXCEL_EXPORT_REBUILD_VIZ_TYPES config override and use the REBUILD_VIZ_TYPES constant directly. The allow-list bounds which legacy charts (no saved query_context) get a form-data-rebuilt context in the Excel export; it's correctness-critical and conservative by design, and the config override was speculative flexibility nobody needs. Everything else without a saved context is still skipped and listed for re-save. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…port Add EXCEL_EXPORT_QUERY_CONTEXT_BUILDER, an optional hook tried before the built-in form-data rebuild when a chart has no saved query_context. It receives the chart's form data and returns a query-context payload (or None), letting a deployment supply a faithful context — e.g. from a service running the chart's real frontend buildQuery — for viz types the built-in rebuild can't reproduce (pivot, timeseries, multi-query). The hook must return None when it can't build faithfully, so an allowlisted table stays on the tested built-in path until a faithful builder is available. The call is guarded (any failure falls through to the rebuild) and its result is shape-validated via the extracted _usable_query_context helper, preserving "builder problem → rebuild, don't fail the export". The allowlist and unsupported-processing checks now bound only the built-in rebuild; the hook is intentionally not gated by them. Default None keeps OSS behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… rebuild
The form-data query-context rebuild is a hand-port of frontend `buildQuery`
logic, and had drifted from it in four ways that make a rebuilt sheet differ
from what the chart renders:
- Free-form SQL filters were wrapped as `({clause})` with no newline. A
predicate ending in a `--` comment commented out the closing paren and every
predicate joined after it, failing the export on a chart that renders fine.
Ports `sanitizeClause` from `processFilters.ts`.
- `timeseries_limit_metric` was read raw. The drag-and-drop sort-by control
persists a list, which nested inside `orderby` and failed the query; now
unwrapped with `as_list(...)[0]`, mirroring `ensureIsArray(...)[0]`.
- Pie's `contribution` operator is attached unconditionally by
`Pie/buildQuery.ts`, so a rebuilt pie lost the percentage column a
saved-context pie carries. The rebuild now applies it.
- `show_totals` pushes a second totals query in aggregate mode; the
single-query rebuild dropped that row silently. Such tables are now skipped
and listed for re-save, like `percent_metrics` already were.
Also hardens the builder hook added in 9e2b5b4: its payload is deep-copied
before `apply_dashboard_filter_context` mutates `queries[*]` in place (a
memoizing builder would otherwise accumulate `isExtra` filters across charts
and exports), and `_usable_query_context` now type-checks `queries` as a list
so a malformed return falls through to the built-in rebuild instead of failing
later. `order_by_cols` entries that parse but aren't `[col, asc]` pairs are
dropped rather than appended to `orderby`.
Docs: `EXCEL_EXPORT_REBUILD_VIZ_TYPES` never existed — cf071d5 made the
allowlist a fixed constant — so UPDATING.md now documents the key that does
exist, `EXCEL_EXPORT_QUERY_CONTEXT_BUILDER`, and the .mdx no longer claims every
context-less chart is skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…value
The isinstance check added in b435c47b lives in _usable_query_context, which
_saved_query_context delegates to, so a chart whose saved query_context is
`{"queries": "oops"}` now takes the clean "no query context" path instead of
failing later in the general error bucket. Pins that from the saved-context
side, where the behavior is user-visible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
986f650 to
7896b4b
Compare
Code Review Agent Run #af1ee0Actionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
The guard around EXCEL_EXPORT_QUERY_CONTEXT_BUILDER caught bare Exception, and Celery's SoftTimeLimitExceeded subclasses it — so a soft timeout that fired while the hook was in flight (a slow or hanging builder being the realistic case) was logged as a builder failure and swallowed, and the export carried on into the rebuild and the next chart. Re-raise SoftTimeLimitExceeded before the broad guard so it reaches _build_workbook, which already re-raises it deliberately: a soft timeout is a task-level signal, not a per-chart failure. Every other hook failure still falls through to the built-in rebuild. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review Agent Run #f38997Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks for this — tested it live end-to-end (real Celery worker, MailHog, MinIO-backed S3) rather than just reading the diff, and found one correctness gap worth fixing before merge. The Repro (verified against a real dataset with per-day granularity, not one that happens to already be pre-aggregated to the grain being tested):
Root cause: The I've added a regression test that reproduces this (currently failing against HEAD): Everything else I exercised — happy-path rebuilds for all four allowlisted viz types, the |
… with time_range="No filter" A table/pie chart grouped by its own time column, with a time grain set but time_range="No filter" (a very ordinary "all-time totals by month/year" configuration), currently loses time-bucketing: granularity is only set when time_range != "No filter" (or for the big_number trendline's promoted time column), so the column is selected raw instead of truncated to its grain. Verified live against a real dataset with per-day granularity: the same chart returns 252 raw-timestamp rows with time_range="No filter" vs. 3 correctly bucketed yearly rows with an explicit time_range. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…a time range `granularity` does two unrelated jobs downstream: it names the temporal column the time range filters on, and it is the column `time_grain_sqla` buckets (`models/helpers.py` swaps a selected column for its timestamp expression when that column equals `granularity`). The rebuild gated it on `time_range != "No filter"`, which is right for the first job and wrong for the second. So a table or pie grouped by its own time column with a grain set but no active range — plain "all-time totals by month" — exported one row per raw timestamp instead of one per month. Set `granularity` whenever form data carries one, matching the frontend's `extractExtras.ts`, which sets it unconditionally. That also makes the Big Number trendline's `promoted_time_column` flag dead, so it's removed. Fixes the regression test added in f7a9dbf. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Verified the fix in f586967 live end-to-end (same real Celery worker / MailHog / MinIO setup as before) — the exact scenario that was broken now works correctly:
Looks good from a correctness standpoint. Going to do a full code review pass on the current state next (a lot has landed since my first read — the config hook, Pie contribution post-processing, the raw-mode/order-by hardening — worth a fresh look rather than assuming my earlier review still covers it). |
eschutho
left a comment
eschutho
left a comment
There was a problem hiding this comment.
Approving. Summary of what was checked (full details in my earlier comments):
- Live end-to-end verification with a real Celery worker, MailHog, and a MinIO-backed S3 endpoint — happy paths for all four allowlisted viz types, the
_needs_unsupported_processingguard, malformed-query_contextrecovery, the in-flight export lock, and the images-mode feature-flag gate all behave correctly. - Found and reported a real bug (granularity dropped for a
table/piechart grouped by its own time column withtime_range: "No filter") — fixed in f586967 and reverified live for both viz types plus the full regression matrix. - Reviewed the current diff, including the
EXCEL_EXPORT_QUERY_CONTEXT_BUILDERhook, Pie's contribution post-processing, and theorder_by_cols/timeseries_limit_metrichardening — no outstanding correctness issues. Test coverage is thorough (85/85 passing, including dedicated builder-hook and mutation-safety tests). Ruff clean.
Thanks for iterating on this — nice fix, and appreciated the design hook for a real-buildQuery backend.
|
Bito Automatic Review Skipped – PR Already Merged |
4 participants
SUMMARY
Follow-up to the async dashboard "Export Data to Excel" feature to properly handle charts that have no saved
query_context. A chart persists itsquery_contextonly once it has been (re-)saved in Explore, so older charts haveparams(form data) but no context and previously failed mid-export or fell into the generic error bucket. This PR first makes such charts skip cleanly and get listed under the "no query context" remediation (coveringnull/{}/{"queries": []}/malformed values, not just a blank one), then goes further and synthesizes a query context from the chart's saved form data so those charts still export. Because the rebuild is a generic single-query mapping that does not reproduce plugin post-processing (pivot, rolling, forecast) or multi-query charts, it is gated behind a conservative, configurable viz-type allowlist (EXCEL_EXPORT_REBUILD_VIZ_TYPES, defaulttable/big_number_total/big_number/pie); anything else without a saved context is still skipped and listed for re-save, so no chart ever exports silently wrong or incomplete data.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend export-task behavior only.
TESTING INSTRUCTIONS
Run the unit tests:
pytest tests/unit_tests/tasks/test_export_dashboard_excel.py tests/unit_tests/common/test_form_data_query_context.py. They cover the clean-skip cases (blank/null/{}/empty-queries/malformed), the form-data rebuild for an eligible viz type, and the skip-with-notice fallback for an ineligible (multi-query) viz type. End-to-end: export a dashboard containing a legacy chart with no savedquery_context— an eligible chart (e.g. a table) now appears as a data sheet, while an ineligible one is listed in the email as needing a re-save in Explore.ADDITIONAL INFORMATION
🤖 Generated with Claude Code