dimas-b commented on code in PR #4939:
URL: https://github.com/apache/polaris/pull/4939#discussion_r3571668467
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1205,84 +1297,137 @@ public void commitTransaction(CommitTransactionRequest
commitTransactionRequest)
// LinkedHashMap preserves insertion order for deterministic processing.
Map<TableIdentifier, List<UpdateTableRequest>> changesByTable = new
LinkedHashMap<>();
for (UpdateTableRequest change : commitTransactionRequest.tableChanges()) {
- if (CatalogHandlerUtils.isCreate(change)) {
- throw new BadRequestException(
- "Unsupported operation: commitTranaction with
updateForStagedCreate: %s", change);
- }
changesByTable.computeIfAbsent(change.identifier(), k -> new
ArrayList<>()).add(change);
}
- // Process each table's changes in order.
- // Note: All UpdateTableRequests for a given table are coalesced into a
single metadata
- // update and a single tableOps.commit(), which results in one Polaris
entity update per
- // table. This is subtly different from applying each UpdateTableRequest
as an independent
- // commit (as if each were under a lock). Requirements are still validated
sequentially
- // against the evolving metadata, so conflicts are detected correctly.
- // See also the TODO in TransactionWorkspaceMetaStoreManager for a more
general (but more
- // complex) alternative that would intercept at the MetaStoreManager layer.
+ // Process each table's changes in order. Both staged-creates and regular
updates are
+ // processed within the transaction workspace — creates buffer into
pendingCreations,
+ // updates buffer into pendingUpdates.
List<TableMetadata> tableMetadataObjs = new ArrayList<>();
- changesByTable.forEach(
- (tableIdentifier, changes) -> {
- Table table = baseCatalog.loadTable(tableIdentifier);
- if (!(table instanceof BaseTable baseTable)) {
- throw new IllegalStateException("Cannot wrap catalog that does not
produce BaseTable");
- }
-
- TableOperations tableOps = baseTable.operations();
- TableMetadata baseMetadata = tableOps.current();
-
- // Apply each change sequentially: validate requirements against
current state,
- // then apply updates. This ensures conflicts are detected (e.g., if
two changes
- // both expect schema ID 0, the second will fail after the first
increments it).
- TableMetadata currentMetadata = baseMetadata;
- for (UpdateTableRequest change : changes) {
- // Validate requirements against the current metadata state
- final TableMetadata metadataForValidation = currentMetadata;
- change
- .requirements()
- .forEach(requirement ->
requirement.validate(metadataForValidation));
-
- // TODO: Refactor to share/reconcile the update-application logic
below with
- // CatalogHandlerUtils to avoid divergence as complexity grows.
- TableMetadata.Builder metadataBuilder =
TableMetadata.buildFrom(currentMetadata);
- for (MetadataUpdate singleUpdate : change.updates()) {
- // Note: If location-overlap checking is refactored to be
atomic, we could
- // support validation within a single multi-table transaction as
well, but
- // will need to update the TransactionWorkspaceMetaStoreManager
to better
- // expose the concept of being able to read uncommitted updates.
- if (singleUpdate instanceof MetadataUpdate.SetLocation
setLocation) {
- if (!currentMetadata.location().equals(setLocation.location())
- && !realmConfig()
-
.getConfig(FeatureConfiguration.ALLOW_NAMESPACE_LOCATION_OVERLAP)) {
- throw new BadRequestException(
- "Unsupported operation: commitTransaction containing
SetLocation"
- + " for table '%s' and new location '%s'",
- change.identifier(), ((MetadataUpdate.SetLocation)
singleUpdate).location());
+ // Track staged-creates for deferred location validation after the real
metastore is restored.
Review Comment:
Thanks for confirming. I think it deserves a separate discussion. I think
it's way beyond the scope of current PR :sweat_smile:
--
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]
