dimas-b commented on code in PR #4939:
URL: https://github.com/apache/polaris/pull/4939#discussion_r3575038695
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageUtil.java:
##########
@@ -83,6 +86,51 @@ public class StorageUtil {
return Set.of(viewMetadata.location());
}
+ /**
+ * Returns true if the set of locations used by {@code current} differs from
the set used by
+ * {@code base}. Covers the table location as well as write.data.path and
write.metadata.path
+ * property overrides.
+ */
+ public static boolean locationsChanged(TableMetadata base, TableMetadata
current) {
+ return
!getLocationsUsedByTable(base).equals(getLocationsUsedByTable(current));
+ }
+
+ /**
+ * Checks that no two distinct tables in {@code batch} use overlapping
locations (pairwise
+ * containment check across all data locations returned by {@link
#getLocationsUsedByTable}).
+ * Same-identifier pairs are skipped so a table's own write.data.path and
write.metadata.path
+ * don't count as self-overlap.
+ *
+ * @throws BadRequestException if any two distinct tables have overlapping
locations
+ */
+ public static void validateNoOverlapWithinBatch(
+ List<Map.Entry<TableIdentifier, TableMetadata>> batch) {
+ List<Map.Entry<TableIdentifier, StorageLocation>> entries =
+ batch.stream()
+ .flatMap(
+ e ->
+ getLocationsUsedByTable(e.getValue()).stream()
+ .map(loc -> Map.entry(e.getKey(),
StorageLocation.of(loc))))
+ .toList();
+ for (int i = 0; i < entries.size(); i++) {
+ for (int j = i + 1; j < entries.size(); j++) {
+ TableIdentifier idA = entries.get(i).getKey();
+ TableIdentifier idB = entries.get(j).getKey();
+ if (idA.equals(idB)) {
+ continue;
+ }
+ StorageLocation a = entries.get(i).getValue();
+ StorageLocation b = entries.get(j).getValue();
+ if (a.isChildOf(b) || b.isChildOf(a)) {
Review Comment:
We can probably sort locations alphabetically and avoid `O(n^2)`
performance. If a common prefix exists it's next entry will be the one that
conflicts. WDYT?
##########
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:
`commitTransactionBatch()` can work, but I do not see a JDBC impl. for it.
Are you planning to add one later?
##########
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:
Also, it feel that this method will become the focal point of a
`PolarisMetaStoreManager`. It must be implemented correctly by all MetaStore
managers.
With that in mind, I tend to think it might be best to reformulate all
MetaStore changes in terms of this method.
Taking this one step further, `TransactionState` introduced in this PR looks
very valuable to multi-object changes. Background (just FYI):
https://lists.apache.org/thread/rf5orxs815zs4h64p4rwp03q3pbgxb5r
https://lists.apache.org/thread/obvrrh0w36f87rwljptzwonpt89qkcm1
It feels like it may worth refactoring the MetaStore API in a more intrusive
way and actually make `TransactionState` (or a similar object) a central piece
of the API. Per-method "atomic" changes not longer look sufficient for the more
advanced use cases we're trying to implement here. The multitude of change
methods is hard to implement and those individual methods (like
`PolarisMetaStoreManager.createEntityIfNotExists()` seem to be too specific,
while being implementable through the "batch" change method.
What I'm trying to say is that using `TransactionState` in this PR just for
the narrow use case of `commitTransation` does not feel right to me. It feels
like we're making the most technically challenging aspect of the system a
corner case. I'd rather make it the main case and other simpler cases can ride
on top of it.
Sorry for increasing the scope of this PR, but I think it is time to have
this discussion again :)
--
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]
