sungwy commented on code in PR #1155:
URL: https://github.com/apache/iceberg-python/pull/1155#discussion_r1761710974
##########
tests/catalog/test_sql.py:
##########
@@ -225,6 +237,93 @@ def test_creation_from_impl(catalog_name: str, warehouse:
Path) -> None:
)
+def confirm_no_tables_exist(alchemy_engine: Engine) -> list[str]:
+ inspector = inspect(alchemy_engine)
+ for c in SqlCatalogBaseTable.__subclasses__():
+ if inspector.has_table(c.__tablename__):
+ c.__table__.drop(alchemy_engine)
+
+ catalog_tables = [c.__tablename__ for c in
SqlCatalogBaseTable.__subclasses__()]
Review Comment:
This function seems to serve two purposes:
1. confirm that no tables exist
2. return the list of `SqlCatalogBaseTable.__subclasses__()`
(2) is a static list of tables required for the SqlCatalog. Would it be
cleaner to separate out (2) to a global variable in this module, instead of
returning it and passing it as an input to `confirm_all_tables_exist` ?
##########
tests/catalog/test_sql.py:
##########
@@ -225,6 +237,93 @@ def test_creation_from_impl(catalog_name: str, warehouse:
Path) -> None:
)
+def confirm_no_tables_exist(alchemy_engine: Engine) -> list[str]:
Review Comment:
```suggestion
def confirm_no_tables_exist(alchemy_engine: Engine) -> List[str]:
```
for python3.8 compatibility (which will be deprecated soon), but this is
currently needed for our CI to pass
##########
tests/catalog/test_sql.py:
##########
@@ -225,6 +237,93 @@ def test_creation_from_impl(catalog_name: str, warehouse:
Path) -> None:
)
+def confirm_no_tables_exist(alchemy_engine: Engine) -> list[str]:
+ inspector = inspect(alchemy_engine)
+ for c in SqlCatalogBaseTable.__subclasses__():
+ if inspector.has_table(c.__tablename__):
+ c.__table__.drop(alchemy_engine)
+
+ catalog_tables = [c.__tablename__ for c in
SqlCatalogBaseTable.__subclasses__()]
+ any_table_exists = any(t for t in inspector.get_table_names() if t in
catalog_tables)
+ if any_table_exists:
+ pytest.raises(TableAlreadyExistsError, "Tables exist, but should not
have been created yet")
+
+ return catalog_tables
+
+
+def confirm_all_tables_exist(inspector: Inspector, catalog_tables: list[str],
catalog: Catalog) -> None:
Review Comment:
It looks like we are passing the `catalog` here just to assert its type - is
this necessary for this check? Was the intention to check the tables through
the catalog's engine instead like:
```
inspect(catalog.engine).get_table_names()
```
--
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
