kevinjqliu commented on code in PR #1629:
URL: https://github.com/apache/iceberg-python/pull/1629#discussion_r1953886162
##########
tests/catalog/test_sql.py:
##########
@@ -1116,17 +1116,30 @@ def
test_create_namespace_with_empty_identifier(catalog: SqlCatalog, empty_names
lazy_fixture("catalog_sqlite"),
],
)
-@pytest.mark.parametrize("namespace_list", [lazy_fixture("database_list"),
lazy_fixture("hierarchical_namespace_list")])
-def test_list_namespaces(catalog: SqlCatalog, namespace_list: List[str]) ->
None:
+def test_list_namespaces(catalog: SqlCatalog) -> None:
Review Comment:
thanks for adding this test!
##########
pyiceberg/catalog/sql.py:
##########
@@ -610,15 +610,26 @@ def list_namespaces(self, namespace: Union[str,
Identifier] = ()) -> List[Identi
table_stmt =
select(IcebergTables.table_namespace).where(IcebergTables.catalog_name ==
self.name)
namespace_stmt =
select(IcebergNamespaceProperties.namespace).where(IcebergNamespaceProperties.catalog_name
== self.name)
if namespace:
- namespace_str = Catalog.namespace_to_string(namespace,
NoSuchNamespaceError)
- table_stmt =
table_stmt.where(IcebergTables.table_namespace.like(namespace_str))
- namespace_stmt =
namespace_stmt.where(IcebergNamespaceProperties.namespace.like(namespace_str))
+ namespace_like = Catalog.namespace_to_string(namespace,
NoSuchNamespaceError) + "%"
+ table_stmt =
table_stmt.where(IcebergTables.table_namespace.like(namespace_like))
+ namespace_stmt =
namespace_stmt.where(IcebergNamespaceProperties.namespace.like(namespace_like))
stmt = union(
table_stmt,
namespace_stmt,
)
with Session(self.engine) as session:
- return [Catalog.identifier_to_tuple(namespace_col) for
namespace_col in session.execute(stmt).scalars()]
+ namespaces = [Catalog.identifier_to_tuple(namespace_col) for
namespace_col in session.execute(stmt).scalars()]
Review Comment:
instead of all this logic, can we just filter out the results?
##########
pyiceberg/catalog/sql.py:
##########
@@ -610,15 +610,26 @@ def list_namespaces(self, namespace: Union[str,
Identifier] = ()) -> List[Identi
table_stmt =
select(IcebergTables.table_namespace).where(IcebergTables.catalog_name ==
self.name)
namespace_stmt =
select(IcebergNamespaceProperties.namespace).where(IcebergNamespaceProperties.catalog_name
== self.name)
if namespace:
- namespace_str = Catalog.namespace_to_string(namespace,
NoSuchNamespaceError)
- table_stmt =
table_stmt.where(IcebergTables.table_namespace.like(namespace_str))
- namespace_stmt =
namespace_stmt.where(IcebergNamespaceProperties.namespace.like(namespace_str))
+ namespace_like = Catalog.namespace_to_string(namespace,
NoSuchNamespaceError) + "%"
Review Comment:
> I found out that not adding % only returns the namespace itself.
i see that when namespaces are created, we use the [flattened version of the
namespace](https://github.com/apache/iceberg-python/blob/1532ab4c37a5aeed61d76ef0ac1ab8f1101fe6e0/pyiceberg/catalog/sql.py#L541)
for example, namespace=(foo,bar) -> "foo.bar"
so using `.like` should cover this case
##########
tests/catalog/test_sql.py:
##########
@@ -1158,13 +1171,13 @@ def test_list_non_existing_namespaces(catalog:
SqlCatalog) -> None:
def test_drop_namespace(catalog: SqlCatalog, table_schema_nested: Schema,
table_identifier: Identifier) -> None:
namespace = Catalog.namespace_from(table_identifier)
catalog.create_namespace(namespace)
- assert namespace in catalog.list_namespaces()
+ assert namespace[:1] in catalog.list_namespaces()
Review Comment:
why is this excluding the first result?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
