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/08/30 21:25:38 UTC
[GitHub] [superset] justinpark opened a new pull request, #21262: fix(sqllab): table list exceeds MAX_TABLE_NAMES
justinpark opened a new pull request, #21262:
URL: https://github.com/apache/superset/pull/21262
### SUMMARY
Since `max_tables` logic multiplies max_items by the current size, it always returns entire list.
(i.e. `config["MAX_TABLE_NAMES"] = 1000` and `len(tables) = 40000` then `max_tables = 40000 * 1000 = 40,000,000)
The previous logic also cuts out only if substr passed.
This commit always follows the `MAX_TABLE_NAMES` for both cases. (must set MAX_TABLE_NAMES to 0 if full table list needed)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
### TESTING INSTRUCTIONS
Set MAX_TABLE_NAMES to a small number
Go to Sqllab and check the count of table list
### ADDITIONAL INFORMATION
- [ ] 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
cc: @john-bodley @ktmud
--
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] justinpark commented on a diff in pull request #21262: fix(sqllab): table list exceeds MAX_TABLE_NAMES
Posted by GitBox <gi...@apache.org>.
justinpark commented on code in PR #21262:
URL: https://github.com/apache/superset/pull/21262#discussion_r958995057
##########
superset/views/core.py:
##########
@@ -1202,11 +1202,8 @@ def is_match(src: str, target: utils.DatasourceName) -> bool:
max_items = config["MAX_TABLE_NAMES"] or len(tables)
total_items = len(tables) + len(views)
- max_tables = len(tables)
- max_views = len(views)
- if total_items and substr_parsed:
- max_tables = max_items * len(tables) // total_items
- max_views = max_items * len(views) // total_items
+ max_tables = max_items or len(tables)
Review Comment:
@john-bodley you're right. it used to allow the empty `schema_parsed` option before (#1466) but no longer allowed as far as the current FE logic exists.
And `MAX_TABLE_NAMES` never invoked since `substr_parsed` is never truthy.
I hope to restore `MAX_TABLE_NAMES` option to fix our edge cases (>100k).
I posted updates to make `max_items` = max_tables + max_views
--
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] justinpark commented on a diff in pull request #21262: fix(sqllab): table list exceeds MAX_TABLE_NAMES
Posted by GitBox <gi...@apache.org>.
justinpark commented on code in PR #21262:
URL: https://github.com/apache/superset/pull/21262#discussion_r958998296
##########
superset/views/core.py:
##########
@@ -1202,11 +1202,8 @@ def is_match(src: str, target: utils.DatasourceName) -> bool:
max_items = config["MAX_TABLE_NAMES"] or len(tables)
total_items = len(tables) + len(views)
- max_tables = len(tables)
- max_views = len(views)
- if total_items and substr_parsed:
- max_tables = max_items * len(tables) // total_items
- max_views = max_items * len(views) // total_items
+ max_tables = max_items or len(tables)
Review Comment:
current FE doesn't pass `substr_parsed` but I'll make FE changes to use `substr_parsed` for typeahead on truncated list
--
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] justinpark closed pull request #21262: fix(sqllab): table list exceeds MAX_TABLE_NAMES
Posted by GitBox <gi...@apache.org>.
justinpark closed pull request #21262: fix(sqllab): table list exceeds MAX_TABLE_NAMES
URL: https://github.com/apache/superset/pull/21262
--
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 #21262: fix(sqllab): table list exceeds MAX_TABLE_NAMES
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21262:
URL: https://github.com/apache/superset/pull/21262#issuecomment-1232193330
# [Codecov](https://codecov.io/gh/apache/superset/pull/21262?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 [#21262](https://codecov.io/gh/apache/superset/pull/21262?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3ac750a) into [master](https://codecov.io/gh/apache/superset/commit/05354a96bfaeacaa39974977b4502cd26bef8413?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (05354a9) will **decrease** coverage by `11.59%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #21262 +/- ##
===========================================
- Coverage 66.43% 54.83% -11.60%
===========================================
Files 1784 1784
Lines 68185 68182 -3
Branches 7265 7265
===========================================
- Hits 45298 37387 -7911
- Misses 21018 28926 +7908
Partials 1869 1869
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.92% <0.00%> (+<0.01%)` | :arrow_up: |
| python | `57.33% <0.00%> (-24.01%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.64% <0.00%> (+<0.01%)` | :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/21262?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/views/core.py](https://codecov.io/gh/apache/superset/pull/21262/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==) | `34.56% <0.00%> (-41.04%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/21262/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-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/21262/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-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21262/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-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/21262/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%> (-85.19%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/21262/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-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21262/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/21262/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-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
| [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/21262/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-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `29.41% <0.00%> (-68.63%)` | :arrow_down: |
| [superset/datasets/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21262/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-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvaW1wb3J0ZXJzL3YwLnB5) | `24.03% <0.00%> (-67.45%)` | :arrow_down: |
| ... and [282 more](https://codecov.io/gh/apache/superset/pull/21262/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] john-bodley commented on a diff in pull request #21262: fix(sqllab): table list exceeds MAX_TABLE_NAMES
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #21262:
URL: https://github.com/apache/superset/pull/21262#discussion_r958966251
##########
superset/views/core.py:
##########
@@ -1202,11 +1202,8 @@ def is_match(src: str, target: utils.DatasourceName) -> bool:
max_items = config["MAX_TABLE_NAMES"] or len(tables)
total_items = len(tables) + len(views)
- max_tables = len(tables)
- max_views = len(views)
- if total_items and substr_parsed:
- max_tables = max_items * len(tables) // total_items
- max_views = max_items * len(views) // total_items
+ max_tables = max_items or len(tables)
Review Comment:
@justinpark this logic isn't quite right, the `//` ensures that `max_tables` + `max_views` = `max_items`.
Furthermore this endpoint is only invoked [here](https://github.com/apache/superset/blob/master/superset-frontend/src/components/TableSelector/index.tsx#L211-L250) which implies that `substr_parsed` is never truthy and the `MAX_TABLE_NAMES` config doesn't actually get invoked.
I think we should actually refactor this method to remove the `substr_parsed` logic and deprecate the `MAX_TABLE_NAMES`, especially given that it's non deterministic in terms of how it truncates the list.
Furthermore it seems like `schema_parsed` is always truthy (assuming I'm reading the frontend code correctly), i.e., per line #212 `currentSchema` is truthy and never "undefined" which simplifies the logic and thus large swaths of this method can be refactored.
--
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