[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1879: Core: Support TableBuilder in CachingCatalog

[GitBox <gi...@apache.org>]

aokolnychyi commented on a change in pull request #1879:
URL: https://github.com/apache/iceberg/pull/1879#discussion_r537401615



##########
File path: core/src/main/java/org/apache/iceberg/CachingCatalog.java
##########
@@ -160,4 +159,90 @@ private void invalidate(TableIdentifier ident) {
 
     return builder.build();
   }
+
+  @Override
+  public TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
+    return new CachingTableBuilder(identifier, schema);
+  }
+
+  private class CachingTableBuilder implements TableBuilder {
+    private final TableIdentifier ident;
+    private final TableBuilder innerBuilder;
+
+    private CachingTableBuilder(TableIdentifier identifier, Schema schema) {
+      this.innerBuilder = catalog.buildTable(identifier, schema);
+      this.ident = identifier;
+    }
+
+    @Override
+    public TableBuilder withPartitionSpec(PartitionSpec spec) {
+      innerBuilder.withPartitionSpec(spec);
+      return this;
+    }
+
+    @Override
+    public TableBuilder withSortOrder(SortOrder sortOrder) {
+      innerBuilder.withSortOrder(sortOrder);
+      return this;
+    }
+
+    @Override
+    public TableBuilder withLocation(String location) {
+      innerBuilder.withLocation(location);
+      return this;
+    }
+
+    @Override
+    public TableBuilder withProperties(Map<String, String> properties) {
+      innerBuilder.withProperties(properties);
+      return this;
+    }
+
+    @Override
+    public TableBuilder withProperty(String key, String value) {
+      innerBuilder.withProperty(key, value);
+      return this;
+    }
+
+    @Override
+    public Table create() {
+      AtomicBoolean created = new AtomicBoolean(false);
+      Table table = tableCache.get(canonicalizeIdentifier(ident), identifier -> {
+        created.set(true);
+        return innerBuilder.create();
+      });
+
+      if (!created.get()) {
+        throw new AlreadyExistsException("Table already exists: %s", ident);
+      }
+
+      return table;
+    }
+
+    @Override
+    public Transaction createTransaction() {
+      // create a new transaction without altering the cache. the table doesn't exist until the transaction is
+      // committed. if the table is created before the transaction commits, any cached version is correct and the
+      // transaction create will fail. if the transaction commits before another create, then the cache will be empty.
+      return innerBuilder.createTransaction();
+    }
+
+    @Override
+    public Transaction replaceTransaction() {
+      // create a new transaction without altering the cache. the table doesn't change until the transaction is
+      // committed. when the transaction commits, invalidate the table in the cache if it is present.
+      return CommitCallbackTransaction.addCallback(
+          innerBuilder.replaceTransaction(),
+          () -> tableCache.invalidate(canonicalizeIdentifier(ident)));

Review comment:
       Hm, is this correct? We are calling `invalidate` on `tableCache` directly instead of the dedicated method in `CachingCatalog` that also invalidates metadata tables.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org