-
Notifications
You must be signed in to change notification settings - Fork 426
Add missing catalog tests #2955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| def _namespace_exists(catalog: Catalog, namespace: str | Identifier) -> bool: | ||
| try: | ||
| catalog.load_namespace_properties(namespace) | ||
| return True | ||
| except NoSuchNamespaceError: | ||
| return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we just add namespace_exists to all the catalog implementations?
similar to the one in the rest catalog,
iceberg-python/pyiceberg/catalog/rest/__init__.py
Lines 1106 to 1131 in 2f293d6
| @retry(**_RETRY_ARGS) | |
| def namespace_exists(self, namespace: str | Identifier) -> bool: | |
| namespace_tuple = self._check_valid_namespace_identifier(namespace) | |
| namespace = self._encode_namespace_path(namespace_tuple) | |
| # fallback in order to work with older rest catalog implementations | |
| if Capability.V1_NAMESPACE_EXISTS not in self._supported_endpoints: | |
| try: | |
| self.load_namespace_properties(namespace_tuple) | |
| return True | |
| except NoSuchNamespaceError: | |
| return False | |
| response = self._session.head(self.url(Endpoints.namespace_exists, namespace=namespace)) | |
| if response.status_code == 404: | |
| return False | |
| elif response.status_code in (200, 204): | |
| return True | |
| try: | |
| response.raise_for_status() | |
| except HTTPError as exc: | |
| _handle_non_200_response(exc, {}) | |
| return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree 100% with this. I created #2969. I can make a separate PR, but I feel like it'll potentially muddy this one to add it here. How does that sound?
tests/integration/test_catalog.py
Outdated
| # list_namespaces returns a list of tuples | ||
| if isinstance(test_catalog, RestCatalog): | ||
| namespaces = test_catalog.list_namespaces() | ||
| assert ("new",) in namespaces or ("new.db",) in namespaces | ||
| else: | ||
| assert namespace in test_catalog.list_namespaces() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know why theres a behavior difference here?
ideally we're testing for consistent catalog behaviors in this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think of this as an actual issue, but maybe it is.
Calling list namespaces would get you:
rest_catalog - new
others - new.db
That's because iceberg-rest-fixture treats this as hierarchical namespaces, which many other catalogs don't support. The spec supports hierarchical namespaces, so that seems to be working as intended.
My comment was very poor, so I wrote an actual comment to explain this.
|
Nice! |
Rationale for this change
There's a couple missing catalog tests around supporting namespaces + tables with slashes/dots. Along the way, I found an issue in how we create the CreateTableRequest + RegisterTableRequest
Are these changes tested?
Tests are included.
Are there any user-facing changes?