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

Reply via email to