fix(explore): keep certification badges after saving or swapping a dataset - #43319
fix(explore): keep certification badges after saving or swapping a dataset#43319s1ny1998 wants to merge 1 commit into
Conversation
…taset The dataset `show` endpoint exposed `columns.extra` and `metrics.extra` but not the certification and warning attributes derived from them. Explore rehydrates its datasource from that payload after a dataset is saved from the "Edit dataset" modal or swapped from the chart source, so the certified and warning badges — and the certified field values for columns and calculated columns — disappeared until the page was reloaded, where the Explore bootstrap serialization does include them. Expose `is_certified`, `certified_by`, `certification_details` and `warning_markdown` for both columns and metrics so the endpoint matches the serialization Explore already expects. Fixes apache#43279
Code Review Agent Run #a13631Actionable 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 |
dosubot
Bot
added
api
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| "columns.certification_details", | ||
| "columns.certified_by", | ||
| "columns.is_certified", | ||
| "columns.warning_markdown", | ||
| "metrics.certification_details", | ||
| "metrics.certified_by", | ||
| "metrics.is_certified", | ||
| "metrics.warning_markdown", |
There was a problem hiding this comment.
Suggestion: Each newly exposed certification property independently reparses the same serialized extra value, so every column and metric incurs up to four additional JSON parses during serialization. On wide datasets this adds avoidable CPU and latency to every show request; parse the metadata once and serialize the derived fields from the cached mapping or a single combined representation. [performance]
Severity Level: Minor 🧹
- ⚠️ Dataset show responses parse metadata repeatedly.
- ⚠️ Wide datasets incur additional serialization CPU.
- ⚠️ Large Explore datasource hydration may add latency.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/datasets/api.py
**Line:** 285:292
**Comment:**
*Performance: Each newly exposed certification property independently reparses the same serialized `extra` value, so every column and metric incurs up to four additional JSON parses during serialization. On wide datasets this adds avoidable CPU and latency to every `show` request; parse the metadata once and serialize the derived fields from the cached mapping or a single combined representation.
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 performance issue is correct. The current implementation of the dataset API exposes certification and warning metadata by dynamically accessing model properties, which triggers repeated JSON parsing of the To resolve this, you should parse the Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? |
1 participant
SUMMARY
Saving a dataset from Explore (
...beside the dataset name → Edit dataset → Save), or swapping the chart's dataset, temporarily cleared the Certified / Warning icons and the Certified field values for metrics, columns and calculated columns. A full page refresh restored them, so nothing was actually lost on the backend — Explore's client state was simply missing the fields.Both flows rehydrate the Explore datasource from
GET /api/v1/dataset/:id(DatasourceModalafter thePUT, andChangeDatasourceModalon swap), and hand the response straight tochangeDatasource()→SET_DATASOURCE.That payload was missing the certification metadata.
show_select_columnsexposescolumns.extraandmetrics.extra, but not the attributes derived fromextra—is_certified,certified_by,certification_detailsandwarning_markdown— which are@propertyvalues onCertificationMixin. The Explore bootstrap serialization (TableColumn.data/SqlMetric.data) does include them, which is why a refresh fixed it.This adds those four fields, for both columns and metrics, to
show_columnsso the REST endpoint matches the serialization Explore already expects.show_columnsis the right list (rather thanshow_select_columns) because these are model properties, not database columns — the same placecolumns.type_genericlives.Metric field values partly survived before because
DatasourceEditoralready re-parsesmetric.extraon mount; columns had no equivalent, so they came backundefined. That also meant a second save could write an emptyextraback for columns, sincebuildExtraJsonObject()rebuildsextrafrom those now-missing fields — this fixes that too.The change is purely additive to the
showresponse. Feeding ashowpayload straight back intoPUTwas already unsupported (changed_on,created_onandtype_genericare likewise read-only), so the round-trip tests were updated to strip the new fields alongside the existing ones.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: see the recording in #43279 — icons vanish immediately after Save and return after a page refresh.
After: icons and Certified values stay put after Save and after a dataset swap, with no refresh.
TESTING INSTRUCTIONS
Automated:
pytest tests/unit_tests/datasets/api_tests.py::test_get_dataset_exposes_certification_metadata pytest tests/integration_tests/datasets/api_tests.py -k "update_dataset_create_column_and_metric or update_dataset_delete_column or update_dataset_update_column"Manual:
...beside the dataset name → Edit dataset → Save, without changing anything.ADDITIONAL INFORMATION