amogh-jahagirdar commented on code in PR #13614: URL: https://github.com/apache/iceberg/pull/13614#discussion_r2223256780
########## core/src/main/java/org/apache/iceberg/RemoveSnapshots.java: ########## @@ -375,7 +375,7 @@ private void cleanExpiredSnapshots() { } if (incrementalCleanup == null) { - incrementalCleanup = current.refs().size() == 1; + incrementalCleanup = base.refs().size() == 1 && current.refs().size() == 1; Review Comment: I've upleveled some of the validation, but I do think if it's specified through the API option and it's incompatible with the state of the table we should just fail instead of falling back. I do think that we should probably revisit removing the whole `withIncremental` option (it's not on the API but there's complication to having to be defensive against bad input where most users don't even specify it). It was added for test comparisons a long time ago but I think we know that reachability is the safest and we should probably just let the implementation hide the complexity of cleanup selection. -- 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