Scoping "Seen Tables". Improving table scan performance.#3741
Open
Scoping "Seen Tables". Improving table scan performance.#3741
Conversation
FastLee
had a problem deploying
to
account-admin
GitHub Actions
Error
FastLee
force-pushed
the
experimental/scoped_seen_tables
branch
from
6836326 to
6257fd3
Compare
FastLee
temporarily deployed
to
account-admin
GitHub Actions
Inactive
|
❌ 54/55 passed, 1 failed, 5 skipped, 4h37m11s total ❌ test_all_grant_types: AssertionError: assert {('ANONYMOUS ...dummy_twu2b')} == {('ANONYMOUS ..._fzcf6'), ...} (12m12.2s)Running from acceptance #8385 |
FastLee
force-pushed
the
experimental/scoped_seen_tables
branch
from
6257fd3 to
c51ee78
Compare
FastLee
had a problem deploying
to
account-admin
GitHub Actions
Failure
JCZuurmond
suggested changes
Feb 25, 2025
Contributor
JCZuurmond
left a comment
JCZuurmond
left a comment
There was a problem hiding this comment.
@FastLee : I have to think about the choice to add a scope for a bit.
It feels redundant to pass the in-scope tables, extract the schemas names from those tables, then skip the in-scope schemas when iterating over all schemas. Why not directly loop over the in-scope schemas?
A similar approach is repeated for the table names, which makes the schema loop even more redundant. Why not directly go to the in-scope tables?
Moreover, we typically provide the scope using an include_ at initialization, which might not work here, but I have to think about that
| def _iter_schemas(self) -> Iterable[SchemaInfo]: | ||
| for catalog in self._iter_catalogs(): | ||
| if catalog.name is None: | ||
| if catalog.name is None or catalog.catalog_type in ( |
Contributor
There was a problem hiding this comment.
This has probably the same effect as the filter on line 186. If not, move the filter there so that filtering catalogs happens inside the _iter_catalog
| if ( | ||
| schema.catalog_name is None | ||
| or schema.name is None | ||
| or schema.name == "information_schema" |
Contributor
There was a problem hiding this comment.
Move this into _iter_schema
| tables: list = [] | ||
| logger.info(f"Scanning {len(tasks)} schemas for tables") | ||
| table_lists = Threads.gather("list tables", tasks, self.API_LIMIT) | ||
| # Combine tuple of lists to a list |
Contributor
There was a problem hiding this comment.
Use itertools.chain instead
| for table_list in table_lists[0]: | ||
| tables.extend(table_list) | ||
| for table in tables: | ||
| if not isinstance(table, TableInfo): |
Contributor
There was a problem hiding this comment.
Is this supposed to happen? It should not based on the type hinting, right?
| ) | ||
| tables: list = [] | ||
| logger.info(f"Scanning {len(tasks)} schemas for tables") | ||
| table_lists = Threads.gather("list tables", tasks, self.API_LIMIT) |
Contributor
There was a problem hiding this comment.
It is not an API limit, but the number of threads
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scoped down table scan for migrated tables.