-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add verbose map to get /dataset/ endpoint #23655
feat: add verbose map to get /dataset/ endpoint #23655
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional changes LGTM, but we need to add + update the schema. Also, we might want to consider DRYing the creation of verbose_map
into a util.
Codecov Report
@@ Coverage Diff @@
## master #23655 +/- ##
=======================================
Coverage 68.02% 68.02%
=======================================
Files 1936 1937 +1
Lines 74929 74937 +8
Branches 8141 8143 +2
=======================================
+ Hits 50970 50977 +7
- Misses 21871 21872 +1
Partials 2088 2088
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to that! I had no clue that each |
I tested the PR and the charts in drill by modal still don't use the verbose names. I think in order to achieve that, we also need to pass the dataset object that contains verbose map to |
# Conflicts: # superset-frontend/src/components/Chart/DrillBy/DrillByChart.tsx # superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx # superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
setDataset(result); | ||
const verbose_map = {}; | ||
ensureIsArray(result.columns).forEach((column: Column) => { | ||
verbose_map[column.column_name] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move that to a separate file/hook? Like useVerboseMap
?
The we could call it in DrillByChart
like const verboseMap = useVerboseMap(dataset)
and pass dataset to SuperChart
like { ...dataset, verbose_map: verboseMap }
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And since that hook might have a more generic purpose, we could place it in src/hooks
or src/utils
instead of drill by directories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
SUMMARY
verbose_map
field is missing in/v1/dataset/{id}
response, the chart in drill by modal doesn't have access to verbose column and metric names.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION