EnxDev commented on PR #42284:
URL: https://github.com/apache/superset/pull/42284#issuecomment-5177534191
EnxDev's Review Agent โ apache/superset#42284 ยท HEAD 14c86a3
request changes โ the rebuild diverges from the mirrored frontend buildQuery
in four places, and UPDATING.md documents a config key that does not exist in
the code.
Supersedes my earlier review on de7dfc1 (comment). All nine findings from it
are genuinely fixed in this diff โ I re-verified each against the code, not the
"fixed in X" replies: since/until fallback (:296), where_only HAVING split
(:105), columns: [] no longer shadowing groupby (:162),
percent_metrics/time_compare/rolling/resample skips, Big Number granularity
promotion, raw-mode stale metrics/groupby, table order_desc default, and the
stale order_by_cols gate. Good follow-through.
๐ด Functional
- superset/common/form_data_query_context.py:104 ยท High โ
freeform_where_having wraps SQL clauses as ({clause}) with no newline, but the
mirrored processFilters.ts:26-32 appends \n when the clause contains --. A
chart whose free-form SQL filter ends in a -- comment produces (sales > 0 --
note) on one line: the closing paren and every subsequent AND (...) are
swallowed by the comment, so the export fails where the chart renders fine.
Port sanitizeClause verbatim. regression test: an adhoc_filters SQL entry
"sales > 0 -- note" plus a second SQL WHERE clause; assert extras["where"]
contains a newline before the closing paren and both predicates survive.
- superset/tasks/export_dashboard_excel.py:75 ยท Medium โ pie is on the
rebuild allowlist, but Pie/buildQuery.ts:34-42 attaches a contribution
post-processing op unconditionally, not only when percent_metrics is set.
contribution() writes rename_columns as new columns (contribution.py:98,164)
and getContributionLabel appends a distinct suffix, so a rebuilt pie sheet
loses the percentage column a saved-context pie sheet has โ two pies on one
dashboard export different columns depending only on whether they were
re-saved. Either apply the contribution op for pie or drop pie from the
allowlist. regression test: rebuild a pie and assert the query carries the
contribution post_processing entry.
- superset/tasks/export_dashboard_excel.py:134 ยท Medium โ
_needs_unsupported_processing misses show_totals. buildQuery.ts:345-365 pushes
a second totals query when metrics && show_totals && aggregate; the
single-query rebuild drops it, so the sheet silently omits the totals row. This
is the same class of defect as percent_metrics, which you already decided
warrants a skip. Add show_totals (aggregate mode only) to the skip check.
regression test: table params with show_totals: true โ chart listed under
ERROR_NO_QUERY_CONTEXT, no query run.
- superset/common/form_data_query_context.py:186 ยท Medium โ
timeseries_limit_metric is read raw, but the frontend normalizes it with
ensureIsArray(...)[0] (buildQuery.ts:67). A chart storing ["revenue"] yields
orderby: [[["revenue"], True]], which fails downstream and dumps the chart into
the general error bucket instead of exporting. Unwrap a single-element list.
regression test: timeseries_limit_metric: ["revenue"] โ orderby == [["revenue",
True]].
- UPDATING.md:76 ยท High โ EXCEL_EXPORT_REBUILD_VIZ_TYPES is documented as a
new config key and the PR summary calls the allowlist "configurable", but it
exists nowhere in the codebase: REBUILD_VIZ_TYPES
(export_dashboard_excel.py:75) is a hardcoded constant with no
current_app.config lookup, superset/config.py is not in this diff, and
test_rebuild_viz_types_is_the_conservative_default asserts "no config
override". Note this also reverts the fix promised to codeant-ai
("_rebuild_viz_types now checks is None explicitly") โ that function is gone at
HEAD. Operators following the release notes would set a key that does nothing,
with no way to un-skip their charts. Either restore the config read (mirroring
_table_viz_types() at :105 plus a config.py default) or drop the key from
UPDATING.md and the PR summary.
๐ก Should-fix
- superset/tasks/export_dashboard_excel.py:106 โ _saved_query_context only
tests parsed.get("queries") for truthiness, so {"queries": "oops"} passes as a
valid saved context and fails later in the general bucket rather than the clean
"no query context" path. Check isinstance(..., list).
๐ต Nits
- superset/common/form_data_query_context.py:206-215 โ parsed order_by_cols
entries aren't validated as 2-element [col, asc] pairs; a stray null appends
None to orderby.
--
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]
