[GitHub] [iceberg] nastra commented on a change in pull request #2957: Core: add TableMetadata update builder

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

nastra commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r709824337



##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -475,10 +488,7 @@ public TableMetadata withUUID() {
     if (uuid != null) {
       return this;
     } else {
-      return new TableMetadata(null, formatVersion, UUID.randomUUID().toString(), location,
-          lastSequenceNumber, lastUpdatedMillis, lastColumnId, currentSchemaId, schemas, defaultSpecId, specs,
-          lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties,
-          currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
+      return builderFrom(this).generateUUID().build();

Review comment:
       should this also have a `.withFile(null)`?

##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -500,11 +510,14 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
       builder.add(new Schema(newSchemaId, newSchema.columns(), newSchema.identifierFieldIds()));
     }
 
-    return new TableMetadata(null, formatVersion, uuid, location,
-        lastSequenceNumber, System.currentTimeMillis(), newLastColumnId,
-        newSchemaId, builder.build(), defaultSpecId, updatedSpecs, lastAssignedPartitionId,
-        defaultSortOrderId, updatedSortOrders, properties, currentSnapshotId, snapshots, snapshotLog,
-        addPreviousFile(file, lastUpdatedMillis));
+    return builderFrom(this)

Review comment:
       should this have a `.withFile(null)` and `.withPreviousFiles(addPreviousFile(file, lastUpdatedMillis))`?

##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -676,26 +694,22 @@ private TableMetadata setCurrentSnapshotTo(Snapshot snapshot) {
         .add(new SnapshotLogEntry(nowMillis, snapshot.snapshotId()))
         .build();
 
-    return new TableMetadata(null, formatVersion, uuid, location,
-        lastSequenceNumber, nowMillis, lastColumnId, currentSchemaId, schemas, defaultSpecId, specs,
-        lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties, snapshot.snapshotId(), snapshots,
-        newSnapshotLog, addPreviousFile(file, lastUpdatedMillis));
+    return builderFrom(this)
+        .refreshLastUpdateMillis()

Review comment:
       does this actually need to be equal to `nowMillis`? because when we reach this part, the millis will be different for the `SnapshotLogEntry` and for `TableMetadata`

##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -538,11 +551,12 @@ public TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
       builder.add(freshSpec(newDefaultSpecId, schema, newPartitionSpec));
     }
 
-    return new TableMetadata(null, formatVersion, uuid, location,
-        lastSequenceNumber, System.currentTimeMillis(), lastColumnId, currentSchemaId, schemas, newDefaultSpecId,
-        builder.build(), Math.max(lastAssignedPartitionId, newPartitionSpec.lastAssignedFieldId()),
-        defaultSortOrderId, sortOrders, properties,
-        currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
+    return builderFrom(this)
+        .refreshLastUpdateMillis()
+        .withDefaultSpecId(newDefaultSpecId)
+        .withSpecs(builder.build())
+        .withLastAssignedPartitionId(Math.max(lastAssignedPartitionId, newPartitionSpec.lastAssignedFieldId()))
+        .build();

Review comment:
       I think it's a bit confusing when some stuff is set via builder methods and other stuff is set when `build()` is called (such as it's the case for `previousFiles`). IMO it would be more consistent when everything that needs to be set is set via builder methods. What could be still added to the `build` method is potential validation logic (if necessary)




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