amogh-jahagirdar commented on code in PR #10755:
URL: https://github.com/apache/iceberg/pull/10755#discussion_r1688408490
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -597,6 +597,12 @@ public TableMetadata replaceProperties(Map<String, String>
rawProperties) {
.build();
}
+ // it's not safe for external client to call this directly, must be called by
+ // `Table.removeUnusedSpecs()`.
+ TableMetadata withSpecs(List<Integer> specIds) {
+ return new Builder(this).setSpecs(specIds).build();
+ }
Review Comment:
Hm I don't think I'd expose a `withSpecs` option which directly lets callers
set the spec. I know the comment says that external clients shouldn't call it,
but I believe in such scenarios it's better to avoid exposing the API.
The way I thought about this is we would expose a `RemovePartitionSpec`
update and expose `removePartitionSpec(Integer spec)` API on the
TableMetadata.Builder. An unfortunate implication of this approach is the REST
spec would also need to be updated, but even in the current approach with a
`SetPartitionSpecs` it would also need to be updated.
I think the Remove better fits the immutable pattern we have with other
cases like `AddSnapshot` and `RemoveSnapshot`. The change set also becomes a
lot more clear to analyze when debugging (there's an explicit "hey this spec is
being removed" as opposed to some list which someone needs to compare against
current).
--
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
