fix(examples): replace deprecated groupby field with columns in birth_names/world_bank - #43468
fix(examples): replace deprecated groupby field with columns in birth_names/world_bank#43468eschutho wants to merge 1 commit into
Conversation
…_names/world_bank DEPRECATED_FIELDS maps groupby -> columns (superset/common/query_object.py). #43422/#43432 already migrated timeseries_limit_metric and granularity_sqla in these same fixtures but missed the sibling groupby field, on both the .py slice-creation defaults and the exported .yaml chart configs.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| params=get_slice_json( | ||
| defaults, | ||
| groupby=["name"], | ||
| columns=["name"], |
There was a problem hiding this comment.
Suggestion: The chart form data now stores dimensions only in columns, but aggregate-mode frontend query extraction intentionally ignores columns and reads dimensions from groupby; the timeseries builder and chart renderers also consume formData.groupby. As a result, the affected pie, table, area, and timeseries charts are queried without their intended name/gender/state dimensions and render collapsed or incorrectly labeled results. Preserve the frontend-compatible groupby field or update the complete frontend migration path, rather than changing only the saved fixture key. [api mismatch]
Severity Level: Major ⚠️
- ❌ Generated Birth Names table charts lose their name grouping.
- ⚠️ Example dashboard tables can show collapsed aggregate results.
- ⚠️ Table rendering loses grouping-based labels and interactions.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/examples/birth_names.py
**Line:** 290:290
**Comment:**
*Api Mismatch: The chart form data now stores dimensions only in `columns`, but aggregate-mode frontend query extraction intentionally ignores `columns` and reads dimensions from `groupby`; the timeseries builder and chart renderers also consume `formData.groupby`. As a result, the affected pie, table, area, and timeseries charts are queried without their intended name/gender/state dimensions and render collapsed or incorrectly labeled results. Preserve the frontend-compatible `groupby` field or update the complete frontend migration path, rather than changing only the saved fixture key.
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| viz_type="table", | ||
| metrics=["sum__SP_POP_TOTL"], | ||
| groupby=["country_name"], | ||
| columns=["country_name"], |
There was a problem hiding this comment.
Suggestion: The affected World Bank charts likewise provide their dimensions only through columns, while the timeseries and treemap builders and renderers read aggregate dimensions from groupby; aggregate query-field extraction also ignores columns. Consequently, Growth Rate, World's Pop Growth, Rural Breakdown, and Treemap can query ungrouped data and lose their country/region hierarchy or series labels. Keep the frontend-required grouping field or migrate the consumers together with these fixtures. [api mismatch]
Severity Level: Major ⚠️
- ❌ World Bank country table loses country-level grouping.
- ⚠️ Generated dashboard results collapse into aggregate rows.
- ⚠️ Country labels and table interactions become incorrect.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/examples/world_bank.py
**Line:** 193:193
**Comment:**
*Api Mismatch: The affected World Bank charts likewise provide their dimensions only through `columns`, while the timeseries and treemap builders and renderers read aggregate dimensions from `groupby`; aggregate query-field extraction also ignores `columns`. Consequently, Growth Rate, World's Pop Growth, Rural Breakdown, and Treemap can query ungrouped data and lose their country/region hierarchy or series labels. Keep the frontend-required grouping field or migrate the consumers together with these fixtures.
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![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #d0487c
Actionable Suggestions - 2
-
superset/examples/usa_births_names/charts/Genders.yaml - 1
- CWE-20: Wrong chart parameter · Line 26-26
-
superset/examples/usa_births_names/charts/Trends.yaml - 1
- Broken chart series grouping · Line 26-26
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/examples/usa_births_names/charts/Top_10_Boy_Name_Share.yaml - 1
- Wrong parameter for echarts_area · Line 33-33
-
superset/examples/usa_births_names/charts/Top_10_Girl_Name_Share.yaml - 1
- CWE-20: Groupby Key Broken · Line 33-33
-
superset/examples/world_health/charts/Most_Populated_Countries.yaml - 1
- Wrong field for aggregate mode · Line 28-28
Review Details
-
Files reviewed - 21 · Commit Range:
b6088ee..b6088ee- superset/examples/birth_names.py
- superset/examples/usa_births_names/charts/Boy_Name_Cloud.yaml
- superset/examples/usa_births_names/charts/Boys.yaml
- superset/examples/usa_births_names/charts/Genders.yaml
- superset/examples/usa_births_names/charts/Genders_by_State.yaml
- superset/examples/usa_births_names/charts/Girl_Name_Cloud.yaml
- superset/examples/usa_births_names/charts/Girls.yaml
- superset/examples/usa_births_names/charts/Participants.yaml
- superset/examples/usa_births_names/charts/Pivot_Table_v2.yaml
- superset/examples/usa_births_names/charts/Top_10_Boy_Name_Share.yaml
- superset/examples/usa_births_names/charts/Top_10_Girl_Name_Share.yaml
- superset/examples/usa_births_names/charts/Trends.yaml
- superset/examples/world_bank.py
- superset/examples/world_health/charts/Box_plot.yaml
- superset/examples/world_health/charts/Growth_Rate.yaml
- superset/examples/world_health/charts/Most_Populated_Countries.yaml
- superset/examples/world_health/charts/Rural.yaml
- superset/examples/world_health/charts/Rural_Breakdown.yaml
- superset/examples/world_health/charts/Treemap.yaml
- superset/examples/world_health/charts/World_s_Pop_Growth.yaml
- superset/examples/world_health/charts/World_s_Population.yaml
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| compare_suffix: o10Y | ||
| granularity_sqla: ds | ||
| groupby: | ||
| columns: |
There was a problem hiding this comment.
The Pie chart plugin (plugin-chart-echarts/src/Pie/controlPanel.tsx line 54) explicitly requires the groupby parameter, not columns. All 5 other pie chart examples in the codebase (Pie.yaml, Work_Location_Preference.yaml, etc.) use groupby. The columns parameter belongs to Table/Pivot Table charts, not Pie. This change will cause the Genders chart to not group by gender at runtime. (See also: CWE-20)
Code Review Run #d0487c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| compare_suffix: o10Y | ||
| granularity_sqla: ds | ||
| groupby: | ||
| columns: |
There was a problem hiding this comment.
Changing groupby to columns will break chart functionality. The frontend buildQuery.ts reads formData.groupby (line 44) to determine series grouping via series_columns: groupby (line 95). The columns field serves a different purpose (raw query column selection). While security/manager.py (lines 1550-1551) treats them as equivalent for access control, they are NOT interchangeable for query execution semantics.
Code Review Run #d0487c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
1 participant
Summary
Datadog logs for
apache/supersetproduction surfaced a recurringDeprecationWarning: "The field `groupby` is deprecated, please use `columns` instead." (superset/common/query_object.py::DEPRECATED_FIELDS).The
groupby->columnsrename was already applied to two siblingDEPRECATED_FIELDSentries in these same example fixtures (timeseries_limit_metricin #43422,granularity_sqlain #43432), but both of those PRs left everygroupbyoccurrence insuperset/examples/birth_names.pyandsuperset/examples/world_bank.py(and their exported.yamlchart configs undersuperset/examples/usa_births_names/charts/andsuperset/examples/world_health/charts/) untouched. This closes that gap for thegroupbyfield specifically.Decisions made that were not in the instructions
None.
What changed
superset/examples/birth_names.py,superset/examples/world_bank.py: renamed thegroupbykey/kwarg tocolumnsin thecreate_slices()default params and per-slice overrides passed toget_slice_json().superset/examples/usa_births_names/charts/andsuperset/examples/world_health/charts/: renamed thegroupbykey underparams:tocolumns:.groupbyRows/groupbyColumns(pivot-table-v2-specific fields, unrelated to this deprecation) were left untouched.No behavior change: this only renames a dict key that's consumed identically either way —
get_slice_json()does a plaindict.update()merge, and downstreamQueryObjectconstruction already treatscolumnsas the current name for this field (DEPRECATED_FIELDSexists specifically to accept both during a migration window). The generated example dashboards/charts are unchanged.Test plan
ruff check/ruff format --checkon both changed.pyfiles: clean..yamlfiles parse viayaml.safe_load: clean.ast.parse()on both changed.pyfiles: clean.get_slice_json()'s dict-merge/override semantics are unaffected by the rename (defaults + per-slice kwarg override both resolve to thecolumnskey as expected).ModuleNotFoundError/ImportErrorchain unrelated to these files) — could not run the integration test suite that exercisescreate_slices()(tests/integration_tests/fixtures/birth_names_dashboard.py,world_bank_dashboard.py); relying on CI for full integration coverage.