smaheshwar-pltr commented on code in PR #3380:
URL: https://github.com/apache/iceberg-python/pull/3380#discussion_r3262822676
##########
pyiceberg/catalog/__init__.py:
##########
@@ -323,6 +323,20 @@ def delete_data_files(io: FileIO, manifests_to_delete:
list[ManifestFile]) -> No
deleted_files[path] = True
+def _raise_if_view_exists(catalog: Catalog, identifier: str | Identifier) ->
None:
+ """Raise `TableAlreadyExistsError` if a view exists at the given
identifier.
+
+ Catalogs that don't support views raise `NotImplementedError` from
`view_exists` —
+ treat that as "no view at this identifier".
+ """
+ try:
+ view_collision = catalog.view_exists(identifier)
+ except NotImplementedError:
+ view_collision = False
+ if view_collision:
Review Comment:
Helper mirrors the pattern from Java
[`HiveCatalog`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L877-L882)
and Java REST's
[`RESTSessionCatalog.replaceTransaction`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L1035-L1037).
Re-uses `TableAlreadyExistsError` so existing `create_table_if_not_exists`
callers keep working. Catalogs without view support raise `NotImplementedError`
from `view_exists` — treat that as 'no view'.
##########
pyiceberg/catalog/__init__.py:
##########
@@ -920,6 +934,7 @@ def create_table_transaction(
sort_order: SortOrder = UNSORTED_SORT_ORDER,
properties: Properties = EMPTY_DICT,
) -> CreateTableTransaction:
+ _raise_if_view_exists(self, identifier)
Review Comment:
Deliberate divergence from Java: Java
[`HiveCatalog.ViewAwareTableBuilder`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L855-L883)
overrides `create()` with the view check but does not override
`createTransaction()`, leaving the eager-create path stricter than the txn
path. Checking here fixes that asymmetry — a `create_table_transaction` that
would collide with a view fails fast, before any metadata files are written.
##########
pyiceberg/catalog/sql.py:
##########
@@ -211,6 +212,7 @@ def create_table(
table_name = Catalog.table_name_from(identifier)
if not self.namespace_exists(namespace_identifier):
raise NoSuchNamespaceError(f"Namespace does not exist:
{namespace_identifier}")
+ _raise_if_view_exists(self, identifier)
Review Comment:
Mirrors Java
[`HiveCatalog.ViewAwareTableBuilder.create()`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L876-L882).
Java
[`JdbcCatalog`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java#L900-L919)
only checks views on `replaceTransaction`, not on create — runtime no-op here
since `SqlCatalog.view_exists` raises `NotImplementedError`, but the helper
keeps the call site uniform across catalogs.
##########
pyiceberg/catalog/sql.py:
##########
@@ -263,6 +265,7 @@ def register_table(self, identifier: str | Identifier,
metadata_location: str, o
table_name = Catalog.table_name_from(identifier)
if not self.namespace_exists(namespace):
raise NoSuchNamespaceError(f"Namespace does not exist:
{namespace}")
+ _raise_if_view_exists(self, identifier)
Review Comment:
Mirrors Java
[`HiveCatalog.registerTable`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L927-L943).
Java
[`JdbcCatalog`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java)
inherits `registerTable` from `BaseMetastoreCatalog` without a view check —
runtime no-op here for the same reason as `create_table`.
##########
pyiceberg/catalog/sql.py:
##########
@@ -376,6 +379,7 @@ def rename_table(self, from_identifier: str | Identifier,
to_identifier: str | I
to_table_name = Catalog.table_name_from(to_identifier)
if not self.namespace_exists(to_namespace):
raise NoSuchNamespaceError(f"Namespace does not exist:
{to_namespace}")
+ _raise_if_view_exists(self, to_identifier)
Review Comment:
Mirrors Java
[`HiveCatalog.renameTableOrView`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L367-L369).
Java
[`JdbcCatalog.renameTable`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java#L368-L370)
does the check too but gates it on `schemaVersion == V1`. PyIceberg's
SqlCatalog has no equivalent V1/V2 split — `view_exists` raises
`NotImplementedError`, so this is a no-op at runtime.
##########
pyiceberg/catalog/hive.py:
##########
@@ -413,6 +414,7 @@ def create_table(
ValueError: If the identifier is invalid.
"""
properties = {**DEFAULT_PROPERTIES, **properties}
+ _raise_if_view_exists(self, identifier)
Review Comment:
1:1 with Java
[`HiveCatalog.ViewAwareTableBuilder.create()`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L876-L882).
Hive is the only PyIceberg catalog that actually implements `view_exists`, so
this is the call site where the check fires for real.
##########
pyiceberg/catalog/hive.py:
##########
@@ -461,6 +463,7 @@ def register_table(self, identifier: str | Identifier,
metadata_location: str, o
if overwrite:
raise NotImplementedError("`overwrite` isn't supported")
+ _raise_if_view_exists(self, identifier)
Review Comment:
1:1 with Java
[`HiveCatalog.registerTable`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L941-L943).
##########
pyiceberg/catalog/hive.py:
##########
@@ -700,6 +703,7 @@ def rename_table(self, from_identifier: str | Identifier,
to_identifier: str | I
if self.table_exists(to_identifier):
raise TableAlreadyExistsError(f"Table already exists:
{to_table_name}")
+ _raise_if_view_exists(self, to_identifier)
Review Comment:
1:1 with Java
[`HiveCatalog.renameTableOrView`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L367-L369).
The check is on the destination, matching Java.
##########
pyiceberg/catalog/glue.py:
##########
@@ -571,6 +572,7 @@ def create_table(
"""
database_name, table_name =
self.identifier_to_database_and_table(identifier)
+ _raise_if_view_exists(self, identifier)
Review Comment:
Java
[`GlueCatalog`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java)
has no view support and no check here; closest reference is
[`HiveCatalog.ViewAwareTableBuilder.create()`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L876-L882).
Runtime no-op until Glue gains view support — `view_exists` raises
`NotImplementedError`.
##########
pyiceberg/catalog/glue.py:
##########
@@ -621,6 +623,7 @@ def register_table(self, identifier: str | Identifier,
metadata_location: str, o
if overwrite:
raise NotImplementedError("`overwrite` isn't supported")
+ _raise_if_view_exists(self, identifier)
Review Comment:
Same shape as the other Glue call sites — no Java analog (Glue Java has no
view support), Hive reference
[`HiveCatalog.registerTable`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L941-L943).
Runtime no-op.
##########
pyiceberg/catalog/glue.py:
##########
@@ -772,6 +775,7 @@ def rename_table(self, from_identifier: str | Identifier,
to_identifier: str | I
"""
from_database_name, from_table_name =
self.identifier_to_database_and_table(from_identifier, NoSuchTableError)
to_database_name, to_table_name =
self.identifier_to_database_and_table(to_identifier)
+ _raise_if_view_exists(self, to_identifier)
Review Comment:
No Java Glue analog. Hive reference:
[`HiveCatalog.renameTableOrView`](https://github.com/apache/iceberg/blob/2f6606a247e2b16be46ca6c02fc4cfc2e17691e6/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L367-L369).
Runtime no-op.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
