You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/07/13 02:33:07 UTC

[GitHub] [superset] john-bodley opened a new pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices

john-bodley opened a new pull request #15648:
URL: https://github.com/apache/superset/pull/15648


   ### SUMMARY
   
   We noticed that large dashboards were repeatedly requesting the same datasource metadata regardless of whether multiple slices shared the same datasource. This PR updates the logic (though less pretty) to ensure that when building the mapping of slices by datasource [this](https://github.com/apache/superset/blob/master/superset/models/slice.py#L120-L123) query isn't called unnecessarily. 
   
   The solution was to merely create a dictionary which is _equivalently_ keyed by `(datasource_type, datasource_id)`  from which the datasource object is only fetched once.
   
   ### TESTING INSTRUCTIONS
   
   Tested with `SQLALCHEMY_ECHO = True` and confirmed that the number of database queries was significantly reduced.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] 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] john-bodley commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668775930



##########
File path: superset/models/dashboard.py
##########
@@ -246,13 +256,24 @@ def data(self) -> Dict[str, Any]:
         unless=lambda: not is_feature_enabled("DASHBOARD_CACHE"),
     )
     def datasets_trimmed_for_slices(self) -> List[Dict[str, Any]]:
-        datasource_slices = utils.indexed(self.slices, "datasource")
-        return [
-            # Filter out unneeded fields from the datasource payload
-            datasource.data_for_slices(slices)
-            for datasource, slices in datasource_slices.items()
-            if datasource
-        ]
+        # Verbose but efficient database enumeration of dashboard datasources.
+        datasource_slices: Dict[
+            Tuple[Type["BaseDatasource"], int], Set[Slice]
+        ] = defaultdict(set)
+
+        for slc in self.slices:
+            datasource_slices[(slc.cls_model, slc.datasource_id)].add(slc)
+
+        result: List[Dict[str, Any]] = []
+
+        for (cls_model, datasource_id), slices in datasource_slices.items():
+            datasource = db.session.query(cls_model).filter_by(id=datasource_id).first()
+
+            if datasource:
+                # Filter out unneeded fields from the datasource payload
+                result.append(datasource.data_for_slices(slices))
+
+        return result

Review comment:
       @ktmud I’m sure there’s further optimizations, especially if all the data is not required. This PR merely reduces the number of database requests. I was going to address the other issues in a future PR.




-- 
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] john-bodley commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668774483



##########
File path: superset/models/dashboard.py
##########
@@ -171,8 +172,17 @@ def url(self) -> str:
 
     @property
     def datasources(self) -> Set[BaseDatasource]:
-        # pylint: disable=using-constant-test
-        return {slc.datasource for slc in self.slices if slc.datasource}
+        # Verbose but efficient database enumeration of dashboard datasources.
+        datasources = {(slc.cls_model, slc.datasource_id) for slc in self.slices}
+        result: Set[BaseDatasource] = set()
+
+        for cls_model, datasource_id in datasources:
+            datasource = db.session.query(cls_model).filter_by(id=datasource_id).first()
+
+            if datasource:
+                result.add(datasource)
+
+        return result

Review comment:
       Agreed @ktmud. I initially didn’t refactor this method and thus did a somewhat copy-and-paste from the other refactor. 




-- 
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] edited a comment on pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15648:
URL: https://github.com/apache/superset/pull/15648#issuecomment-878786402


   # [Codecov](https://codecov.io/gh/apache/superset/pull/15648?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 [#15648](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (027890d) into [master](https://codecov.io/gh/apache/superset/commit/f39582c900fda9e26d3ca48287d516f44a002aae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f39582c) will **decrease** coverage by `0.23%`.
   > The diff coverage is `76.00%`.
   
   > :exclamation: Current head 027890d differs from pull request most recent head a82cb39. Consider uploading reports for the commit a82cb39 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/15648/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #15648      +/-   ##
   ==========================================
   - Coverage   76.99%   76.76%   -0.24%     
   ==========================================
     Files         978      978              
     Lines       51486    51501      +15     
     Branches     6950     6950              
   ==========================================
   - Hits        39642    39534     -108     
   - Misses      11620    11743     +123     
     Partials      224      224              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `81.55% <100.00%> (-0.01%)` | :arrow_down: |
   | postgres | `81.58% <100.00%> (-0.01%)` | :arrow_down: |
   | presto | `?` | |
   | python | `81.66% <100.00%> (-0.46%)` | :arrow_down: |
   | sqlite | `81.18% <100.00%> (-0.01%)` | :arrow_down: |
   
   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/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.57% <14.28%> (+0.15%)` | :arrow_up: |
   | [superset/models/dashboard.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2Rhc2hib2FyZC5weQ==) | `77.27% <100.00%> (+1.39%)` | :arrow_up: |
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-82.15%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `69.44% <0.00%> (-17.07%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.36% <0.00%> (-6.95%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `81.03% <0.00%> (-1.73%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.26% <0.00%> (-1.65%)` | :arrow_down: |
   | [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `88.12% <0.00%> (-0.78%)` | :arrow_down: |
   | [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `88.16% <0.00%> (-0.40%)` | :arrow_down: |
   | [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `89.79% <0.00%> (-0.27%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f39582c...a82cb39](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] codecov[bot] edited a comment on pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15648:
URL: https://github.com/apache/superset/pull/15648#issuecomment-878786402


   # [Codecov](https://codecov.io/gh/apache/superset/pull/15648?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 [#15648](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c14b3f7) into [master](https://codecov.io/gh/apache/superset/commit/f39582c900fda9e26d3ca48287d516f44a002aae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f39582c) will **decrease** coverage by `0.28%`.
   > The diff coverage is `56.09%`.
   
   > :exclamation: Current head c14b3f7 differs from pull request most recent head 18c69d6. Consider uploading reports for the commit 18c69d6 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/15648/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #15648      +/-   ##
   ==========================================
   - Coverage   76.99%   76.71%   -0.29%     
   ==========================================
     Files         978      983       +5     
     Lines       51486    51711     +225     
     Branches     6950     6983      +33     
   ==========================================
   + Hits        39642    39668      +26     
   - Misses      11620    11819     +199     
     Partials      224      224              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `81.54% <80.89%> (-0.03%)` | :arrow_down: |
   | postgres | `81.56% <80.89%> (-0.02%)` | :arrow_down: |
   | presto | `?` | |
   | python | `81.65% <82.02%> (-0.47%)` | :arrow_down: |
   | sqlite | `81.17% <82.02%> (-0.02%)` | :arrow_down: |
   
   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/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `34.81% <0.00%> (-0.15%)` | :arrow_down: |
   | [...-frontend/src/explore/components/controls/index.js](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9pbmRleC5qcw==) | `100.00% <ø> (ø)` | |
   | [...set-frontend/src/views/CRUD/data/database/types.ts](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS90eXBlcy50cw==) | `100.00% <ø> (ø)` | |
   | [superset/databases/api.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `92.95% <ø> (ø)` | |
   | [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `75.01% <0.00%> (-0.11%)` | :arrow_down: |
   | [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.57% <14.28%> (+0.15%)` | :arrow_up: |
   | [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `53.74% <22.22%> (-1.31%)` | :arrow_down: |
   | [...FormattingControl/ConditionalFormattingControl.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db25kaXRpb25hbEZvcm1hdHRpbmdDb250cm9sL0NvbmRpdGlvbmFsRm9ybWF0dGluZ0NvbnRyb2wudHN4) | `22.41% <22.41%> (ø)` | |
   | [...onalFormattingControl/FormattingPopoverContent.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db25kaXRpb25hbEZvcm1hdHRpbmdDb250cm9sL0Zvcm1hdHRpbmdQb3BvdmVyQ29udGVudC50c3g=) | `33.33% <33.33%> (ø)` | |
   | [...ConditionalFormattingControl/FormattingPopover.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db25kaXRpb25hbEZvcm1hdHRpbmdDb250cm9sL0Zvcm1hdHRpbmdQb3BvdmVyLnRzeA==) | `36.36% <36.36%> (ø)` | |
   | ... and [32 more](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f39582c...18c69d6](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] codecov[bot] commented on pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #15648:
URL: https://github.com/apache/superset/pull/15648#issuecomment-878786402


   # [Codecov](https://codecov.io/gh/apache/superset/pull/15648?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 [#15648](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (027890d) into [master](https://codecov.io/gh/apache/superset/commit/f39582c900fda9e26d3ca48287d516f44a002aae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f39582c) will **decrease** coverage by `0.27%`.
   > The diff coverage is `76.00%`.
   
   > :exclamation: Current head 027890d differs from pull request most recent head a82cb39. Consider uploading reports for the commit a82cb39 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/15648/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #15648      +/-   ##
   ==========================================
   - Coverage   76.99%   76.72%   -0.28%     
   ==========================================
     Files         978      978              
     Lines       51486    51461      -25     
     Branches     6950     6950              
   ==========================================
   - Hits        39642    39482     -160     
   - Misses      11620    11755     +135     
     Partials      224      224              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `81.55% <100.00%> (-0.01%)` | :arrow_down: |
   | postgres | `?` | |
   | presto | `?` | |
   | python | `81.59% <100.00%> (-0.53%)` | :arrow_down: |
   | sqlite | `81.18% <100.00%> (-0.01%)` | :arrow_down: |
   
   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/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.57% <14.28%> (+0.15%)` | :arrow_up: |
   | [superset/models/dashboard.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2Rhc2hib2FyZC5weQ==) | `75.11% <100.00%> (-0.77%)` | :arrow_down: |
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-82.15%)` | :arrow_down: |
   | [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `69.44% <0.00%> (-17.07%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.36% <0.00%> (-6.95%)` | :arrow_down: |
   | [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `94.05% <0.00%> (-2.98%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `81.03% <0.00%> (-1.73%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.24% <0.00%> (-1.66%)` | :arrow_down: |
   | ... and [15 more](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f39582c...a82cb39](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] ktmud commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668470726



##########
File path: superset/models/dashboard.py
##########
@@ -246,13 +256,24 @@ def data(self) -> Dict[str, Any]:
         unless=lambda: not is_feature_enabled("DASHBOARD_CACHE"),
     )
     def datasets_trimmed_for_slices(self) -> List[Dict[str, Any]]:
-        datasource_slices = utils.indexed(self.slices, "datasource")
-        return [
-            # Filter out unneeded fields from the datasource payload
-            datasource.data_for_slices(slices)
-            for datasource, slices in datasource_slices.items()
-            if datasource
-        ]
+        # Verbose but efficient database enumeration of dashboard datasources.
+        datasource_slices: Dict[
+            Tuple[Type["BaseDatasource"], int], Set[Slice]
+        ] = defaultdict(set)
+
+        for slc in self.slices:
+            datasource_slices[(slc.cls_model, slc.datasource_id)].add(slc)
+
+        result: List[Dict[str, Any]] = []
+
+        for (cls_model, datasource_id), slices in datasource_slices.items():
+            datasource = db.session.query(cls_model).filter_by(id=datasource_id).first()
+
+            if datasource:
+                # Filter out unneeded fields from the datasource payload
+                result.append(datasource.data_for_slices(slices))
+
+        return result

Review comment:
       For this method, though, I believe the real bottle neck is in `dashboard.data_for_slices` where [these two lines](https://github.com/airbnb/superset-fork/blob/b295c6ad43798d33b15af7b34ab4a4ff1fd2a903/superset/connectors/base/models.py#L263-L264) takes a lot of time. 
   
   Generating the `datasource.data` dictionary for the one big datasource we  have takes about ~1.2s (80% of the query time). There may be two solutions here:
   
   1. Refactor `datasource.data(...)` and `datasource.data_for_slices(...)` to load metrics & columns lazily.
   2. Creating cache for `Datasource.data`,  clearing the cache using SQLA hook in a similar fashion to [clear_dashboard_cache](https://github.com/airbnb/superset-fork/blob/f24264ccdc6bf6ab95f1969295dd9ba3612e9054/superset/models/dashboard.py#L405).
   
   

##########
File path: superset/models/dashboard.py
##########
@@ -171,8 +172,17 @@ def url(self) -> str:
 
     @property
     def datasources(self) -> Set[BaseDatasource]:
-        # pylint: disable=using-constant-test
-        return {slc.datasource for slc in self.slices if slc.datasource}
+        # Verbose but efficient database enumeration of dashboard datasources.
+        datasources = {(slc.cls_model, slc.datasource_id) for slc in self.slices}
+        result: Set[BaseDatasource] = set()
+
+        for cls_model, datasource_id in datasources:
+            datasource = db.session.query(cls_model).filter_by(id=datasource_id).first()
+
+            if datasource:
+                result.add(datasource)
+
+        return result

Review comment:
       here's another way (probably more efficient):
   
   ```python
       @property
       def datasources(self) -> Set[BaseDatasource]:
           datasources = defaultdict(set)
           for slc in self.slices:
               datasources[slc.cls_model].add(slc.datasource_id)
           result = []
           for cls_model, ids in datasources.items():
               result += db.session.query(cls_model).filter(cls_model.id._in(ids)).all()
           return set(result)
   ```
   
   On one of my test dashboard, this returns an average query time of `0.028s`.
   <img src="https://user-images.githubusercontent.com/335541/125401010-750f3680-e367-11eb-9c1a-d4ff680ba986.png" width="300" />
   
   
   While the original method:
   
   ```python
       @property
       def datasources(self) -> Set[BaseDatasource]:
           # pylint: disable=using-constant-test
           return {slc.datasource for slc in self.slices if slc.datasource}
   ```
   
   costs about `0.5s` (17x slower).
   
   <img src="https://user-images.githubusercontent.com/335541/125400609-f4503a80-e366-11eb-9779-f4feee779f84.png" width="200">
   
   The code in this PR is about 2x slower than my optimized version:
   <img src="https://user-images.githubusercontent.com/335541/125401154-a7209880-e367-11eb-922f-006f310dfac3.png" width="200">
   
   
   




-- 
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] edited a comment on pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15648:
URL: https://github.com/apache/superset/pull/15648#issuecomment-878786402


   # [Codecov](https://codecov.io/gh/apache/superset/pull/15648?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 [#15648](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c14b3f7) into [master](https://codecov.io/gh/apache/superset/commit/f39582c900fda9e26d3ca48287d516f44a002aae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f39582c) will **decrease** coverage by `0.28%`.
   > The diff coverage is `56.09%`.
   
   > :exclamation: Current head c14b3f7 differs from pull request most recent head 18c69d6. Consider uploading reports for the commit 18c69d6 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/15648/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #15648      +/-   ##
   ==========================================
   - Coverage   76.99%   76.71%   -0.29%     
   ==========================================
     Files         978      983       +5     
     Lines       51486    51711     +225     
     Branches     6950     6983      +33     
   ==========================================
   + Hits        39642    39668      +26     
   - Misses      11620    11819     +199     
     Partials      224      224              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `81.54% <80.89%> (-0.03%)` | :arrow_down: |
   | postgres | `81.56% <80.89%> (-0.03%)` | :arrow_down: |
   | presto | `?` | |
   | python | `81.65% <82.02%> (-0.47%)` | :arrow_down: |
   | sqlite | `81.17% <82.02%> (-0.02%)` | :arrow_down: |
   
   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/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `34.81% <0.00%> (-0.15%)` | :arrow_down: |
   | [...-frontend/src/explore/components/controls/index.js](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9pbmRleC5qcw==) | `100.00% <ø> (ø)` | |
   | [...set-frontend/src/views/CRUD/data/database/types.ts](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS90eXBlcy50cw==) | `100.00% <ø> (ø)` | |
   | [superset/databases/api.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `92.95% <ø> (ø)` | |
   | [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `75.01% <0.00%> (-0.11%)` | :arrow_down: |
   | [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.57% <14.28%> (+0.15%)` | :arrow_up: |
   | [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `53.74% <22.22%> (-1.31%)` | :arrow_down: |
   | [...FormattingControl/ConditionalFormattingControl.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db25kaXRpb25hbEZvcm1hdHRpbmdDb250cm9sL0NvbmRpdGlvbmFsRm9ybWF0dGluZ0NvbnRyb2wudHN4) | `22.41% <22.41%> (ø)` | |
   | [...onalFormattingControl/FormattingPopoverContent.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db25kaXRpb25hbEZvcm1hdHRpbmdDb250cm9sL0Zvcm1hdHRpbmdQb3BvdmVyQ29udGVudC50c3g=) | `33.33% <33.33%> (ø)` | |
   | [...ConditionalFormattingControl/FormattingPopover.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db25kaXRpb25hbEZvcm1hdHRpbmdDb250cm9sL0Zvcm1hdHRpbmdQb3BvdmVyLnRzeA==) | `36.36% <36.36%> (ø)` | |
   | ... and [32 more](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f39582c...18c69d6](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] villebro commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r669837659



##########
File path: superset/models/dashboard.py
##########
@@ -171,8 +172,21 @@ def url(self) -> str:
 
     @property
     def datasources(self) -> Set[BaseDatasource]:
-        # pylint: disable=using-constant-test
-        return {slc.datasource for slc in self.slices if slc.datasource}

Review comment:
       @john-bodley yes, that sounds like a perfect solution 👍  It would be nice if we had a mechanism that could somehow automatically surface these types of perf issues (=hitting the metadata db multiple times for the same object). @kgabryje successfully used a React library called [WDYR - Why Did You Render](https://www.npmjs.com/package/@welldone-software/why-did-you-render) that warns about potentially avoidable React rerenders - It'd be great to have something similar for SqlAlchemy ORM. Alternatively we could build a wrapper around `db.session.query().filter()` that caches on both the class passed to `query() and the kwargs passed to `filter()`.

##########
File path: superset/models/dashboard.py
##########
@@ -171,8 +172,21 @@ def url(self) -> str:
 
     @property
     def datasources(self) -> Set[BaseDatasource]:
-        # pylint: disable=using-constant-test
-        return {slc.datasource for slc in self.slices if slc.datasource}

Review comment:
       @john-bodley yes, that sounds like a perfect solution 👍  It would be nice if we had a mechanism that could somehow automatically surface these types of perf issues (=hitting the metadata db multiple times for the same object). @kgabryje successfully used a React library called [WDYR - Why Did You Render](https://www.npmjs.com/package/@welldone-software/why-did-you-render) that warns about potentially avoidable React rerenders - It'd be great to have something similar for SqlAlchemy ORM. Alternatively we could build a wrapper around `db.session.query().filter()` that caches on both the class passed to `query()` and the kwargs passed to `filter()`.




-- 
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] john-bodley commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668382831



##########
File path: superset/models/dashboard.py
##########
@@ -246,12 +247,27 @@ def data(self) -> Dict[str, Any]:
         unless=lambda: not is_feature_enabled("DASHBOARD_CACHE"),
     )
     def datasets_trimmed_for_slices(self) -> List[Dict[str, Any]]:
-        datasource_slices = utils.indexed(self.slices, "datasource")
+
+        # Efficient manner of processing datasources associated with a dashboard. Rather
+        # than keying by the convenient `Slice.datasource` model (which results in a
+        # per slice database query), we lazily request datasource after deduping given
+        # that the same datasource could appear multiple times within a dashboard.
+        datasource_slices: Dict[
+            Tuple(Type["BaseDatasource"], int), Slice
+        ] = defaultdict(set)
+
+        for slc in self.slices:
+            datasource_slices[(slc.cls_model, slc.datasource_id)].add(slc)
+
         return [
             # Filter out unneeded fields from the datasource payload
             datasource.data_for_slices(slices)
-            for datasource, slices in datasource_slices.items()
-            if datasource
+            for (cls_model, datasource_id), slices in datasource_slices.items()
+            if (
+                datasource := db.session.query(cls_model)

Review comment:
       Loving the walrus.




-- 
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] john-bodley merged pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
john-bodley merged pull request #15648:
URL: https://github.com/apache/superset/pull/15648


   


-- 
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] john-bodley commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668775930



##########
File path: superset/models/dashboard.py
##########
@@ -246,13 +256,24 @@ def data(self) -> Dict[str, Any]:
         unless=lambda: not is_feature_enabled("DASHBOARD_CACHE"),
     )
     def datasets_trimmed_for_slices(self) -> List[Dict[str, Any]]:
-        datasource_slices = utils.indexed(self.slices, "datasource")
-        return [
-            # Filter out unneeded fields from the datasource payload
-            datasource.data_for_slices(slices)
-            for datasource, slices in datasource_slices.items()
-            if datasource
-        ]
+        # Verbose but efficient database enumeration of dashboard datasources.
+        datasource_slices: Dict[
+            Tuple[Type["BaseDatasource"], int], Set[Slice]
+        ] = defaultdict(set)
+
+        for slc in self.slices:
+            datasource_slices[(slc.cls_model, slc.datasource_id)].add(slc)
+
+        result: List[Dict[str, Any]] = []
+
+        for (cls_model, datasource_id), slices in datasource_slices.items():
+            datasource = db.session.query(cls_model).filter_by(id=datasource_id).first()
+
+            if datasource:
+                # Filter out unneeded fields from the datasource payload
+                result.append(datasource.data_for_slices(slices))
+
+        return result

Review comment:
       @ktmud I’m sure there’s further optimizations, especially if all the data is not required. This PR merely reduces the number of database requests.




-- 
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] john-bodley commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668935398



##########
File path: superset/models/dashboard.py
##########
@@ -246,13 +260,24 @@ def data(self) -> Dict[str, Any]:
         unless=lambda: not is_feature_enabled("DASHBOARD_CACHE"),
     )
     def datasets_trimmed_for_slices(self) -> List[Dict[str, Any]]:
-        datasource_slices = utils.indexed(self.slices, "datasource")
-        return [
-            # Filter out unneeded fields from the datasource payload
-            datasource.data_for_slices(slices)
-            for datasource, slices in datasource_slices.items()
-            if datasource
-        ]
+        # Verbose but efficient database enumeration of dashboard datasources.
+        slices_by_datasource: Dict[
+            Tuple[Type["BaseDatasource"], int], Set[Slice]
+        ] = defaultdict(set)
+
+        for slc in self.slices:
+            slices_by_datasource[(slc.cls_model, slc.datasource_id)].add(slc)
+
+        result: List[Dict[str, Any]] = []
+
+        for (cls_model, datasource_id), slices in slices_by_datasource.items():
+            datasource = db.session.query(cls_model).filter_by(id=datasource_id).first()

Review comment:
       @etr2460 probably `one_or_none()` is better (as `one()` will raise). This was just a copy-and-paste from the `Slice` class. I've updated the logic.




-- 
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] john-bodley commented on pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #15648:
URL: https://github.com/apache/superset/pull/15648#issuecomment-879114928


   @ktmud I've addressed your comments.


-- 
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] john-bodley commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668935398



##########
File path: superset/models/dashboard.py
##########
@@ -246,13 +260,24 @@ def data(self) -> Dict[str, Any]:
         unless=lambda: not is_feature_enabled("DASHBOARD_CACHE"),
     )
     def datasets_trimmed_for_slices(self) -> List[Dict[str, Any]]:
-        datasource_slices = utils.indexed(self.slices, "datasource")
-        return [
-            # Filter out unneeded fields from the datasource payload
-            datasource.data_for_slices(slices)
-            for datasource, slices in datasource_slices.items()
-            if datasource
-        ]
+        # Verbose but efficient database enumeration of dashboard datasources.
+        slices_by_datasource: Dict[
+            Tuple[Type["BaseDatasource"], int], Set[Slice]
+        ] = defaultdict(set)
+
+        for slc in self.slices:
+            slices_by_datasource[(slc.cls_model, slc.datasource_id)].add(slc)
+
+        result: List[Dict[str, Any]] = []
+
+        for (cls_model, datasource_id), slices in slices_by_datasource.items():
+            datasource = db.session.query(cls_model).filter_by(id=datasource_id).first()

Review comment:
       @etr2460 probably `one_or_none()` is better (as `one()` will raise). This was just a copy-and-paste from the `Slice` class. 




-- 
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] edited a comment on pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #15648:
URL: https://github.com/apache/superset/pull/15648#issuecomment-878786402


   # [Codecov](https://codecov.io/gh/apache/superset/pull/15648?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 [#15648](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c14b3f7) into [master](https://codecov.io/gh/apache/superset/commit/f39582c900fda9e26d3ca48287d516f44a002aae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f39582c) will **decrease** coverage by `0.19%`.
   > The diff coverage is `56.09%`.
   
   > :exclamation: Current head c14b3f7 differs from pull request most recent head 18c69d6. Consider uploading reports for the commit 18c69d6 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/15648/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #15648      +/-   ##
   ==========================================
   - Coverage   76.99%   76.80%   -0.20%     
   ==========================================
     Files         978      983       +5     
     Lines       51486    51711     +225     
     Branches     6950     6983      +33     
   ==========================================
   + Hits        39642    39715      +73     
   - Misses      11620    11772     +152     
     Partials      224      224              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `81.54% <80.89%> (-0.03%)` | :arrow_down: |
   | postgres | `81.56% <80.89%> (-0.03%)` | :arrow_down: |
   | presto | `81.26% <82.02%> (-0.03%)` | :arrow_down: |
   | python | `81.83% <82.02%> (-0.29%)` | :arrow_down: |
   | sqlite | `81.17% <82.02%> (-0.02%)` | :arrow_down: |
   
   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/15648?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `34.81% <0.00%> (-0.15%)` | :arrow_down: |
   | [...-frontend/src/explore/components/controls/index.js](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9pbmRleC5qcw==) | `100.00% <ø> (ø)` | |
   | [...set-frontend/src/views/CRUD/data/database/types.ts](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS90eXBlcy50cw==) | `100.00% <ø> (ø)` | |
   | [superset/databases/api.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `92.95% <ø> (ø)` | |
   | [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `75.01% <0.00%> (-0.11%)` | :arrow_down: |
   | [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `47.57% <14.28%> (+0.15%)` | :arrow_up: |
   | [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `53.74% <22.22%> (-1.31%)` | :arrow_down: |
   | [...FormattingControl/ConditionalFormattingControl.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db25kaXRpb25hbEZvcm1hdHRpbmdDb250cm9sL0NvbmRpdGlvbmFsRm9ybWF0dGluZ0NvbnRyb2wudHN4) | `22.41% <22.41%> (ø)` | |
   | [...onalFormattingControl/FormattingPopoverContent.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db25kaXRpb25hbEZvcm1hdHRpbmdDb250cm9sL0Zvcm1hdHRpbmdQb3BvdmVyQ29udGVudC50c3g=) | `33.33% <33.33%> (ø)` | |
   | [...ConditionalFormattingControl/FormattingPopover.tsx](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db25kaXRpb25hbEZvcm1hdHRpbmdDb250cm9sL0Zvcm1hdHRpbmdQb3BvdmVyLnRzeA==) | `36.36% <36.36%> (ø)` | |
   | ... and [29 more](https://codecov.io/gh/apache/superset/pull/15648/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f39582c...18c69d6](https://codecov.io/gh/apache/superset/pull/15648?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] john-bodley commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r669788272



##########
File path: superset/models/dashboard.py
##########
@@ -171,8 +172,21 @@ def url(self) -> str:
 
     @property
     def datasources(self) -> Set[BaseDatasource]:
-        # pylint: disable=using-constant-test
-        return {slc.datasource for slc in self.slices if slc.datasource}

Review comment:
       @villebro per your point we could also use [`functools.lru_cache`](https://docs.python.org/3/library/functools.html#functools.lru_cache) to cache the `datasource` property for the lifetime of the request. It's likely safer than using `@cache.memoize(...)` as there's likely little concern that the data will be stale (within the context of a request).




-- 
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] etr2460 commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668917642



##########
File path: superset/models/dashboard.py
##########
@@ -246,13 +260,24 @@ def data(self) -> Dict[str, Any]:
         unless=lambda: not is_feature_enabled("DASHBOARD_CACHE"),
     )
     def datasets_trimmed_for_slices(self) -> List[Dict[str, Any]]:
-        datasource_slices = utils.indexed(self.slices, "datasource")
-        return [
-            # Filter out unneeded fields from the datasource payload
-            datasource.data_for_slices(slices)
-            for datasource, slices in datasource_slices.items()
-            if datasource
-        ]
+        # Verbose but efficient database enumeration of dashboard datasources.
+        slices_by_datasource: Dict[
+            Tuple[Type["BaseDatasource"], int], Set[Slice]
+        ] = defaultdict(set)
+
+        for slc in self.slices:
+            slices_by_datasource[(slc.cls_model, slc.datasource_id)].add(slc)
+
+        result: List[Dict[str, Any]] = []
+
+        for (cls_model, datasource_id), slices in slices_by_datasource.items():
+            datasource = db.session.query(cls_model).filter_by(id=datasource_id).first()

Review comment:
       should this call `one()` instead?




-- 
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] john-bodley commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668383063



##########
File path: superset/models/dashboard.py
##########
@@ -289,7 +305,8 @@ def clear_cache_for_slice(cls, slice_id: int) -> None:
     @debounce(0.1)
     def clear_cache_for_datasource(cls, datasource_id: int) -> None:
         filter_query = select(
-            [dashboard_slices.c.dashboard_id], distinct=True,
+            [dashboard_slices.c.dashboard_id],

Review comment:
       Black.




-- 
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] john-bodley commented on a change in pull request #15648: perf: Refactor Dashboard.datasets_trimmed_for_slices et al.

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #15648:
URL: https://github.com/apache/superset/pull/15648#discussion_r668382831



##########
File path: superset/models/dashboard.py
##########
@@ -246,12 +247,27 @@ def data(self) -> Dict[str, Any]:
         unless=lambda: not is_feature_enabled("DASHBOARD_CACHE"),
     )
     def datasets_trimmed_for_slices(self) -> List[Dict[str, Any]]:
-        datasource_slices = utils.indexed(self.slices, "datasource")
+
+        # Efficient manner of processing datasources associated with a dashboard. Rather
+        # than keying by the convenient `Slice.datasource` model (which results in a
+        # per slice database query), we lazily request datasource after deduping given
+        # that the same datasource could appear multiple times within a dashboard.
+        datasource_slices: Dict[
+            Tuple(Type["BaseDatasource"], int), Slice
+        ] = defaultdict(set)
+
+        for slc in self.slices:
+            datasource_slices[(slc.cls_model, slc.datasource_id)].add(slc)
+
         return [
             # Filter out unneeded fields from the datasource payload
             datasource.data_for_slices(slices)
-            for datasource, slices in datasource_slices.items()
-            if datasource
+            for (cls_model, datasource_id), slices in datasource_slices.items()
+            if (
+                datasource := db.session.query(cls_model)

Review comment:
       Loving the walrus. Sadly I later found out we needed to support Python 3.7.




-- 
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