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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
