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 2022/07/18 22:27:34 UTC
[GitHub] [superset] john-bodley opened a new pull request, #20757: fix: PrestoEngineSpec._show_columns return type
john-bodley opened a new pull request, #20757:
URL: https://github.com/apache/superset/pull/20757
<!---
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 `PrestoEngineSpec._show_columns` method stated it returned a `List[RowProxy]` type however it actually returned a `ResultProxy`, i.e., per the SQLAlchemy [documentation](https://docs.sqlalchemy.org/en/13/core/connections.html),
> The [RowProxy](https://docs.sqlalchemy.org/en/13/core/connections.html?highlight=rowproxy#sqlalchemy.engine.RowProxy) object is retrieved from a database result, from the [ResultProxy](https://docs.sqlalchemy.org/en/13/core/connections.html?highlight=rowproxy#sqlalchemy.engine.ResultProxy) object using methods like [ResultProxy.fetchall()](https://docs.sqlalchemy.org/en/13/core/connections.html?highlight=rowproxy#sqlalchemy.engine.ResultProxy.fetchall).
From a coding perspective this wasn't problematic as the output is later processed via,
```python
for column in PrestoEngineSpec._show_columns(...):
....
```
and though the `ResultProxy` supports iteration—per the `__iter__` method in the DB API, it does not support pickling (and thus caching) whereas the `RowProxy` does (it's acts somewhat akin to a Python `tuple`). This was resulting in an issue at Airbnb where we actually override and cache the `PrestoEngineSpec._show_columns` output.
The fix is merely to augment the method to return a `List[RowProxy]` as promised rather than a `ResultProxy`.
### 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
[GitHub] [superset] codecov[bot] commented on pull request #20757: fix: PrestoEngineSpec._show_columns return type
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20757:
URL: https://github.com/apache/superset/pull/20757#issuecomment-1188392021
# [Codecov](https://codecov.io/gh/apache/superset/pull/20757?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 [#20757](https://codecov.io/gh/apache/superset/pull/20757?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (48408ec) into [master](https://codecov.io/gh/apache/superset/commit/11019221787eee60c94bd3b5749b6550aebede63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1101922) will **decrease** coverage by `11.50%`.
> The diff coverage is `59.45%`.
> :exclamation: Current head 48408ec differs from pull request most recent head 106546b. Consider uploading reports for the commit 106546b to get more accurate results
```diff
@@ Coverage Diff @@
## master #20757 +/- ##
===========================================
- Coverage 66.20% 54.69% -11.51%
===========================================
Files 1754 1754
Lines 66678 66688 +10
Branches 7049 7049
===========================================
- Hits 44143 36476 -7667
- Misses 20738 28415 +7677
Partials 1797 1797
```
| Flag | Coverage Δ | |
|---|---|---|
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.09% <48.64%> (?)` | |
| python | `57.64% <59.45%> (-23.74%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.57% <51.35%> (+0.13%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20757?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/commands/importers/v1/assets.py](https://codecov.io/gh/apache/superset/pull/20757/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-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL2Fzc2V0cy5weQ==) | `32.14% <ø> (-53.58%)` | :arrow_down: |
| [superset/commands/importers/v1/examples.py](https://codecov.io/gh/apache/superset/pull/20757/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-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL2V4YW1wbGVzLnB5) | `0.00% <ø> (ø)` | |
| [...erset/dashboards/commands/importers/v1/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/20757/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjEvX19pbml0X18ucHk=) | `35.29% <ø> (-63.24%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/superset/pull/20757/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `52.38% <0.00%> (-45.21%)` | :arrow_down: |
| [superset/db\_engine\_specs/snowflake.py](https://codecov.io/gh/apache/superset/pull/20757/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Nub3dmbGFrZS5weQ==) | `75.65% <0.00%> (-0.67%)` | :arrow_down: |
| [superset/models/sql\_types/presto\_sql\_types.py](https://codecov.io/gh/apache/superset/pull/20757/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-c3VwZXJzZXQvbW9kZWxzL3NxbF90eXBlcy9wcmVzdG9fc3FsX3R5cGVzLnB5) | `76.47% <ø> (-3.93%)` | :arrow_down: |
| [superset/utils/mock\_data.py](https://codecov.io/gh/apache/superset/pull/20757/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-c3VwZXJzZXQvdXRpbHMvbW9ja19kYXRhLnB5) | `0.00% <ø> (ø)` | |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/20757/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=) | `34.73% <25.00%> (-35.27%)` | :arrow_down: |
| [superset/db\_engine\_specs/gsheets.py](https://codecov.io/gh/apache/superset/pull/20757/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2dzaGVldHMucHk=) | `69.00% <33.33%> (-6.76%)` | :arrow_down: |
| [superset/db\_engine\_specs/trino.py](https://codecov.io/gh/apache/superset/pull/20757/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3RyaW5vLnB5) | `45.33% <50.00%> (-43.86%)` | :arrow_down: |
| ... and [290 more](https://codecov.io/gh/apache/superset/pull/20757/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/20757?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/20757?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 [1101922...106546b](https://codecov.io/gh/apache/superset/pull/20757?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 merged pull request #20757: fix: PrestoEngineSpec._show_columns return type
Posted by GitBox <gi...@apache.org>.
john-bodley merged PR #20757:
URL: https://github.com/apache/superset/pull/20757
--
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