refactor, address review

Author: alessandro-nori

pyiceberg/catalog/sql.py

@@ -618,17 +618,18 @@ def list_namespaces(self, namespace: Union[str, Identifier] = ()) -> List[Identi
             namespace_stmt,
         )
         with Session(self.engine) as session:
-            namespaces = [Catalog.identifier_to_tuple(namespace_col) for namespace_col in session.execute(stmt).scalars()]
-            sub_namespaces_level_length = len(Catalog.identifier_to_tuple(namespace)) + 1 if namespace else 1
-
-            # only get sub namespaces/children
-            namespaces = list({ns[:sub_namespaces_level_length] for ns in namespaces if len(ns) >= sub_namespaces_level_length})
-
-            if namespace:
-                namespace_tuple = Catalog.identifier_to_tuple(namespace)
-                namespace_level_length = len(namespace_tuple)
-                # exclude fuzzy matches when `namespace` contains `%` or `_`
-                namespaces = [ns for ns in namespaces if ns[:namespace_level_length] == namespace_tuple]
+            namespace_tuple = Catalog.identifier_to_tuple(namespace)
+            sub_namespaces_level_length = len(namespace_tuple) + 1
+
+            namespaces = list(
+                {  # only get distinct namespaces
+                    ns[:sub_namespaces_level_length]  # truncate to the required level
+                    for ns in {Catalog.identifier_to_tuple(ns) for ns in session.execute(stmt).scalars()}
+                    if len(ns) >= sub_namespaces_level_length  # only get sub namespaces/children
+                    and ns[: sub_namespaces_level_length - 1] == namespace_tuple
+                    # exclude fuzzy matches when `namespace` contains `%` or `_`
+                }
+            )
 
             return namespaces
 

tests/catalog/test_base.py

@@ -312,7 +312,7 @@ def test_rename_table(catalog: InMemoryCatalog) -> None:
     assert table._identifier == Catalog.identifier_to_tuple(new_table)
 
     # And
-    assert ("new",) in catalog.list_namespaces()
+    assert catalog._namespace_exists(table._identifier[:-1])
 
     # And
     with pytest.raises(NoSuchTableError, match=NO_SUCH_TABLE_ERROR):
@@ -336,7 +336,7 @@ def test_rename_table_from_self_identifier(catalog: InMemoryCatalog) -> None:
     assert new_table._identifier == Catalog.identifier_to_tuple(new_table_name)
 
     # And
-    assert ("new",) in catalog.list_namespaces()
+    assert catalog._namespace_exists(new_table._identifier[:-1])
 
     # And
     with pytest.raises(NoSuchTableError, match=NO_SUCH_TABLE_ERROR):
@@ -350,7 +350,7 @@ def test_create_namespace(catalog: InMemoryCatalog) -> None:
     catalog.create_namespace(TEST_TABLE_NAMESPACE, TEST_TABLE_PROPERTIES)
 
     # Then
-    assert TEST_TABLE_NAMESPACE[:1] in catalog.list_namespaces()
+    assert catalog._namespace_exists(TEST_TABLE_NAMESPACE)
     assert TEST_TABLE_PROPERTIES == catalog.load_namespace_properties(TEST_TABLE_NAMESPACE)
 
 
@@ -375,14 +375,19 @@ def test_list_namespaces(catalog: InMemoryCatalog) -> None:
     # Then
     assert TEST_TABLE_NAMESPACE[:1] in namespaces
 
+    # When
+    namespaces = catalog.list_namespaces(TEST_TABLE_NAMESPACE)
+    # Then
+    assert not namespaces
+
 
 def test_drop_namespace(catalog: InMemoryCatalog) -> None:
     # Given
     catalog.create_namespace(TEST_TABLE_NAMESPACE, TEST_TABLE_PROPERTIES)
     # When
     catalog.drop_namespace(TEST_TABLE_NAMESPACE)
     # Then
-    assert TEST_TABLE_NAMESPACE[:1] not in catalog.list_namespaces()
+    assert not catalog._namespace_exists(TEST_TABLE_NAMESPACE)
 
 
 def test_drop_namespace_raises_error_when_namespace_does_not_exist(catalog: InMemoryCatalog) -> None:
@@ -431,7 +436,7 @@ def test_update_namespace_metadata(catalog: InMemoryCatalog) -> None:
     summary = catalog.update_namespace_properties(TEST_TABLE_NAMESPACE, updates=new_metadata)
 
     # Then
-    assert TEST_TABLE_NAMESPACE[:1] in catalog.list_namespaces()
+    assert catalog._namespace_exists(TEST_TABLE_NAMESPACE)
     assert new_metadata.items() <= catalog.load_namespace_properties(TEST_TABLE_NAMESPACE).items()
     assert summary.removed == []
     assert sorted(summary.updated) == ["key3", "key4"]
@@ -448,7 +453,7 @@ def test_update_namespace_metadata_removals(catalog: InMemoryCatalog) -> None:
     summary = catalog.update_namespace_properties(TEST_TABLE_NAMESPACE, remove_metadata, new_metadata)
 
     # Then
-    assert TEST_TABLE_NAMESPACE[:1] in catalog.list_namespaces()
+    assert catalog._namespace_exists(TEST_TABLE_NAMESPACE)
     assert new_metadata.items() <= catalog.load_namespace_properties(TEST_TABLE_NAMESPACE).items()
     assert remove_metadata.isdisjoint(catalog.load_namespace_properties(TEST_TABLE_NAMESPACE).keys())
     assert summary.removed == ["key1"]

tests/catalog/test_sql.py

@@ -1118,26 +1118,19 @@ def test_create_namespace_with_empty_identifier(catalog: SqlCatalog, empty_names
     ],
 )
 def test_list_namespaces(catalog: SqlCatalog) -> None:
-    namespace_list = ["db", "db.ns1", "db.ns1.ns2", "db.ns2", "db2", "db2.ns1", "db%"]
+    namespace_list = ["db", "db.ns1", "db.ns1.ns2", "db.ns2", "db2", "db2.ns1", "db%", "db.ns1X.ns3"]
     for namespace in namespace_list:
         catalog.create_namespace(namespace)
 
     ns_list = catalog.list_namespaces()
-    expected_list: list[tuple[str, ...]] = [("db",), ("db2",), ("db%",)]
-    for ns in expected_list:
+    for ns in [("db",), ("db%",), ("db2",)]:
         assert ns in ns_list
 
     ns_list = catalog.list_namespaces("db")
-    expected_list = [("db", "ns1"), ("db", "ns2")]
-    assert len(ns_list) == len(expected_list)
-    for ns in expected_list:
-        assert ns in ns_list
+    assert sorted(ns_list) == [("db", "ns1"), ("db", "ns1X"), ("db", "ns2")]
 
     ns_list = catalog.list_namespaces("db.ns1")
-    expected_list = [("db", "ns1", "ns2")]
-    assert len(ns_list) == len(expected_list)
-    for ns in expected_list:
-        assert ns in ns_list
+    assert sorted(ns_list) == [("db", "ns1", "ns2")]
 
     ns_list = catalog.list_namespaces("db.ns1.ns2")
     assert len(ns_list) == 0