[GitHub] [iceberg] szehon-ho commented on a diff in pull request #6591: Core: Avoid creating new metadata file when `registerTable` API is used

["szehon-ho (via GitHub)" <gi...@apache.org>]

szehon-ho commented on code in PR #6591:
URL: https://github.com/apache/iceberg/pull/6591#discussion_r1082932748


##########
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##########
@@ -154,6 +154,12 @@ protected void disableRefresh() {
     this.shouldRefresh = false;
   }
 
+  protected String writeNewMetadataIfRequired(boolean isCreateTable, TableMetadata metadata) {

Review Comment:
   Should we use this in HiveTableOperations as well?



##########
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##########
@@ -154,6 +154,12 @@ protected void disableRefresh() {
     this.shouldRefresh = false;
   }
 
+  protected String writeNewMetadataIfRequired(boolean isCreateTable, TableMetadata metadata) {

Review Comment:
   I think we can remove 'is' in 'isCreateTable' following guideline for boolean args.  Ref: https://iceberg.apache.org/contribute/#java-style-guidelines
   
   In this case, what do you think of calling it 'newTable'?  (createTable will be a bit like dictating whether we want to create a table or not, whereas here we are describing whether the table is new)



##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java:
##########
@@ -102,7 +102,7 @@ public void doRefresh() {
 
   @Override
   public void doCommit(TableMetadata base, TableMetadata metadata) {
-    String newMetadataLocation = writeNewMetadata(metadata, currentVersion() + 1);
+    String newMetadataLocation = writeNewMetadataIfRequired(base == null, metadata);

Review Comment:
   Also to me, its clearer to add an extra line : 
   ```
   boolean newTable = base == null;
   String newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata);
   ```
   
   (if we keep the name I suggested above).



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