feat: Add additional information to datasets list API endpoint #9959

willbarrett · 2020-06-01T23:32:50Z

SUMMARY

Preset would like to add some more information to the new React-based Datasets list view. We're looking to add:

A list of owners in icon form
The number of charts built on the datasource
The number of fields in the datasource
The number of metrics in the datasource
Whether the datasource is physical or virtual (is there SQL)
This PR adds the requested information to the API endpoint.

Changes are to support this UI

TEST PLAN

ADDITIONAL INFORMATION

Reviewers

@dpgaspar @nytai

codecov-commenter · 2020-06-01T23:38:52Z

Codecov Report

Merging #9959 into master will increase coverage by 0.05%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master    #9959      +/-   ##
==========================================
+ Coverage   71.34%   71.39%   +0.05%     
==========================================
  Files         585      585              
  Lines       30889    30968      +79     
  Branches     3237     3261      +24     
==========================================
+ Hits        22037    22110      +73     
- Misses       8743     8749       +6     
  Partials      109      109

Flag	Coverage Δ
#cypress	`54.06% <ø> (+0.14%)`	⬆️
#javascript	`59.39% <ø> (-0.01%)`	⬇️
#python	`71.59% <95.00%> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
superset/datasets/api.py	`92.56% <ø> (ø)`
superset/connectors/base/models.py	`90.10% <94.44%> (+0.29%)`	⬆️
superset/connectors/sqla/models.py	`90.52% <100.00%> (+0.01%)`	⬆️
.../src/dashboard/components/RefreshIntervalModal.jsx	`90.00% <0.00%> (-10.00%)`	⬇️
superset-frontend/src/explore/exploreUtils.js	`81.65% <0.00%> (-1.39%)`	⬇️
...erset-frontend/src/dashboard/components/Header.jsx	`55.47% <0.00%> (-1.24%)`	⬇️
.../src/dashboard/components/gridComponents/Chart.jsx	`88.76% <0.00%> (-1.13%)`	⬇️
...et-frontend/src/dashboard/components/SaveModal.jsx	`81.81% <0.00%> (-1.11%)`	⬇️
superset-frontend/src/explore/controlUtils.js	`96.90% <0.00%> (-0.04%)`	⬇️
superset/app.py	`81.35% <0.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7618ee...b6901a1. Read the comment docs.

willbarrett · 2020-06-02T20:05:58Z

superset/datasets/api.py

        "default_endpoint",
        "explore_url",
+        "kind",
+        "metrics.id",


Required to join the metrics to get a count. Same story with columns.id and slices.id I'm looking into a FAB change to make this less hacky and more performant.

cc @dpgaspar does that result in round trips for each entry in the page? It seems like a bad idea. Is number of cols/metrics necessary here? Do we have a wireframe for context?

This should not result in an N+1, given some recent work that Daniel has done. I'll update the ticket with a screenshot of the design.

In general, adding a count of related records is a common problem for webapps and there are performant solutions. I can see us wanting to display similar information in other places, so I recommend we find a performant way forward with FAB.

@willbarrett is preparing a PR on FAB to add the ability to define a custom SQLAlchemy query for the list endpoint, should be a great new feature

mistercrunch · 2020-06-03T06:25:59Z

superset/connectors/base/models.py

@@ -50,6 +50,9 @@
    "series",
 ]

+DATASOURCE_TYPE_VIRTUAL = "virtual"


Enum! :) http://xion.io/post/code/python-enums-are-ok.html

dpgaspar

Tried it out, and something is off with the backref joins from metric, columns and slices the results get truncated. I'll investigate it since your code seems correct

willbarrett · 2020-06-04T22:18:34Z

I'm hoping to leverage this change in FAB: dpgaspar/Flask-AppBuilder#1386 to improve this PR.

etr2460 · 2020-06-11T22:56:48Z

superset/connectors/base/models.py

@@ -51,6 +52,11 @@
 ]


+class DatasourceKind(Enum):


Most enums i've seen in python have been in SHOUT_CASE, should we be consistent here?

Also, I think if you have this extend str as well, then instead of returning DatasourceKind.virtual.value below you can return str(DatasourceKind.virtual). see https://github.com/apache/incubator-superset/blob/7f6dbf838e4e527e640a002ce20bf5da1abf4a98/superset/errors.py#L24

etr2460 · 2020-06-11T22:57:24Z

superset/connectors/base/models.py

@@ -99,6 +105,17 @@ def slices(self) -> RelationshipProperty:
            ),
        )

+    @property
+    def kind(self) -> str:
+        if self.sql:


maybe return DatasourceKind.virtual.value if self.sql else DatasourceKind.physical.value?

etr2460 · 2020-06-11T22:57:58Z

tests/datasets/api_tests.py

@@ -23,6 +23,7 @@
 import yaml
 from sqlalchemy.sql import func

+import tests.test_app


should this be included here? I don't see any code that references it

Without this inclusion the test file fails when run in isolation.

etr2460 · 2020-06-11T22:58:21Z

superset/connectors/base/models.py

@@ -377,7 +402,7 @@ def get_column(self, column_name: Optional[str]) -> Optional["BaseColumn"]:

    @staticmethod
    def get_fk_many_from_list(
-        object_list: List[Any], fkmany: List[Column], fkmany_class: Type, key_attr: str,
+        object_list: List[Any], fkmany: List[Column], fkmany_class: Type, key_attr: str


hmm, why'd we lose the trailing comma here? Are you on the right version of black?

Let's avoid digging into styling changes that pass CI.

john-bodley · 2020-06-15T06:09:13Z

superset/connectors/sqla/models.py

@@ -35,6 +35,7 @@
    DateTime,
    desc,
    ForeignKey,
+    func,


I'm somewhat perplexed why the imports have changed on this file as they don't seem to be used. Maybe this branch should be rebased against master.

willbarrett · 2020-07-15T19:18:31Z

Closing this until the requried FAB change is incorporated.

Will Barrett added 5 commits May 21, 2020 13:15

Make datasets API tests run in isolation

0c647b5

Ignore cover folder for HTML coverage reports

4a8fabe

Add slice_count to datasets list endpoint

477738d

Add column_count, metric_countand owners to datasets list endpoint

fe3ec26

Merge branch 'master' into wbarrett/add-to-datsets-list

3733733

superset-github-bot bot added the preset-io label Jun 1, 2020

pull-request-size bot added the size/M label Jun 1, 2020

dpgaspar changed the title ~~feature: Add additional information to datasets list API endpoint~~ feat: Add additional information to datasets list API endpoint Jun 2, 2020

Add physical/virtual flag and fix lint

66273b5

willbarrett closed this Jun 2, 2020

willbarrett reopened this Jun 2, 2020

willbarrett marked this pull request as ready for review June 2, 2020 20:04

willbarrett commented Jun 2, 2020

View reviewed changes

mistercrunch reviewed Jun 3, 2020

View reviewed changes

Use enum.Enum

b6901a1

dpgaspar requested changes Jun 4, 2020

View reviewed changes

willbarrett mentioned this pull request Jun 9, 2020

feat: Add owners and physical/virtual status to Datasets API list endpoint #10025

Merged

6 tasks

etr2460 reviewed Jun 11, 2020

View reviewed changes

john-bodley reviewed Jun 15, 2020

View reviewed changes

willbarrett closed this Jul 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add additional information to datasets list API endpoint #9959

feat: Add additional information to datasets list API endpoint #9959

willbarrett commented Jun 1, 2020 •

edited

codecov-commenter commented Jun 1, 2020 •

edited

willbarrett Jun 2, 2020 •

edited

mistercrunch Jun 3, 2020

willbarrett Jun 3, 2020

willbarrett Jun 3, 2020

dpgaspar Jun 4, 2020

mistercrunch Jun 3, 2020

dpgaspar left a comment •

edited

willbarrett commented Jun 4, 2020

etr2460 Jun 11, 2020

etr2460 Jun 11, 2020

etr2460 Jun 11, 2020

willbarrett Jun 12, 2020

etr2460 Jun 11, 2020

willbarrett Jun 12, 2020

john-bodley Jun 15, 2020

willbarrett commented Jul 15, 2020

feat: Add additional information to datasets list API endpoint #9959

feat: Add additional information to datasets list API endpoint #9959

Conversation

willbarrett commented Jun 1, 2020 • edited

SUMMARY

Changes are to support this UI

TEST PLAN

ADDITIONAL INFORMATION

Reviewers

codecov-commenter commented Jun 1, 2020 • edited

Codecov Report

willbarrett Jun 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar left a comment • edited

Choose a reason for hiding this comment

willbarrett commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willbarrett commented Jul 15, 2020

willbarrett commented Jun 1, 2020 •

edited

codecov-commenter commented Jun 1, 2020 •

edited

willbarrett Jun 2, 2020 •

edited

dpgaspar left a comment •

edited