feat: add rewrite manifests - #774

If the table was upgraded from v2, existing live files may still have null per-file row IDs. Then ManifestEntryAdapterV3::GetFirstRowId falls back to the writer-level value, so those files get 0, and the manifest list skips assignment because the manifest now has a non-null first_row_id. Java leaves the rewrite writer’s manifest firstRowId null via newManifestWriter, allowing manifest-list assignment when needed after upgrade.

Good catch. I removed the non-null rewrite writer first_row_id logic and added a v2 -> v3 regression test.

Java does not rewrite at all when clusterByFunc == null, while this rewrites when predicate_ is set. Is it intentional?

Not intentional. I changed RequiresRewrite to match Java.

Closed manifests are kept only in this local result vector until Rewrite() returns, and new_manifests_ is assigned only after the call succeeds in Apply(). If a later length(), WriteExistingEntry, Close(), or
ToManifestFile() returns an error, finalization/cleanup no longer has the already-created manifest paths.

Could we either append closed manifests to member state as soon as they are produced, or clean up the local partial result on the error path? It would also be good to add a test that injects an apply-time manifest writer failure after at least one rolled manifest has been closed.

Good catch. I'll track rewritten manifests once each writer is closed so that apply-time failures can clean up already-created manifests. Will add a regression test too.

Why this function returns string instead of size_t just like what C++ hash function does?

We should combine deleted_manifests_ and deleted_manifest_paths_ into a single unordered_set<ManifestFile> by using pure ManifestFile.path in its hash function.

Same for rewritten_manifests_ and rewritten_manifest_paths_ below.

Why do we need this? This class only rewrites data manifests, right?

Why do we need this? I didn't find this in the Java code.

I have to admit that SnapshotCache is not a good name. Perhaps we should rename it to SnapshotReader or something.

Why do we need this kind of change?

Review comment from Codex: This registers standalone updates for commit retries, but explicit transaction updates still return from txn->Apply without calling Finalize on failure. A failed rewrite can leave generated manifests behind. Please finalize or abort transaction-owned updates on apply failure and add a Transaction::NewRewriteManifests fault-injection test.

Why do we need this?

Let's move this function to src/iceberg/manifest/manifest_util_internal.h and follow the style of CopyAppendManifest?

The generated path is lost after MakeWriter succeeds. If writing, closing, or ToManifestFile fails, the output is not reliably closed or tracked for cleanup. Please retain the path and manage it with RAII until the manifest is completed successfully.

Unfortunately, CopyAppendManifest in the manifest_util_internal.h has the same issue.

cluster_by_func_ is user code and may throw. An exception here bypasses the close-all loop and leaves partial manifests untracked. Please make writer cleanup RAII and add a throwing-callback test.

Please require shared ownership and always register the update.

Please keep this temporary transaction detached. Remove ctx->transaction = ... from Transaction::Make(ctx); only explicit transactions should set it. Otherwise the next Commit() sees an expired transaction.

Please route the empty-update branch in Transaction::Commit() through the normal finalization block. Otherwise registered no-op updates skip Finalize().