aokolnychyi commented on code in PR #11620:
URL: https://github.com/apache/iceberg/pull/11620#discussion_r1884369789


##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -158,6 +160,21 @@ public Map<String, String> properties() {
     return properties;
   }
 
+  public int formatVersion() {
+    return formatVersion;
+  }
+
+  private int formatVersion(Table table) {
+    if (table instanceof HasTableOperations) {
+      return ((HasTableOperations) 
table).operations().current().formatVersion();

Review Comment:
   Optional: What about an extra variable for `TableOperations`? We have an 
explicit casts and 3 method invocations on a single line. An extra variable may 
be easier to read and would be consistent with `metadataFileLocation`.



##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -158,6 +160,21 @@ public Map<String, String> properties() {
     return properties;
   }
 
+  public int formatVersion() {
+    return formatVersion;
+  }
+
+  private int formatVersion(Table table) {
+    if (table instanceof HasTableOperations) {
+      return ((HasTableOperations) 
table).operations().current().formatVersion();
+    } else {
+      // formatVersion=-1 will never be used/returned, because

Review Comment:
   Optional: We may actually avoid the override by doing something like below.
   
   ```
   public int formatVersion() {
     if (formatVersion == UNKNOWN_FORMAT_VERSION) {
       throw new UnsupportedOperationException("Format version is unknown");
     }
     return formatVersion;
   }
   
   private int formatVersion(Table table) {
     if (table instanceof HasTableOperations) {
       ...
     } else {
       return UNKNOWN_FORMAT_VERSION;
     }
   }
   ```
   
   This would also cover cases for regular tables that didn't have 
`TableOperations` under the hood.



-- 
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-unsubscr...@iceberg.apache.org

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


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

Reply via email to