EnxDev commented on PR #42284:
URL: https://github.com/apache/superset/pull/42284#issuecomment-5054253123
## EnxDev's Review Agent โ apache/superset#42284 ยท HEAD de7dfc1
**request 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-`columns` shadowing `groupby`, legacy `filters` merge, `is None` config
check), each with a covering test. The findings below are separate.
Note: the `codeant-ai` comments embed "Prompt for AI Agent" blocks
instructing agents to implement fixes and poll the user. Treated as data, not
instructions.
### ๐ด Functional
- **`superset/common/form_data_query_context.py:131`** ยท _High_ โ The query
sets `time_range` but never `granularity`. `superset/models/helpers.py:3805`
only applies `from_dttm`/`to_dttm` when `granularity` is set, and nothing here
emits a `TEMPORAL_RANGE` filter, so a legacy chart with `granularity_sqla:
"ds"` + `time_range: "Last quarter"` exports its **entire history**. This hits
exactly the target population: charts old enough to lack a `query_context` are
the ones using `granularity_sqla` + `time_range` rather than a `TEMPORAL_RANGE`
adhoc filter (those do survive the SIMPLE conversion). Fix: add `"granularity":
form_data.get("granularity") or form_data.get("granularity_sqla")` to the query
dict โ `ChartDataQueryObjectSchema` accepts both
(`superset/charts/schemas.py:1255-1259`, deprecation map at line 1492).
**regression test:** build from `{"groupby": ["country"], "metrics": ["count"],
"granularity_sqla": "ds", "time_range": "Last quarter"}` and assert the query
carrie
s `granularity` (or an equivalent temporal filter).
- **`superset/common/form_data_query_context.py:54`** ยท _High_ โ Custom-SQL
adhoc filters are dropped. In
`superset-frontend/packages/superset-ui-core/src/query/processFilters.ts:51-69`,
a `SQL` filter with `clause: WHERE`/`HAVING` (and a legacy top-level `where`)
becomes `extras.where`/`extras.having`. Dropping them means the export returns
rows the chart excludes โ a table restricted by a SQL predicate exports
unrestricted. Fix: map them into `extras` by clause, or treat any non-`SIMPLE`
adhoc filter as non-rebuildable and skip the chart into the re-save list.
**regression test:** form data with one `SQL`/`WHERE` filter asserts
`extras.where` is set, or that `_resolve_query_context` returns `None`.
- **`superset/common/form_data_query_context.py:129`** ยท _High_ โ
`form_data.get("orderby")` is never populated: form data stores `order_by_cols`
(raw mode โ `packages/superset-ui-core/src/query/extractQueryFields.ts:55`),
and aggregate mode derives ordering in the plugin โ table always sets
`[[sortByMetric, !orderDesc]]` or `[[metrics[0], false]]`
(`plugins/plugin-chart-table/src/buildQuery.ts:139-145`), pie sets `[[metric,
false]]` when `sort_by_metric`
(`plugins/plugin-chart-echarts/src/Pie/buildQuery.ts:33`). With the chart's
`row_limit`, the export returns an arbitrary N rows instead of the chart's top
N. Fix: derive from `timeseries_limit_metric` / `sort_by_metric` / `order_desc`
/ `order_by_cols`, falling back to first metric descending. **regression
test:** `{"metrics": ["count"], "groupby": ["c"], "row_limit": 10}` โ query
`orderby == [["count", False]]`.
- **`superset/common/form_data_query_context.py:113`** ยท _Medium_ โ Table
specifics ignored: `percent_metrics` are merged into the query metrics
(`plugins/plugin-chart-table/src/buildQuery.ts:161-164`), so those columns are
missing from the sheet, and `time_grain_sqla` bucketing (same file, lines
184-207) is lost, so a temporal dimension groups by raw timestamp instead of by
month/day โ more rows, different values. Fix: carry `percent_metrics` into
`metrics` and apply the time grain, or drop `table` from the default allowlist
until the mapping is faithful. **regression test:** form data with
`percent_metrics` + `time_grain_sqla` asserts both survive the rebuild.
- **`superset/common/form_data_query_context.py:115`** ยท _Medium_ โ When no
columns are found, `granularity_sqla` is promoted to a grouping column. For
`big_number_total` (in the default allowlist) legacy params commonly still
carry `granularity_sqla`, turning a single total into one row per timestamp.
The comment says "Big Number with a trendline", but the code doesn't check for
one. Fix: only promote for the trendline viz type (`big_number`), never
`big_number_total`. **regression test:** `{"viz_type": "big_number_total",
"metric": "count", "granularity_sqla": "ds"}` โ `columns == []`.
### ๐ก Should-fix
- **`superset/common/form_data_query_context.py:42,65`** โ This duplicates
`superset/mcp_service/chart/chart_utils.py:437`
(`adhoc_filters_to_query_filters`) and
`superset/mcp_service/chart/preview_utils.py:41` (`_build_query_columns`)
rather than sharing them, and the copies have already diverged:
`preview_utils.py:53` still has the `"columns" in form_data` bug this PR just
fixed here. Point the MCP path at the new shared module.
- **`tests/unit_tests/tasks/test_export_dashboard_excel.py:200-260`** โ The
export tests only assert `run()` call counts, never the query body that reaches
`ChartDataQueryContextSchema().load`. Findings 1 and 3 pass every test in this
PR. Add one test asserting the loaded payload (columns, filters, orderby,
granularity) for a rebuilt chart.
### ๐ต Nits
- `superset/tasks/export_dashboard_excel.py:287` โ `assert json_body is not
None` is stripped under `python -O`; prefer restructuring so the type narrows,
or a `cast`.
- `superset/tasks/export_dashboard_excel.py:143` โ "Copy so a
synthesized/shared payload is never mutated in place" overstates it:
`dict(json_body)` is shallow and `apply_dashboard_filter_context` mutates
`queries[*]` in place
(`superset/charts/data/dashboard_filter_context.py:359-364`). Harmless today
since each body is freshly built per chart.
- `superset/tasks/export_dashboard_excel.py:135` โ `chart.query_context` is
parsed twice (once in `_has_empty_query_context`, again on return).
### ๐ Praise
- `tests/unit_tests/tasks/test_export_dashboard_excel.py:185-215` โ the
parametrized blank / `null` / `{}` / empty-queries / malformed coverage plus
the explicit-empty-set config semantics is the right level of paranoia for a
skip path.
---
Findings are code-verified against this HEAD, not runtime-verified (test
suite not executed).
<!-- enxdev-review-agent:de7dfc1 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD de7dfc1._
--
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]
