Implement list_views to the Hive catalog - #3717
Conversation
| raise NotImplementedError | ||
| """List Iceberg views under the given namespace in the catalog. | ||
|
|
||
| When the database doesn't exist, it will just return an empty list. |
There was a problem hiding this comment.
I think we should throw an exception if the database doesn't exist.
There was a problem hiding this comment.
It makes sense; I will fix it.
| (database_name, table.tableName) | ||
| for table in open_client.get_table_objects_by_name( | ||
| dbname=database_name, tbl_names=open_client.get_all_tables(db_name=database_name) | ||
| ) | ||
| if table.parameters.get(TABLE_TYPE, "").lower() == ICEBERG_VIEW |
There was a problem hiding this comment.
Getting all tables and filtering them at the caller's side is slow, and HMS may become unstable. We should consider providing a VIRTUAL_VIEW condition.
However, follow-up is still acceptable. Pre-existing for list_tables.
| When the database doesn't exist, it will just return an empty list. | ||
|
|
||
| Args: | ||
| namespace: Database to list. | ||
|
|
||
| Returns: | ||
| List[Identifier]: list of views identifiers. | ||
|
|
||
| Raises: | ||
| NoSuchNamespaceError: If a namespace with the given name does not exist, or the identifier is invalid. |
There was a problem hiding this comment.
When the database doesn't exist, it will just return an empty list.
...
NoSuchNamespaceError: If a namespace with the given name does not exist
These sentences appear contradictory.
| assert "Database default already exists" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_list_views(hive_table: HiveTable) -> None: |
There was a problem hiding this comment.
Can we add an integration test? Create a view by Spark, and list it by PyIceberg.
There was a problem hiding this comment.
I added an integration test for HiveCatalog.list_views
| dbname="database", tbl_names=["table1", "table2", "table3", "table4"] | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
I recommend adding a negative test - specifying a non-existing database.
There was a problem hiding this comment.
I added test_list_views_to_namespace_does_not_exists
|
Please move the integration test to the common suite https://github.com/apache/iceberg-python/blob/main/tests/integration/test_catalog.py#L624 |
|
Sorry I should have been a little more explicit, the common catalog integration test suite is designed to cover as much testing as possible while re-using the same tests, just different catalog features. I think it would be wise to properly add a list_views test for the hive catalog in there, instead of just copying the new test in. |
|
@jayceslesar, thanks for your suggestion. |
3 participants
Rationale for this change
I want to add list_views to the Hive catalog
Are these changes tested?
Added test_list_views