You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2023/02/17 21:23:31 UTC
[GitHub] [superset] john-bodley opened a new pull request, #23113: chore(sqla): Performance tradeoff with eager loading
john-bodley opened a new pull request, #23113:
URL: https://github.com/apache/superset/pull/23113
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
https://github.com/apache/superset/pull/22413 introduced logic for eager loading of the columns/metrics (as well as their back references) for a dataset, however the eagerness comes at a cost, i.e., the RESTful API GET `/api/v1/explore` endpoint is less performant given that all when fetching a slice or dataset the columns/metrics relationships are eagerly loaded.
This PR provides a tradeoff where we lazily load the columns/metrics relationships which negatively impacts the `/api/v1/dataset` endpoint but speeds up the `/api/v1/explore` endpoint (the later is used significantly more than the former). The TL;DR is the proposed behavior is more akin to how objects were lazy loaded prior to https://github.com/apache/superset/pull/22413 but there is significant performance improvements by not leveraging the `joined` [relationship](https://docs.sqlalchemy.org/en/14/orm/relationship_api.html#sqlalchemy.orm.relationship).
Specifically for Airbnb's uber dataset the results are:
| Endpoint | Current | Proposed |
| ---------------- | -------- | --------- |
| `/api/v1/dataset` | ~ 7 s | ~ 26 s† |
| `/api/v1/explore` | ~ 16 s | ~ 5 s |
† Note even though this is slower it is significantly faster than prior to https://github.com/apache/superset/pull/22413 which used to time out. Furthermore a significantly cost is in the (now) lazy loading of the back references, i.e., `metric.table` which likely could be refactored in code.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
CI and local performance testing.
### 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] john-bodley commented on pull request #23113: chore(sqla): Address performance tradeoff with eager loading
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #23113:
URL: https://github.com/apache/superset/pull/23113#issuecomment-1435313621
@ktmud regarding,
> Do you think it'd worth explicit set the lazy behavior even though it is the default?
I'm kind of on the fence about this, and I did contemplate it. I think generally it's good/standard practice not to define functional arguments which are the default. Granted defaults can change, but it does/can result in code bloat.
--
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 #23113: chore(sqla): Address performance tradeoff with eager loading
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #23113:
URL: https://github.com/apache/superset/pull/23113
--
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 pull request #23113: chore(sqla): Address performance tradeoff with eager loading
Posted by "ktmud (via GitHub)" <gi...@apache.org>.
ktmud commented on PR #23113:
URL: https://github.com/apache/superset/pull/23113#issuecomment-1435300157
Thanks for the detailed explanation! Do you think it'd worth explicit set the lazy behavior even though it is the default?
--
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 #23113: chore(sqla): Performance tradeoff with eager loading
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23113:
URL: https://github.com/apache/superset/pull/23113#issuecomment-1435286572
# [Codecov](https://codecov.io/gh/apache/superset/pull/23113?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 [#23113](https://codecov.io/gh/apache/superset/pull/23113?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2306aa4) into [master](https://codecov.io/gh/apache/superset/commit/dec6ac49f6e60f3cb3bcda25694b3b9dcf2f41cc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dec6ac4) will **decrease** coverage by `0.71%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #23113 +/- ##
==========================================
- Coverage 67.54% 66.84% -0.71%
==========================================
Files 1881 1881
Lines 72396 72396
Branches 7882 7882
==========================================
- Hits 48902 48393 -509
- Misses 21472 21981 +509
Partials 2022 2022
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.76% <ø> (ø)` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.67% <ø> (ø)` | |
| python | `80.81% <ø> (-1.47%)` | :arrow_down: |
| sqlite | `76.90% <ø> (ø)` | |
| unit | `52.53% <ø> (ø)` | |
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/23113?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/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/23113?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.77% <ø> (-0.60%)` | :arrow_down: |
| [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/23113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
| [superset/datasets/commands/bulk\_delete.py](https://codecov.io/gh/apache/superset/pull/23113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvYnVsa19kZWxldGUucHk=) | `33.33% <0.00%> (-53.34%)` | :arrow_down: |
| [superset/datasets/columns/commands/delete.py](https://codecov.io/gh/apache/superset/pull/23113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29sdW1ucy9jb21tYW5kcy9kZWxldGUucHk=) | `44.11% <0.00%> (-52.95%)` | :arrow_down: |
| [superset/datasets/metrics/commands/delete.py](https://codecov.io/gh/apache/superset/pull/23113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvbWV0cmljcy9jb21tYW5kcy9kZWxldGUucHk=) | `44.11% <0.00%> (-52.95%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/23113?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/datasets/commands/delete.py](https://codecov.io/gh/apache/superset/pull/23113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvZGVsZXRlLnB5) | `48.57% <0.00%> (-48.58%)` | :arrow_down: |
| [superset/datasets/commands/refresh.py](https://codecov.io/gh/apache/superset/pull/23113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvcmVmcmVzaC5weQ==) | `43.75% <0.00%> (-43.75%)` | :arrow_down: |
| [superset/datasets/dao.py](https://codecov.io/gh/apache/superset/pull/23113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvZGFvLnB5) | `52.89% <0.00%> (-38.41%)` | :arrow_down: |
| [superset/datasets/commands/duplicate.py](https://codecov.io/gh/apache/superset/pull/23113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvZHVwbGljYXRlLnB5) | `53.24% <0.00%> (-37.67%)` | :arrow_down: |
| ... and [32 more](https://codecov.io/gh/apache/superset/pull/23113?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] john-bodley commented on a diff in pull request #23113: chore(sqla): Performance tradeoff with eager loading
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #23113:
URL: https://github.com/apache/superset/pull/23113#discussion_r1110383294
##########
superset/connectors/sqla/models.py:
##########
@@ -551,13 +549,11 @@ class SqlaTable(Model, BaseDatasource): # pylint: disable=too-many-public-metho
TableColumn,
back_populates="table",
cascade="all, delete-orphan",
- lazy="selectin", # Only non-eager loading that works with bidirectional joined.
Review Comment:
The default is `lazy="select"` (which is akin to `lazy=True`), i.e., the relationships are lazy loaded.
--
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