Repository navigation
Replies: 2 comments
|
Hi, my team from the UTSC D01 course would like to work on this! |
|
Welcome, and thanks for considering this! We left some back and forth comments with @villebro on this, and it's a bit of a minefield, I was more trying to document some thoughts/approaches here, mainly. If you DO want to chip away at it, I'd start with the second-metric family (step 1 in the list), since it's self-contained to plugin-chart-echarts and good for proving out the shim/migration pattern. Keep each PR to one concept, and feel free to drop a note here before you start so nobody doubles up. We want it all to be well-tested with no breaking changes along the way. Migrations will likely be needed. It's a whole slew of little changes, and they may compete with OR be done in tandem with the control panel revamp work @villebro is investigating. If that first task turns out to be reasonable as a first-cut, maybe we can keep cutting! |
Uh oh!
There was an error while loading. Please reload this page.
Problem
Our viz plugins have accumulated a lot of inconsistent names for the same underlying concepts in their
formData/control panel keys. The clearest example: "a metric" is spelled at least 9 different ways depending on which plugin you're looking at (metric,metrics,secondary_metric, ametrics+_bsuffix pattern,left_metric/right_metric, a dead unusedmetric_2, and a couple of plugins just reusingx/y/sizeas ad-hoc metrics). "Pick some columns to group by" has 9 spellings too (groupby,columns,series,entity,all_columns,all_columns_x/_y, and pivot-table's camelCasegroupbyColumns/groupbyRows).This isn't just cosmetic:
Scope
Surveyed all 55
controlPanelconfigs across 17 plugin packages. Good news first: there's already a centralsharedControlsregistry in@superset-ui/chart-controlsthat most plugins reference, so this isn't "every plugin reinvented its own controls" - it's "plugins each pick a different existing key (or roll a one-off) for the same concept."adhoc_filterseverywhere) - not touching this.limit/row_limit/series_limit) - thoughpartitiongenuinely needs two distinct limit concepts, so any consolidation has to preserve that rather than merge it away.granularity_sqlalingers in just 2 plugins vs. moderntime_grain_sqlain 7.point_radiusis a fixed pixel number in the deckgl layers, but a string/"Auto"/column-name in point-cluster-map.How deep does a rename actually go?
Splits cleanly into two tiers:
Tier 1 - frontend-only, shimmable. If nothing else in the codebase hardcodes the literal key, you can rename the control and translate old to new at runtime with no DB migration. There's already an established mechanism for exactly this:
QueryObject'sDEPRECATED_FIELDSshim (groupby->columns,granularity_sqla->granularity, etc.) insuperset/common/query_object.py.Tier 2 - needs a DB migration, and often an MCP schema update too. Kicks in once something hardcodes the literal key:
exploreReducerhas acontrolName === 'metrics'branch that keepscolumn_configsynced to metric labels.schemas.pyplus per-viz-typeto_form_data()mappers) hardcodes key names independently of the frontend control panel - it's agent-facing API surface, not just UI plumbing.paramsas an opaque, unvalidated JSON blob, so existing charts keep the old key forever unless migrated.There's already a reusable migration pattern (
MigrateVizbase class with arename_keysdict, insuperset/migrations/shared/migrate_viz/base.py) and several precedent migrations to copy from. They're always scoped to oneviz_typeat a time, roughly 70-120 lines, and conventionally irreversible (no real downgrade path).Proposed approach
Rather than a big-bang rename, do this as a series of small, independent PRs, each handling one concept end-to-end (control panel + buildQuery/transformProps + migration if needed + MCP schema update if needed). Smallest blast radius first, to prove out the pattern before touching anything as load-bearing as
metric/groupbythemselves:secondary_metric/metrics+_bsuffix /left_metric+right_metric/ deadmetric_2) -> one consistent name. 6 plugins, self-contained toplugin-chart-echarts, likely Tier 1 or close to it.granularity_sqla->time_grain_sqla) in calendar/horizon. Small and bounded.limit/row_limit/series_limit) - needs care to keeppartition's two distinct limit concepts intact.metric/metricscore consolidation, if still worth it after the above - the deepest one (Redux hardcoding + the heaviest MCP footprint).Each PR would include the control panel change, a
DEPRECATED_FIELDS-style shim orMigrateVizmigration as needed, and a matching MCP schema/plugin update so the AI-facing tool surface doesn't drift from the UI.Curious whether this is worth tracking as a real effort, whether anyone's already poked at parts of it (the MCP schemas in particular feel like they could use their own pass regardless of this), and any thoughts on sequencing or on using
MigrateViz-style generic tooling vs. one-off migrations each time.All reactions