[DeleteManifest] Making file validation optional

[haizhou-zhao]

Feature Request / Improvement

Background

This issue comes up when we are trying to fix a corrupted table where one manifest file (with active reference to data files) was (unintentionally) removed from the file system. At that point, our only way out was to remove the reference to that manifest file so that queries on the table does not fail with NotFoundException. This are the APIs we executed to achieve that:

table.rewriteManifests().deleteManifest(manifest).commit()

However, we found that Manifest rewrite implementation will force consistency on file counts, which means the above line of code will fail due to the following validation:

iceberg/core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java

Line 182 in c7de6cb

validateFilesCounts();

Feature Request

Although keeping file counts consistent with manifest rewrite serves most cases, in corner cases like fixing a corrupted table with manifest file deleted, user does intentionally want to lose all the data files referred by that missing manifest file (so that query against the table could start to work, and they can backfill the lost data later). We'd like to raise a feature request to make validateFilesCounts check optional on manifest rewrite, so that users have the choice to disable it by some configuration.

Query engine

None

[haizhou-zhao]

cc: @dramaticlly

[dramaticlly]

@aokolnychyi @szehon-ho I am wondering what do you think.

Basically the problem we are trying to resolve is remove existing manifests from table and I hope to achieve this with extra configuration like forced() in table.rewriteManifests().deleteManifest(manifest).forced().commit() so that we can skip the validation on active file count.

The reason to remove existing manifest can vary, but for our use case we are trying to recover from failure in #8599 , where REST client prematurely removed manifests on network timeout but the commit actual did go through on catalog side. So table ends up in a corrupted state where manifest file is deleted on file system but tracked in iceberg metadata.

[szehon-ho]

I guess they initially made rewriteManifest as a safe API.

I wonder, does DeleteFiles API achieve this? The next commit will remove that manifest if all its contents are removed.

If we really need a specific API, it sounds like we need deleteManifest method there.

[szehon-ho]

btw, did you look at that case? I didnt follow but from #8599 supoosedly it only happens when its instance of CleanableException

[dramaticlly]

thank you Szehon, I think DeleteFiles API only work with data files, delete files but not for manifest files. I tried to leverage delete by file path in DeleteFiles ->MergingSnapshotProducer but it does not work.

I agreed that deleteManifest sounds like a right way to do this instead of stub in rewriteManifests. I am aware of the fix 8599 above, it's just that some damage has occurred this time and we are looking forward to a way to fix corrupted table. There can be potentially other reasons for dropping a manifest from iceberg metadata, which is not relate to 8599.

[szehon-ho]

Sounds good. Im thinking adding deleteManifest method there is better, then.

I tried to leverage delete by file path in DeleteFiles ->MergingSnapshotProducer but it does not work.

Is it an issue we should fix, or expected behavior? Be good to fix that, if the former.

[szehon-ho]

Well, that was my thought as its parallel with appendManifest. But now I see #10396 try to deprecate that, so like to get @Fokko thought here. I think appendManifest/ deleteManifest are helpful, not sure the rationale to deprecate

[Fokko]

@szehon-ho sorry for the late reply here. I closed my PR, and I've left a comment with an explanation: #10396 (comment) The main problem is keeping the snapshot summary up to date.

[szehon-ho]

Thanks @Fokko !

[github-actions]

This issue has been automatically marked as stale because it has been open for 180 days with no activity. It will be closed in next 14 days if no further activity occurs. To permanently prevent this issue from being considered stale, add the label 'not-stale', but commenting on the issue is preferred when possible.