rdblue commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r698076432
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -403,9 +403,26 @@ static TableMetadata fromJson(FileIO io, InputFile file,
JsonNode node) {
}
}
- return new TableMetadata(file, formatVersion, uuid, location,
- lastSequenceNumber, lastUpdatedMillis, lastAssignedColumnId,
currentSchemaId, schemas, defaultSpecId, specs,
- lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties,
currentVersionId,
- snapshots, entries.build(), metadataEntries.build());
+ return TableMetadata.builder()
Review comment:
I think the benefit of a builder for this class would be using it to
accumulate state as it is read, rather than keeping the local variables.
For example, rather than calling `snapshots.add` and then calling
`withSnapshots(snapshots)`, the builder could help by exposing a `addSnapshot`
method. But before adding that, I think it's worth considering what the purpose
of the builder is, and I don't think that it is to make parsing easier.
To me, the purpose of the builder is to make updating table metadata easier.
That's what I initially introduced the high-level methods for, like
`TableMetadata.replaceCurrentSnapshot`. I think a builder pattern works better
for implementing those methods since you can make multiple changes in the same
builder instead of building a new `TableMetadata` and calling a different
high-level method on it.
So in the end, I don't think that I would update this just yet like I said
in my first comment. And I would probably change the API of the builder in
general. If you want to use a builder here, then maybe two builders are
appropriate. One for building from existing table metadata and one for just
constructing the `TableMetadata` objects.
--
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]
