ayushtkn commented on code in PR #4939:
URL: https://github.com/apache/polaris/pull/4939#discussion_r3563057233
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -284,6 +284,30 @@ default BaseResult bootstrapPolarisService(@NonNull
PolarisCallContext callCtx)
@NonNull EntitiesResult updateEntitiesPropertiesIfNotChanged(
@NonNull PolarisCallContext callCtx, @NonNull List<EntityWithPath>
entities);
+ /**
+ * Commits a batch of entity creations and property updates within a single
transaction.
+ *
+ * @param callCtx call context
+ * @param creates entities to create
+ * @param updates entities to update (compare-and-swap)
+ * @return result indicating success or failure
+ */
+ default @NonNull EntitiesResult commitTransactionBatch(
+ @NonNull PolarisCallContext callCtx,
+ @NonNull List<EntityWithPath> creates,
+ @NonNull List<EntityWithPath> updates) {
+ for (EntityWithPath create : creates) {
+ EntityResult result = createEntityIfNotExists(callCtx,
create.catalogPath(), create.entity());
Review Comment:
Thanx. Have added `AtomicOperationMetaStoreManager.commitTransactionBatch`
which should take care, I have added tests around that as well in the
`AtomicOperationMetaStoreManagerTest`
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java:
##########
@@ -1218,6 +1218,37 @@ void validateStagedTableCreate(TableIdentifier
tableIdentifier, TableMetadata ta
catalogEntity, tableIdentifier, resolvedNamespace, location,
storageLeafEntity));
}
+ /**
+ * Validates location overlap for an existing table whose locations changed.
Unlike {@link
+ * #validateStagedTableCreate}, this uses the parent (namespace) path for
resolution since the
+ * table already exists and its resolved path includes the table entity
itself.
+ */
+ void validateTableLocationUpdate(TableIdentifier tableIdentifier,
TableMetadata tableMetadata) {
+ PolarisResolvedPathWrapper resolvedTableEntities =
+ resolvedEntityView.getPassthroughResolvedPath(
Review Comment:
Added `validateNoOverlapWithinBatch` that runs first with a pairwise
`StorageLocation.isChildOf` check across all location-affecting entries
(creates + location-changing updates) in the batch
##########
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:
yes, what I could decode
--
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]
