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/07/21 22:02:09 UTC
[GitHub] [superset] john-bodley opened a new pull request, #24773: chore(connector): Cleanup base models and views
john-bodley opened a new pull request, #24773:
URL: https://github.com/apache/superset/pull/24773
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
The `superset.connectors` submodule represents a bygone error of Superset when we supported both the Druid NoSQL and SQLAlchemy database engine connectors.
Many of the base models and views are superfluous as they're only extended by one and only class. This PR:
1. Integrates the `BaseColumn` and `BaseMetric` classes into the `TableColumn` and `SqlMetric` classes respectively.
2. Integrates the `DatasourceModelView` class into the `TableModelView` class.
3. Moves `superset.connectors.base.models.BaseDatasource` to `superset.connectors.base.models.BaseDatasource`.
Ideally I would love to deprecate the `BaseDatasource` however at this stage I'm not brave enough. Currently (in addition to `SqlaTable` class) the ` the `AnnotationDatasource` class is derived from it and thus sunsetting it is somewhat problematic. We maybe need to remove the `AnnotationDatasource` class somewhat like the `Query` class which behaves somewhat like a datasource/dataset.
Without doubt the `superset.connectors` submodule should be abandoned with the models, views, and utilities moved elsewhere, but I thought it would be prudent to take smaller bites.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
CI.
### 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
Re: [PR] chore(connector): Cleanup base models and views according to SIP-92 [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #24773:
URL: https://github.com/apache/superset/pull/24773
--
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
Re: [PR] chore(connector): Cleanup base models and views according to SIP-92 [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #24773:
URL: https://github.com/apache/superset/pull/24773#issuecomment-1816798515
@eschutho and @michael-s-molina I was wondering whether this PR would be a good candidate to have merged prior to the 3.1 cut given it refactors a non-trivial number of files.
--
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
Re: [PR] chore(connector): Cleanup base models and views according to SIP-92 [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #24773:
URL: https://github.com/apache/superset/pull/24773#issuecomment-1821828401
@eschutho it's likely somewhat of a tough balance right? If you do this right after a major/minor release then it gives time for the `master` branch to harden, but it likely makes cherry-picking harder—as you pointed out. The value of merging these changes just prior to a branch cut, is the `3.0` branch will soon be a LTS branch and thus there's likely only going to be a few cherry-picks/reverts onto said branch, whereas most of the upcoming cherry-picks/reverts will be occurring on `3.1` which _should_ be relatively pain free.
--
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
Re: [PR] chore(connector): Cleanup base models and views according to SIP-92 [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #24773:
URL: https://github.com/apache/superset/pull/24773#issuecomment-1821815906
@john-bodley This change looks really great!
> @eschutho and @michael-s-molina I was wondering whether this PR would be a good candidate to have merged prior to the 3.1 cut given it refactors a non-trivial number of files.
This is already merged, so I'm late to respond, but in theory, I would say it would be good to merge refactors right after a minor/major release has gone out, so that the next release starts hardening on top of the refactor. But you're still going to be impacted for any patch releases. There's no way to make this perfect, of course.
--
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 #24773: chore(connector): Cleanup base models and views
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24773:
URL: https://github.com/apache/superset/pull/24773#issuecomment-1646299325
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24773](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (53f8988) into [master](https://app.codecov.io/gh/apache/superset/commit/abb8e28e4914ad46ef50e33934ec97c1e8fcf5b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (abb8e28) will **decrease** coverage by `9.01%`.
> The diff coverage is `61.89%`.
> :exclamation: Current head 53f8988 differs from pull request most recent head 42e2c6c. Consider uploading reports for the commit 42e2c6c to get more accurate results
```diff
@@ Coverage Diff @@
## master #24773 +/- ##
==========================================
- Coverage 67.23% 58.22% -9.01%
==========================================
Files 1902 1900 -2
Lines 73939 73835 -104
Branches 8176 8176
==========================================
- Hits 49713 42992 -6721
- Misses 22113 28730 +6617
Partials 2113 2113
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.98% <61.58%> (-0.06%)` | :arrow_down: |
| python | `60.93% <61.89%> (-18.84%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `54.92% <52.74%> (?)` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset/charts/post\_processing.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL3Bvc3RfcHJvY2Vzc2luZy5weQ==) | `93.23% <0.00%> (+81.20%)` | :arrow_up: |
| [superset/commands/utils.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvdXRpbHMucHk=) | `37.83% <0.00%> (-59.46%)` | :arrow_down: |
| [superset/common/query\_context.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHQucHk=) | `85.00% <0.00%> (-10.00%)` | :arrow_down: |
| [superset/common/query\_context\_factory.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHRfZmFjdG9yeS5weQ==) | `73.91% <0.00%> (-2.90%)` | :arrow_down: |
| [superset/common/query\_object.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | `95.47% <0.00%> (+2.01%)` | :arrow_up: |
| [superset/common/query\_object\_factory.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdF9mYWN0b3J5LnB5) | `91.89% <0.00%> (ø)` | |
| [superset/daos/chart.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGFvcy9jaGFydC5weQ==) | `67.27% <0.00%> (-25.46%)` | :arrow_down: |
| [superset/examples/world\_bank.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `0.00% <0.00%> (-30.67%)` | :arrow_down: |
| [superset/models/slice.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbW9kZWxzL3NsaWNlLnB5) | `58.91% <0.00%> (-28.72%)` | :arrow_down: |
| [superset/security/manager.py](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | `61.47% <0.00%> (-32.48%)` | :arrow_down: |
| ... and [11 more](https://app.codecov.io/gh/apache/superset/pull/24773?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [328 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24773/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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