Repository navigation
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 fixThere was a problem hiding this comment.
Good catch, fixed. Pushed a small cache on CertificationMixin.get_extra_dict() keyed off the raw extra string, so the four certification/warning properties share one json.loads per column/metric instead of reparsing each time, and it self-invalidates if extra is mutated on the instance (0a580f3).
|
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? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43319 +/- ##
===========================================
+ Coverage 66.67% 78.84% +12.16%
===========================================
Files 2876 2878 +2
Lines 164007 165301 +1294
Branches 37834 38099 +265
===========================================
+ Hits 109347 130324 +20977
+ Misses 52514 32499 -20015
- Partials 2146 2478 +332
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:
|
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Taking a look into the bot review comments, but this is looking promising....
… JSON parsing CertificationMixin.get_extra_dict() re-parses the serialized `extra` JSON on every call. Since is_certified, certified_by, certification_details, and warning_markdown are now all exposed on the dataset show endpoint, serializing a single column/metric can call get_extra_dict() up to four times. Cache the parsed dict on the instance, keyed off the raw string it was parsed from, so it self-invalidates whenever `extra` changes. Addresses codeant-ai review feedback on apache#43319. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rusackas
added
the
merge-if-green
Code Review Agent Run #b3add6Actionable 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 |
…taset (#43319) Co-authored-by: rusackas <evan@rusackas.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
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