HonahX commented on code in PR #265:
URL: https://github.com/apache/iceberg-python/pull/265#discussion_r1451397678
##########
tests/catalog/test_sql.py:
##########
@@ -87,6 +88,20 @@ def catalog_sqlite(warehouse: Path) -> Generator[SqlCatalog,
None, None]:
catalog.destroy_tables()
+@pytest.fixture(scope="module")
+def catalog_sqlite_without_rowcount(warehouse: Path) -> Generator[SqlCatalog,
None, None]:
+ props = {
+ "uri": "sqlite:////tmp/sql-catalog.db",
+ "warehouse": f"file://{warehouse}",
+ }
+ catalog = SqlCatalog("test_sql_catalog", **props)
+ catalog.engine.dialect.supports_sane_rowcount = False
+ print(f"DEBUG: {catalog.engine.dialect.supports_sane_rowcount=}")
Review Comment:
I think this print can be removed
##########
pyiceberg/catalog/sql.py:
##########
@@ -268,16 +269,32 @@ def drop_table(self, identifier: Union[str, Identifier])
-> None:
identifier_tuple = self.identifier_to_tuple_without_catalog(identifier)
database_name, table_name =
self.identifier_to_database_and_table(identifier_tuple, NoSuchTableError)
with Session(self.engine) as session:
- res = session.execute(
- delete(IcebergTables).where(
- IcebergTables.catalog_name == self.name,
- IcebergTables.table_namespace == database_name,
- IcebergTables.table_name == table_name,
+ if self.engine.dialect.supports_sane_rowcount:
+ res = session.execute(
+ delete(IcebergTables).where(
+ IcebergTables.catalog_name == self.name,
+ IcebergTables.table_namespace == database_name,
+ IcebergTables.table_name == table_name,
+ )
)
- )
+ if res.rowcount < 1:
+ raise NoSuchTableError(f"Table does not exist:
{database_name}.{table_name}")
+ else:
+ try:
+ tbl = (
+ session.query(IcebergTables)
+ .with_for_update(of=IcebergTables, nowait=True)
Review Comment:
Quick question: Why do we use `nowait` here? It looks like we're not
catching errors from concurrent `SELECT FOR UPDATE NOWAIT` attempts.
I think `skip_locked=True` may be better here, as it lets the existing
`NoResultFound` catch handle the case when the row is locked by another
session. What do you think?
--
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
