You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "lilykuang (via GitHub)" <gi...@apache.org> on 2023/04/12 02:08:01 UTC
[GitHub] [superset] lilykuang opened a new pull request, #23655: feat: add verbose map to /dataset/:id endpoint [WIP]
lilykuang opened a new pull request, #23655:
URL: https://github.com/apache/superset/pull/23655
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #23655: feat: add verbose map to get /dataset/ endpoint
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #23655:
URL: https://github.com/apache/superset/pull/23655#issuecomment-1512722149
> Btw, as the elements of both `columns` and `metrics` contain `verbose_name`, could it potentially be more simple to just create a `useVerboseName(dataset)` hook in the frontend that extracts the `verboseMap` from the requested `dataset`?
+1 to that! I had no clue that each `column`/`metric` contains its `verbose_name`, I assumed we can only take them from `verbose_names` on top level of dataset endpoint response
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #23655: feat: add verbose map to get /dataset/ endpoint
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23655:
URL: https://github.com/apache/superset/pull/23655#issuecomment-1507468787
## [Codecov](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23655](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (44dfddb) into [master](https://codecov.io/gh/apache/superset/commit/a5b6ccc1ec98cce297d5f8579c7704668fe698f3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a5b6ccc) will **decrease** coverage by `11.26%`.
> The diff coverage is `76.35%`.
> :exclamation: Current head 44dfddb differs from pull request most recent head 8b2595c. Consider uploading reports for the commit 8b2595c to get more accurate results
```diff
@@ Coverage Diff @@
## master #23655 +/- ##
===========================================
- Coverage 67.94% 56.68% -11.26%
===========================================
Files 1918 1922 +4
Lines 73890 74082 +192
Branches 8058 8101 +43
===========================================
- Hits 50204 41996 -8208
- Misses 21625 30015 +8390
- Partials 2061 2071 +10
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.12% <74.13%> (?)` | |
| python | `59.43% <75.86%> (-23.60%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `53.05% <74.13%> (+0.03%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...d/packages/superset-ui-chart-controls/src/types.ts](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [...ackages/superset-ui-core/src/utils/featureFlags.ts](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZmVhdHVyZUZsYWdzLnRz) | `100.00% <ø> (ø)` | |
| [...s/plugin-chart-echarts/src/Timeseries/constants.ts](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy9jb25zdGFudHMudHM=) | `100.00% <ø> (ø)` | |
| [...gin-chart-echarts/src/Timeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1Qcm9wcy50cw==) | `57.27% <0.00%> (-1.07%)` | :arrow_down: |
| [...tend/plugins/plugin-chart-echarts/src/constants.ts](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvY29uc3RhbnRzLnRz) | `100.00% <ø> (ø)` | |
| [...tend/plugins/plugin-chart-echarts/src/controls.tsx](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvY29udHJvbHMudHN4) | `76.00% <ø> (ø)` | |
| [...ugins/preset-chart-xy/src/components/Line/Line.tsx](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wcmVzZXQtY2hhcnQteHkvc3JjL2NvbXBvbmVudHMvTGluZS9MaW5lLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...preset-chart-xy/src/utils/createMarginSelector.tsx](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wcmVzZXQtY2hhcnQteHkvc3JjL3V0aWxzL2NyZWF0ZU1hcmdpblNlbGVjdG9yLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (ø)` | |
| [...frontend/src/SqlLab/components/SqlEditor/index.jsx](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci9pbmRleC5qc3g=) | `56.68% <0.00%> (-1.92%)` | :arrow_down: |
| ... and [32 more](https://codecov.io/gh/apache/superset/pull/23655?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [297 files with indirect coverage changes](https://codecov.io/gh/apache/superset/pull/23655/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #23655: feat: add verbose map to get /dataset/ endpoint
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #23655:
URL: https://github.com/apache/superset/pull/23655#issuecomment-1518068414
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 `<SuperChart ... />` in `DrillByChart`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #23655: feat: add verbose map to get /dataset/ endpoint
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #23655:
URL: https://github.com/apache/superset/pull/23655#discussion_r1175207504
##########
superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx:
##########
@@ -123,7 +124,16 @@ export const DrillByMenuItems = ({
endpoint: `/api/v1/dataset/${datasetId}`,
})
.then(({ json: { result } }) => {
- setDataset(result);
+ const verbose_map = {};
+ ensureIsArray(result.columns).forEach((column: Column) => {
+ verbose_map[column.column_name] =
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lilykuang commented on a diff in pull request #23655: feat: add verbose map to /dataset/:id endpoint [WIP]
Posted by "lilykuang (via GitHub)" <gi...@apache.org>.
lilykuang commented on code in PR #23655:
URL: https://github.com/apache/superset/pull/23655#discussion_r1164445698
##########
superset/datasets/api.py:
##########
@@ -248,6 +249,59 @@ class DatasetRestApi(BaseSupersetModelRestApi):
list_outer_default_load = True
show_outer_default_load = True
+ @expose("/<int:pk>", methods=["GET"])
+ @protect()
+ @safe
+ def get(self, pk: int, **kwargs: Any) -> Response:
+ """Get a dataset
+ ---
+ get:
+ description: >-
+ Get a dataset
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ description: The dataset id
+ name: pk
+ responses:
+ 200:
+ description: dataset
+ content:
+ application/json:
+ schema:
+ type: object
Review Comment:
thank you @villebro. i will add the full schema
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #23655: feat: add verbose map to /dataset/:id endpoint [WIP]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23655:
URL: https://github.com/apache/superset/pull/23655#discussion_r1164001692
##########
superset/datasets/api.py:
##########
@@ -248,6 +249,59 @@ class DatasetRestApi(BaseSupersetModelRestApi):
list_outer_default_load = True
show_outer_default_load = True
+ @expose("/<int:pk>", methods=["GET"])
+ @protect()
+ @safe
+ def get(self, pk: int, **kwargs: Any) -> Response:
+ """Get a dataset
+ ---
+ get:
+ description: >-
+ Get a dataset
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ description: The dataset id
+ name: pk
+ responses:
+ 200:
+ description: dataset
+ content:
+ application/json:
+ schema:
+ type: object
Review Comment:
Could we add a full reference here? Before this change the schema looks like this:
and after this change it looks like this:
<img width="1108" alt="image" src="https://user-images.githubusercontent.com/33317356/231444464-b3997f48-0e4a-4aac-ac75-d7100961e067.png">
Check here for an example of how to add the schema:
https://github.com/apache/superset/blob/c8fa44e9e904160de705cd643d1df092815348b1/superset/annotation_layers/annotations/api.py#L168-L186
Also, please add `verbose_map` to the schema.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] lilykuang merged pull request #23655: feat: add verbose map to get /dataset/ endpoint
Posted by "lilykuang (via GitHub)" <gi...@apache.org>.
lilykuang merged PR #23655:
URL: https://github.com/apache/superset/pull/23655
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #23655: feat: add verbose map to get /dataset/ endpoint
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23655:
URL: https://github.com/apache/superset/pull/23655#discussion_r1169597164
##########
superset/datasets/schemas.py:
##########
@@ -145,6 +146,34 @@ class DatasetRelatedObjectsResponse(Schema):
dashboards = fields.Nested(DatasetRelatedDashboards)
+class DatasetResponse(Schema):
+ id = fields.Int()
+ cache_timeout = fields.Int()
+ columns = fields.List(fields.Dict())
Review Comment:
Setting this as `fields.List(fields.Dict())` causes it to lose the datatype. This is after this change:
<img width="922" alt="image" src="https://user-images.githubusercontent.com/33317356/232698854-769e8dc0-a3f8-4fd5-bbc9-322cf2f94ced.png">
This is before the change:
<img width="919" alt="image" src="https://user-images.githubusercontent.com/33317356/232699232-dbaab196-9936-4196-921c-da0200eed33b.png">
Here's a schema that I believe is more complete that could probably serve as a better template:
https://github.com/apache/superset/blob/c197bf9e6db85a76d8118a4ec11a83ca2f6aad6d/superset/datasets/schemas.py#L47-L63
##########
superset/datasets/schemas.py:
##########
@@ -145,6 +146,34 @@ class DatasetRelatedObjectsResponse(Schema):
dashboards = fields.Nested(DatasetRelatedDashboards)
+class DatasetResponse(Schema):
+ id = fields.Int()
+ cache_timeout = fields.Int()
+ columns = fields.List(fields.Dict())
+ database = fields.Nested(DatabaseSchema)
+ datasource_type = fields.String()
+ default_endpoint = fields.String()
+ description = fields.String()
+ extra = fields.String()
+ fetch_values_predicate = fields.String()
+ filter_select_enabled = fields.Bool()
+ is_managed_externally = fields.Bool()
+ is_sqllab_view = fields.Bool()
+ kind = fields.String()
+ main_dttm_col = fields.String()
+ metrics = fields.List(fields.Dict())
Review Comment:
same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #23655: feat: add verbose map to /dataset/:id endpoint [WIP]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23655:
URL: https://github.com/apache/superset/pull/23655#discussion_r1164001692
##########
superset/datasets/api.py:
##########
@@ -248,6 +249,59 @@ class DatasetRestApi(BaseSupersetModelRestApi):
list_outer_default_load = True
show_outer_default_load = True
+ @expose("/<int:pk>", methods=["GET"])
+ @protect()
+ @safe
+ def get(self, pk: int, **kwargs: Any) -> Response:
+ """Get a dataset
+ ---
+ get:
+ description: >-
+ Get a dataset
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ description: The dataset id
+ name: pk
+ responses:
+ 200:
+ description: dataset
+ content:
+ application/json:
+ schema:
+ type: object
Review Comment:
Could we add a full schema reference here? Before this change the schema looks like this:
and after this change it looks like this:
<img width="1108" alt="image" src="https://user-images.githubusercontent.com/33317356/231444464-b3997f48-0e4a-4aac-ac75-d7100961e067.png">
Check here for an example of how to add the schema:
https://github.com/apache/superset/blob/c8fa44e9e904160de705cd643d1df092815348b1/superset/annotation_layers/annotations/api.py#L168-L186
Also, please add `verbose_map` to the schema.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #23655: feat: add verbose map to get /dataset/ endpoint
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #23655:
URL: https://github.com/apache/superset/pull/23655#discussion_r1175455244
##########
superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx:
##########
@@ -123,7 +124,16 @@ export const DrillByMenuItems = ({
endpoint: `/api/v1/dataset/${datasetId}`,
})
.then(({ json: { result } }) => {
- setDataset(result);
+ const verbose_map = {};
+ ensureIsArray(result.columns).forEach((column: Column) => {
+ verbose_map[column.column_name] =
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org