feat: add CatalogProviderList support - #1363
Conversation
Sorry, something went wrong.
nuno-faria
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
| ... | ||
|
|
||
| @abstractmethod | ||
| def catalog(self, name: str) -> Catalog | None: |
There was a problem hiding this comment.
Shouldn't the return type here be CatalogProvider?
Sorry, something went wrong.
There was a problem hiding this comment.
Good point! This should be any of the 3 returnable types, including Catalog.
Sorry, something went wrong.
| ... | ||
|
|
||
| def register_catalog( # noqa: B027 | ||
| self, name: str, catalog: CatalogProviderExportable | CatalogProvider | Catalog |
There was a problem hiding this comment.
Also is the Catalog type here correct?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, especially if they want to register an in-memory catalog they created with the static method.
Sorry, something went wrong.
|
|
||
| class CustomCatalogProviderList(dfn.catalog.CatalogProviderList): | ||
| def __init__(self): | ||
| self.catalogs = {"my_catalog": CustomCatalogProvider()} |
There was a problem hiding this comment.
Maybe this could have an additional catalog just to show it supports multiple ones.
Sorry, something went wrong.
There was a problem hiding this comment.
I added a different catalog type in the register test to follow.
Sorry, something went wrong.
|
Thanks for the review @nuno-faria ! |
Sorry, something went wrong.
Which issue does this PR close?
Follow on to #1156
Rationale for this change
We currently support the following kinds of providers: Catalog, Schema, and Table. There is one more in the heirarchy, CatalogProviderList. This adds the missing support.
What changes are included in this PR?
Following the same patterns as in CatalogProvider (and others) this adds support for Python and Rust based CatalogProviderList. It adds the option to register a new provider list with the session context.
Are there any user-facing changes?
New addition only.