amogh-jahagirdar commented on issue #13568: URL: https://github.com/apache/iceberg/issues/13568#issuecomment-3086052674
Ok I had some time to go through this today and @sqd I largely agree with your analysis. The issue in the current implementation is that we use the latest metadata after the expiration commit for determining if incremental file cleanup can be performed. However, the snapshot expiration could've removed all the non-main branches/tags; the current check would incorrectly select incremental file cleanup, which was written with the assumption that there's a single branch for the metadata that existed before the commit. However, in the current implementation, the previous reference could just be some tagged ancestor and so the assumptions to build a reliable ancestry ends up being incorrect. I think in a later version, maybe 2.0 we should actually go a step further and possibly look at deprecating/removing the incremental file cleanup. It was preserved over the years but also over the years more constructs have been an added to the table metadata structure which make incremental cleanup either not possible or really complicated, to the point where we should probably just do reachable cleanup even though it may be more expensive. My hunch on why this hasn't surfaced over the years this code has existed is 2-fold, a.) Many folks are using the spark snapshot expiration implementation which will always do reachable file cleanup (spark does the reachable analysis in a distributed manner) after commiting the expiration operation. b.) The folks who are using branching/tagging typically have multiple refs with longer retention, so generally the metadata after also has multiple refs and we select the reachable cleanup strategy. **The TLDR is this should be a case where we use reachable file cleanup and that decision is based on if the metadata we expired against had multiple refs. We should probably have some harder checks too to prevent incremental cleanup being enabled when the previous metadata had multiple refs. ** I'll look into a fix for this. -- 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
