yuqi1129 commented on code in PR #13197:
URL: https://github.com/apache/gravitino/pull/13197#discussion_r4072177389
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -549,6 +551,7 @@ private EntityCombinedTable importTable(NameIdentifier
identifier) {
+ "when Table is renamed by external systems not controlled by
Gravitino. In this "
+ "case, we need to overwrite the stored entity to keep the
consistency.",
stringId);
Review Comment:
Fixed in e9bb2e9528. The current import path inserts a new name without
overwrite, so a copied ID conflicts at the unique key. After
EntityAlreadyExistsException, loadTable/loadSchema reloads once: a completed
concurrent import still succeeds, while an unresolved conflict with an external
StringIdentifier now returns an actionable IllegalArgumentException naming the
target, ID, and gravitino.identifier property. Added dispatcher tests for a
cross-schema table copy and cross-catalog schema copy, plus relational
insert-without-overwrite conflict checks on H2/MySQL/PostgreSQL. No EntityStore
API was added.
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalog.java:
##########
@@ -86,7 +86,13 @@ static boolean shouldValidateWarehouseProperty(String
backend, String warehouse)
@Override
public Capability newCapability() {
- return new
IcebergCatalogCapability(HierarchicalSchemaUtil.schemaSeparator());
+ Map<String, String> properties = entity().getProperties();
+ boolean hiveBackend =
+ properties != null
+ && IcebergCatalogBackend.HIVE
+ .name()
+
.equalsIgnoreCase(properties.get(IcebergConstants.CATALOG_BACKEND));
+ return new
IcebergCatalogCapability(HierarchicalSchemaUtil.schemaSeparator(), hiveBackend);
Review Comment:
Agreed. I reverted the Iceberg Hive-backend case normalization change and
its test in e9bb2e9528. This PR no longer changes name lookup behavior for
existing Iceberg-on-Hive catalogs; handling migration of already registered
mixed-case names should be designed separately.
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -585,6 +600,80 @@ private EntityCombinedTable importTable(NameIdentifier
identifier) {
.withHiddenProperties(table.hiddenProperties());
}
+ /**
+ * Tells an external rename apart from a copied id before an import re-binds
a row.
+ *
+ * <p>An import that finds a {@link StringIdentifier} but no row under this
name overwrites the
+ * row that owns the id. That is right after an external rename: the old
name is gone and the row
+ * should follow the table. It is wrong when the id was copied ({@code
CREATE TABLE t2 LIKE t1}
+ * carries {@code TBLPROPERTIES}, so does a copy tool or a restored backup):
the source table is
+ * still there, and re-binding would move its row and every attachment keyed
by that id (owner,
+ * tags, policies, role grants) to the copy. The store cannot tell the two
apart; only the
+ * external catalog can, so this asks it whether the id's current owner
still exists.
+ */
+ private NameIdentifier checkImportedIdNotCopied(NameIdentifier identifier,
long id) {
+ NameIdentifier currentOwner =
findRegisteredTableById(identifier.namespace(), id);
+ if (currentOwner == null || currentOwner.equals(identifier)) {
+ return currentOwner;
+ }
+ NameIdentifier catalogIdent = getCatalogIdentifier(identifier);
+ boolean distinctOwnerStillExists =
+ doWithCatalog(
+ catalogIdent,
+ c ->
+ c.doWithTableOps(
+ ops -> {
+ if (!ops.tableExists(currentOwner)) {
Review Comment:
Fixed in e9bb2e9528. When the external listing confirms a single case alias,
import reuses the existing registration without put/update, so alternating
loads do not rename the row or advance its version. If columns need an update,
loadTable uses the stored identifier. The table and schema alias tests now load
both spellings repeatedly and verify no store write or alias row.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/OccWriteSupport.java:
##########
@@ -68,6 +68,28 @@ public static <T> T findAndLockForOverwrite(
return current;
}
+ /**
+ * Refuses an overwrite whose stable ID is already owned by a live row under
another parent.
+ *
+ * <p>An import trusts the ID it finds in the external object. When that ID
was copied from
+ * another object (copied table properties, a restored backup), an upsert
keyed by the primary key
+ * would move the existing row, and every attachment keyed by that ID, to
the new name and parent.
+ * The lookup must lock the row so the decision holds until the transaction
ends. A same-parent
+ * match is allowed: that is how an external rename is re-registered.
+ *
+ * @param <T> the persistent object type
+ * @param byIdLockingLookup the locking lookup by stable ID
+ * @param sameParent checks whether the ID owner belongs to the target parent
+ * @throws EntityAlreadyExistsException if the ID belongs to a live row
under another parent
+ */
+ public static <T> void checkOverwriteIdNotOwnedByOtherParent(
+ Supplier<T> byIdLockingLookup, Predicate<T> sameParent) {
+ T owner = byIdLockingLookup.get();
+ if (owner != null && !sameParent.test(owner)) {
+ throw new EntityAlreadyExistsException("The entity ID already belongs to
a different parent");
Review Comment:
Fixed in e9bb2e9528. Both helpers now share requireSameParent, and the
table/schema/topic overwrite guards report the entity ID plus the owning and
target parent IDs. The dispatcher also provides a target/ID/property message
for cross-parent copied-ID imports.
--
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]
