feat(charts): add extensible extra metadata column to slices#41200
feat(charts): add extensible extra metadata column to slices#41200bsovran wants to merge 2 commits into
Conversation
Add a nullable `extra` column to the `slices` table and the `Slice` model to store open-ended, structured chart metadata, mirroring the existing dataset `extra` pattern. Uses `MediumText` for cross-backend consistency and to avoid MySQL's 64KB TEXT limit, and includes the field in chart import/export. Ref: SIP apache#41171 Co-authored-by: Cursor <cursoragent@cursor.com>
bsovran
requested review from
betodealmeida,
eschutho,
michael-s-molina,
mistercrunch and
sadpandajoe
as code owners
Code Review Agent Run #6c6e4bActionable Suggestions - 0Filtered 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 #41200 +/- ##
=======================================
Coverage 64.33% 64.33%
=======================================
Files 2651 2651
Lines 144766 144768 +2
Branches 33401 33401
=======================================
+ Hits 93131 93133 +2
Misses 49965 49965
Partials 1670 1670
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| viz_type = Column(String(250)) | ||
| params = Column(utils.MediumText()) | ||
| query_context = Column(utils.MediumText()) | ||
| extra = Column(utils.MediumText()) |
There was a problem hiding this comment.
Suggestion: The new chart metadata field is persisted on the model but not copied when charts are cloned, so dashboard duplication will silently drop extra metadata on the duplicated charts. Update the chart cloning path to include this new field so cloned charts preserve the same metadata as the source chart. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Dashboard copy with duplicate charts drops chart extra metadata.
- ⚠️ Cloned charts diverge from originals in stored configuration.Steps of Reproduction ✅
1. In a running Superset instance with this PR applied, pick an existing dashboard with
charts and ensure at least one chart has `Slice.extra` set (e.g. via a shell or test
fixture): `original_slc.extra = '{"foo": "bar"}'` for a `Slice` instance attached to a
dashboard. The `Slice` model (including the new `extra` column) is defined in
`superset/models/slice.py:17-27` as seen in the class definition.
2. Trigger a dashboard copy operation with chart duplication enabled. In production this
is done via the dashboard copy API which uses `CopyDashboardCommand`
(`superset/commands/dashboard/copy.py:6-15`) and `DashboardDAO.copy_dashboard`. In tests
the same path is exercised directly in
`superset/tests/integration_tests/dashboards/dao_tests.py:27-42`, where
`DashboardDAO.copy_dashboard(original_dash, dash_data)` is called with
`dash_data["duplicate_slices"] = True`.
3. During the copy operation, `DashboardDAO.copy_dashboard` in
`superset/daos/dashboard.py:368-46` iterates over `original_dash.slices` and, when
`duplicate_slices` is true, calls `new_slice = slc.clone()` at line 386 for each chart,
then adds each `new_slice` to the session and associates it with the new dashboard.
4. The `Slice.clone` method in `superset/models/slice.py:151-42` constructs a new `Slice`
with only `slice_name`, `datasource_id`, `datasource_type`, `datasource_name`, `viz_type`,
`params`, `description`, and `cache_timeout`, and does not copy the `extra` field. As a
result, each cloned chart's `extra` column remains NULL/empty even though the source
chart's `extra` contained metadata, so the duplicated dashboard's charts silently lose
their `extra` metadata.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/slice.py
**Line:** 85:85
**Comment:**
*Incomplete Implementation: The new chart metadata field is persisted on the model but not copied when charts are cloned, so dashboard duplication will silently drop `extra` metadata on the duplicated charts. Update the chart cloning path to include this new field so cloned charts preserve the same metadata as the source chart.
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
|
The flagged issue is correct. The To resolve this, locate the # In superset/models/slice.py, inside the clone() method:
new_slice = Slice(
slice_name=self.slice_name,
# ... existing fields ...
extra=self.extra,
# ...
)There are no other comments on this PR to address. superset/models/slice.py |
Adding `extra` to `Slice.export_fields` changes the chart export YAML, so update the export command test expectations (metadata dict and key order). Also accept `extra` in `ImportV1ChartSchema` so export->import round-trips, and copy `extra` in `Slice.clone()` so duplicated charts (e.g. dashboard copy) preserve the metadata. Co-authored-by: Cursor <cursoragent@cursor.com>
Code Review Agent Run #94803fActionable Suggestions - 0Filtered 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 |
2 participants
SUMMARY
Adds a nullable
extracolumn to theslicestable (and theSlicemodel) to store open-ended, structured chart metadata, mirroring the long-standing datasetextrapattern.Charts currently have no extensible metadata field.
descriptionis unsuitable because it is user-facing prose rendered as markdown and shown as a tooltip in the Charts list. Datasets already solve this with anextraJSON-bearing column (SqlaTable.extra), and columns/metrics have their ownextraas well. This change brings the same proven, low-risk pattern to charts.Design decisions:
extra, consistent withtables.extraand column/metricextra.MediumText(superset.utils.core.MediumText), which resolves toTEXTon Postgres/SQLite andMEDIUMTEXTon MySQL. This matches the existing JSON-bearing chart fieldsparamsandquery_context, avoids MySQL's 64KBTEXTlimit, and stores identical bytes across backends (unlike a native JSON column, which MySQL normalizes/reorders).SqlaTable.extra_dictdoes today.extratoSlice.export_fieldsso it round-trips through import/export.This PR intentionally scopes to the model + migration foundation. Exposing
extrathrough the chart REST API (ChartPostSchema/ChartPutSchema/response schema) and any UI can follow in a separate PR.Associated SIP: #41171
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - backend schema change with no UI surface in this PR.
TESTING INSTRUCTIONS
superset db upgradeand confirm theextracolumn is added toslices(nullable).superset db downgradeand confirm the column is cleanly removed.superset db headsreports a single head.Slice.extra(e.g. via shell) and confirm round-trip through chart export/import.The migration uses the idempotent
add_columns/drop_columnshelpers fromsuperset/migrations/shared/utils.py, following the exact shape of the existing94e7a3499973(addfoldersto datasets) migration. Upgrade/downgrade of the additive column was exercised against a local metadata DB.ADDITIONAL INFORMATION
Runtime/downtime: adding a nullable column to
slicesis a metadata-only DDL change on Postgres (no table rewrite, no default backfill) and completes near-instantly; no downtime expected. MySQL 8 performs this as an INSTANT add for a nullable column.Made with Cursor