Repository navigation
Conversation
…le chart YAMLs The `groupby` -> `columns` field rename (superset/common/query_object.py::DEPRECATED_FIELDS) was already applied to birth_names/world_bank fixtures (apache#43468) and to two sibling deprecated fields elsewhere. This closes the gap for the remaining example dashboards (fcc_new_coder_survey, featured_charts, sales_dashboard, slack_dashboard, video_game_sales), scoped to the 5 viz_types (table, pie, echarts_timeseries_line, echarts_area, treemap_v2) confirmed to read only `columns` (not `groupby`) in their buildQuery.ts, so the rename is a pure key rename with no behavior change.
…rt unsafe types
Self-review caught that echarts_timeseries_line, echarts_area, and treemap_v2
buildQuery.ts implementations destructure `const { groupby } = formData` directly
(e.g. Timeseries/buildQuery.ts:44, Treemap/buildQuery.ts:27) rather than reading
`formData.columns`. A prior grep for `.groupby`/`formData.groupby` missed this
destructuring pattern. Renaming groupby->columns in those 10 files would silently
drop the grouping dimension from the query (Timeseries) or the orderby (Treemap) -
a real data/display regression, not a no-op.
Reverts the 10 affected files (Line, Area, Proportion_of_Revenue_by_Product_Line,
Messages_per_Channel, Commute_Time, Preferred_Employment_Style, TreeMap,
Members_per_Channel, Games_per_Genre, of_Games_That_Hit_100k_in_Sales_By_Release_Year)
back to their original groupby: key. The remaining 16 files (table, pie) are
confirmed safe: `table`'s buildQuery.ts only reads `baseQueryObject.columns`, and
`pie`'s buildQuery.ts never destructures `groupby` at all, relying on the generic
`extractQueryFields` groupby/columns aliasing.
Code Review Agent Run #fe3eccActionable Suggestions - 0Additional Suggestions - 2
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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43723 +/- ##
=======================================
Coverage 81.11% 81.11%
=======================================
Files 2955 2955
Lines 178335 178335
Branches 41311 41311
=======================================
+ Hits 144660 144663 +3
+ Misses 30972 30969 -3
Partials 2703 2703
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:
|
|
Pulling this back for re-review pending a fix. An independent review (EngCodeReviewBot / Minerva) that cleared Converting to draft until this is re-verified. cc @kamil for re-review once resolved. |
… charts EngCodeReviewBot's 2026-09-03 review (posted on this PR) retracted its earlier pie-chart sign-off: plugin-chart-echarts/src/Pie/controlPanel.tsx names its dimension control literally 'groupby' (confirmed at controlPanel.tsx:54), and extractQueryFields.ts discards a 'columns' key in aggregate mode (extractQueryFields.ts:74-78) rather than aliasing it back to groupby. So a pie-type fixture with columns: instead of groupby: silently drops its grouping dimension - the identical bug class already found in the 10 echarts_timeseries_line/echarts_area/treemap_v2 files reverted by the prior commit on this branch. Reverts the rename on the 8 remaining pie-type files (all other viz_types in this PR are table, confirmed safe: buildQuery.ts only reads baseQueryObject.columns). No table-type files touched. Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Agent Run #40c5aaActionable 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 |
|
Re-routing review on this one. @kgabryje was the sole requested reviewer since 2026-08-31 but hasn't had a chance to get to it in the 16+ days since (no worries — everyone's stretched). Since the risk surface here is frontend chart-rendering correctness for the pie/table example charts, I've requested @aminghadersohi, who reviewed and approved the two nearest siblings in this same deprecation series (#43422, #43432) and is active on frontend/viz PRs. Thanks both. |
|
@eschutho — flagging CI before this sits in a reviewer's queue: the latest head ( The good news is that none of them look like your diff:
Those are harness/workflow-level failures, and the run dates to 2026-09-09 against a now-stale branch. A rebase on current Could you get it green before the review pass? Reviewers here don't pick up red-CI PRs, so this is the one thing standing between #43723 and a review. 🦉 — Minerva (EngCodeReviewBot) |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Executed buildQueryObject on all 8 charts: the 3 raw-mode files are byte-identical, but all 5 aggregate-mode files lose their dimension, because extractQueryFields:76 discards a literal columns key before the groupby->columns rename at :89.
…regate-mode table charts Per aminghadersohi's review, five more table charts in this PR run with query_mode: aggregate and had their `groupby:` key renamed to a literal `columns:` key. In extractQueryFields.ts, the aggregate-mode guard (`if (queryMode === QueryMode.Aggregate && normalizedKey === 'columns') return;`) runs before the groupby->columns alias is applied, so a literal `columns:` key is silently dropped and the chart loses its grouping dimension. Restore `groupby:` on: - fcc_new_coder_survey/charts/Breakdown_of_Developer_Type.yaml - fcc_new_coder_survey/charts/Highest_degree_held.yaml - fcc_new_coder_survey/charts/Top_15_Languages_Spoken_at_Home.yaml - sales_dashboard/charts/Total_Items_Sold_By_Product_Line.yaml - slack_dashboard/charts/Top_Timezones.yaml Raw-mode table charts (Table, Games, Publishers_With_Most_Titles) are unaffected and keep the rename. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Aggregate reverts verified — buildQueryObject output now matches the merge base on all 8 files. But the query_object.py:221 warning fires on query_context.queries[].groupby; these 3 charts are query_context: null + raw mode, so the rename can't silence it.
| datasource: 21__table | ||
| granularity_sqla: year | ||
| groupby: | ||
| columns: |
There was a problem hiding this comment.
columns is not a control in plugin-chart-table (controlPanel.tsx declares groupby + all_columns). groupby: [name] is the retained aggregate-mode Dimensions value (resetOnHide: false), so switching this chart to Aggregate now starts empty.
| columns: | |
| groupby: |
Code Review Agent Run #6fc941Actionable 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 @eschutho, this is down to just the three raw-mode table charts now. I agree with Amin's open thread on |
Summary
Datadog logs for
apache/supersetproduction surface a recurringDeprecationWarning: "The fieldgroupbyis deprecated, please usecolumnsinstead." (superset/common/query_object.py::DEPRECATED_FIELDS).The
groupby->columnsrename was already applied to thebirth_names/world_bankexample fixtures in #43468 (still open). This closes the gap for a further batch of example dashboards:fcc_new_coder_survey,featured_charts,sales_dashboard,slack_dashboard,video_game_sales.Decisions made that were not in the instructions
None.
What changed
Renamed the top-level
groupby:key tocolumns:underparams:in 16 exported chart YAML fixtures, scoped strictly to two viz_types verified safe at the query-construction layer:table(8 files):plugin-chart-table'sbuildQuery.tsonly readsbaseQueryObject.columns(built via the genericextractQueryFieldsgroupby/columns aliasing), neverformData.groupbydirectly, for plain string columns like these fixtures use.pie(8 files):Pie/buildQuery.tsnever destructuresformData.groupbyat all; it relies entirely on the same genericextractQueryFieldsaliasing.Every changed file had exactly one top-level (
params:-level)groupby:key; unrelated same-named keys elsewhere in the same files (e.g. a per-columngroupby: true/false"is this groupable" flag nested inside a metric'scolumn:definition, and an inertqueryFields: {groupby: groupby}mapping block in one raw-mode table chart) were left untouched.Scope note: this PR was originally drafted covering 26 files across 5 viz_types (adding
echarts_timeseries_line,echarts_area,treemap_v2). Self-review caught that those three plugins'buildQuery.tsdestructuregroupbydirectly fromformData(e.g.Timeseries/buildQuery.ts:const { groupby } = formData, used to build bothcolumnsandseries_columns;Treemap/buildQuery.ts:groupbydrivesorderby) rather than reading the renamedcolumnsfield — renaming those would have silently dropped the grouping dimension / sort order from the affected example charts. Those 10 files were reverted before opening this PR; only the two types confirmed safe by direct code inspection are included here.No behavior change for the included files: this only renames a dict key that both
tableandpieconsume identically through the existingcolumns/groupbyalias inextractQueryFields.Test plan
yaml.safe_load()on all 16 changed files: clean.pre-commit run check-yaml / trailing-whitespace / end-of-file-fixeron all 16 files: clean.git diffthat only the single top-levelgroupby:key changed per file, with a per-file check confirming exactly one match for^ groupby:before editing.superset-frontend/plugins/**/buildQuery.tsfor bothformData.groupbyand destructuring (const { groupby } = formData) usage across all 5 originally-considered viz_types; confirmedtableandpienever readgroupby, whileecharts_timeseries_line/echarts_area(Timeseries/buildQuery.ts) andtreemap_v2(Treemap/buildQuery.ts) do, which is why those types were excluded from the final diff.