[GitHub] [iceberg] dimas-b commented on a diff in pull request #7893: Nessie: Minor Refactoring of NessieTableOperations

["dimas-b (via GitHub)" <gi...@apache.org>]

dimas-b commented on code in PR #7893:
URL: https://github.com/apache/iceberg/pull/7893#discussion_r1240049441


##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -523,4 +528,49 @@ public void close() {
       api.close();
     }
   }
+
+  public TableMetadata loadTableMetadataWithNessieSpecificProperties(
+      String metadataLocation, IcebergTable table, FileIO fileIO, String identifier) {
+    // Update the TableMetadata with the Content of NessieTableState.
+    TableMetadata deserialized = TableMetadataParser.read(fileIO, metadataLocation);

Review Comment:
   WDYT about keeping `TableMetadataParser.read()` at a higher level, and only updating the properties in this method? This will reduce the dependency of this class on actual I/O :thinking: 



##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -523,4 +528,49 @@ public void close() {
       api.close();
     }
   }
+
+  public TableMetadata loadTableMetadataWithNessieSpecificProperties(
+      String metadataLocation, IcebergTable table, FileIO fileIO, String identifier) {
+    // Update the TableMetadata with the Content of NessieTableState.
+    TableMetadata deserialized = TableMetadataParser.read(fileIO, metadataLocation);
+    Map<String, String> newProperties = Maps.newHashMap(deserialized.properties());
+    newProperties.put(NessieTableOperations.NESSIE_COMMIT_ID_PROPERTY, getRef().getHash());

Review Comment:
   If you're ok with this point and the comment about `FileIO`, then perhaps this method could be in `NessieUtil` as it will have little to do with the Nessie "Client" per se. WDYT?



##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -523,4 +528,49 @@ public void close() {
       api.close();
     }
   }
+
+  public TableMetadata loadTableMetadataWithNessieSpecificProperties(
+      String metadataLocation, IcebergTable table, FileIO fileIO, String identifier) {
+    // Update the TableMetadata with the Content of NessieTableState.
+    TableMetadata deserialized = TableMetadataParser.read(fileIO, metadataLocation);
+    Map<String, String> newProperties = Maps.newHashMap(deserialized.properties());
+    newProperties.put(NessieTableOperations.NESSIE_COMMIT_ID_PROPERTY, getRef().getHash());

Review Comment:
   Consistency between `metadataLocation` and `getRef()` is essentially ensured at the caller level. I think it makes sense to keep both of them as parameters to this method to emphasize that it is the responsibility of the caller to provide matching values for them.



-- 
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: issues-unsubscribe@iceberg.apache.org

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