bharos commented on code in PR #10767:
URL: https://github.com/apache/gravitino/pull/10767#discussion_r3081883612
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableHookDispatcher.java:
##########
@@ -75,7 +72,22 @@ public LoadTableResponse updateTable(
IcebergRequestContext context,
TableIdentifier tableIdentifier,
UpdateTableRequest updateTableRequest) {
- return dispatcher.updateTable(context, tableIdentifier,
updateTableRequest);
+ LoadTableResponse response =
+ dispatcher.updateTable(context, tableIdentifier, updateTableRequest);
+ // Import the table and set ownership if the entity does not yet exist,
+ // e.g. when committing a staged table create.
+ try {
Review Comment:
Good catch, thanks @roryqi . You're right, updateTable serves both regular
property updates and staged create commits, so we shouldn't unconditionally
import/set ownership there.
I've updated the code to check for UpdateRequirement.AssertTableDoesNotExist
in the request's requirements before proceeding with import and ownership. This
is the same check Iceberg's own CatalogHandlers.isCreate() uses to distinguish
staged create commits from regular updates. Only when that requirement is
present (and the entity doesn't already exist in the store) do we import the
table and set ownership.
Also added more details on this fix in the issue here :
https://github.com/apache/gravitino/issues/10766#issuecomment-4246543692
Please let me know if this looks good now, thanks
--
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]
