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/11/09 22:42:23 UTC
[GitHub] [superset] john-bodley opened a new pull request, #22085: chore: Change get_table_names/get_view_names return type
john-bodley opened a new pull request, #22085:
URL: https://github.com/apache/superset/pull/22085
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
After working on https://github.com/apache/superset/pull/21794 I realized that the `get_table_names` and `get_view_names` methods should really return a set of names rather than an ordered list. The results can then be sorted—if needed—within the API response.
### 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] john-bodley merged pull request #22085: chore: Change get_table_names/get_view_names return type
Posted by GitBox <gi...@apache.org>.
john-bodley merged PR #22085:
URL: https://github.com/apache/superset/pull/22085
--
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 #22085: chore: Change get_table_names/get_view_names return type
Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #22085:
URL: https://github.com/apache/superset/pull/22085#issuecomment-1312676334
I don't understand why you have to return set. Shouldn't DBAPI return unique table names to begin with?
--
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 #22085: chore: Change get_table_names/get_view_names return type
Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #22085:
URL: https://github.com/apache/superset/pull/22085#issuecomment-1317314117
Set has the additional memory cost of keeping the hashtable of elements: https://towardsdatascience.com/memory-efficiency-of-common-python-data-structures-88f0f720421
Maybe not a big deal in this case but in general I don't think we should sacrifice performance for an implied convention that is supposed to help with readability.
Regarding readability, in the case where sets are used, people would wonder "why this HAS to be a set" (like I did), which actually makes the code more confusing to them.
--
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 #22085: chore: Change get_table_names/get_view_names return type
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #22085:
URL: https://github.com/apache/superset/pull/22085#discussion_r1024210494
##########
tests/integration_tests/db_engine_specs/base_engine_spec_tests.py:
##########
@@ -229,11 +229,11 @@ def test_get_table_names(self):
""" Make sure base engine spec removes schema name from table name
ie. when try_remove_schema_from_table_name == True. """
- base_result_expected = ["table", "table_2"]
+ base_result_expected = {"table", "table_2"}
base_result = BaseEngineSpec.get_table_names(
database=mock.ANY, schema="schema", inspector=inspector
)
- self.assertListEqual(base_result_expected, base_result)
+ self.assertSetEqual(base_result_expected, base_result)
Review Comment:
I agree. I didn't know if unittest supported this.
--
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 #22085: chore: Change get_table_names/get_view_names return type
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22085:
URL: https://github.com/apache/superset/pull/22085#issuecomment-1317302736
# [Codecov](https://codecov.io/gh/apache/superset/pull/22085?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 [#22085](https://codecov.io/gh/apache/superset/pull/22085?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a876fb4) into [master](https://codecov.io/gh/apache/superset/commit/9f7bd1e63fbd4084b1dd1ad9b1dd718ff43c7e7c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9f7bd1e) will **increase** coverage by `3.16%`.
> The diff coverage is `31.25%`.
```diff
@@ Coverage Diff @@
## master #22085 +/- ##
==========================================
+ Coverage 52.50% 55.66% +3.16%
==========================================
Files 1818 1819 +1
Lines 69632 69555 -77
Branches 7496 7496
==========================================
+ Hits 36562 38720 +2158
+ Misses 31132 28897 -2235
Partials 1938 1938
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.61% <31.25%> (?)` | |
| presto | `52.50% <31.25%> (?)` | |
| python | `57.78% <31.25%> (+6.56%)` | :arrow_up: |
| unit | `50.86% <31.25%> (-0.35%)` | :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/22085?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/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/22085/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=) | `75.46% <ø> (+2.62%)` | :arrow_up: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/22085/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=) | `78.47% <0.00%> (+7.07%)` | :arrow_up: |
| [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/22085/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==) | `35.56% <ø> (+8.77%)` | :arrow_up: |
| [superset/db\_engine\_specs/databricks.py](https://codecov.io/gh/apache/superset/pull/22085/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RhdGFicmlja3MucHk=) | `82.92% <50.00%> (+5.65%)` | :arrow_up: |
| [superset/db\_engine\_specs/duckdb.py](https://codecov.io/gh/apache/superset/pull/22085/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2R1Y2tkYi5weQ==) | `75.86% <50.00%> (ø)` | |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/superset/pull/22085/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) | `59.48% <50.00%> (+5.24%)` | :arrow_up: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/22085/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==) | `40.70% <50.00%> (+14.25%)` | :arrow_up: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/22085/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `89.28% <50.00%> (ø)` | |
| [superset/tables/schemas.py](https://codecov.io/gh/apache/superset/pull/22085/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-c3VwZXJzZXQvdGFibGVzL3NjaGVtYXMucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/columns/schemas.py](https://codecov.io/gh/apache/superset/pull/22085/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-c3VwZXJzZXQvY29sdW1ucy9zY2hlbWFzLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [100 more](https://codecov.io/gh/apache/superset/pull/22085/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) | |
: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] villebro commented on a diff in pull request #22085: chore: Change get_table_names/get_view_names return type
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22085:
URL: https://github.com/apache/superset/pull/22085#discussion_r1020033030
##########
tests/integration_tests/db_engine_specs/base_engine_spec_tests.py:
##########
@@ -229,11 +229,11 @@ def test_get_table_names(self):
""" Make sure base engine spec removes schema name from table name
ie. when try_remove_schema_from_table_name == True. """
- base_result_expected = ["table", "table_2"]
+ base_result_expected = {"table", "table_2"}
base_result = BaseEngineSpec.get_table_names(
database=mock.ANY, schema="schema", inspector=inspector
)
- self.assertListEqual(base_result_expected, base_result)
+ self.assertSetEqual(base_result_expected, base_result)
Review Comment:
could we just do this as that's what appears to be preferred these days?
```
assert base_result_expected == base_result
```
##########
superset/models/core.py:
##########
@@ -556,13 +556,17 @@ def get_all_table_names_in_schema( # pylint: disable=unused-argument
:param cache: whether cache is enabled for the function
:param cache_timeout: timeout in seconds for the cache
:param force: whether to force refresh the cache
- :return: list of tables
+ :return: set of tables
Review Comment:
code style: Some people consider including the type in the return description bad practice if it's already typed in the sig. I'm not sure which camp I belong to (it varies from day to day), but maybe we could rather try to be more expressive regarding the funny `Tuple[str, str]` thingy, something like
```
:return: table name and schema pairs
```
--
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 #22085: chore: Change get_table_names/get_view_names return type
Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #22085:
URL: https://github.com/apache/superset/pull/22085#issuecomment-1317273743
@ktmud regarding your comment,
> I don't understand why you have to return set. Shouldn't DBAPI return unique table names to begin with?
Yes the DB-API will return a unique set of table names so the set logic isn't for deduping purposes (except for when we need to do set differing for determining the tables vs. views). Personally I think—from reading the code—that the type hints help one grok the logic of the code better, i.e., when I see `Set[str]` I know the values are unique whereas when I see `List[str]` I'm not as evident. We should really only use `List[...]` when order matters.
--
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