API: Define RepairManifests action interface - #17399
Conversation
Co-authored-by: Mathew Fournier <160646114+tabmatfournier@users.noreply.github.com> (cherry picked from commit eb2041b)
Narrows the RepairManifests action interface to its first-cut scope: repairing incorrect manifest entry statistics, with a dry-run option. - Drop repairFileEntries() and the duplicate/missing-file Result methods. Removing duplicate entries and dropping entries for missing files changes the logical contents of the table (rows deleted or, for delete files, resurrected) and warrants its own design; it can be added back as a follow-up once those semantics are settled. - Repairing entry stats leaves table contents unchanged and preserves the live file count, so it is a self-contained first step. - Tighten the javadoc: describe what each method does and add @return tags and the missing periods.
| * | ||
| * @return this for method chaining | ||
| */ | ||
| RepairManifests dryRun(); |
There was a problem hiding this comment.
i think its inconsistent with remove-orphan-files dryRun but that pattern (supplying a no-op delete) doesnt really apply here. So im ok with an explicit flag.
There was a problem hiding this comment.
agree that we don't have a catch-all consumer for both incorrect manifest-list or manifest entry, but I still think it would be beneficial to at least know which files are examined with during dry-run. I think it might help the replication to be more resilient if we run this action on both source and target and ensure the result file set are the same.
There was a problem hiding this comment.
@dramaticlly sorry my comment lacked context, its from my reaidng of #10784 (comment) Whether we need a dryRun or not.
I think we need, even though its the not the same as RemoveOrphanFiles where its on the procedure and not action
| Iterable<ManifestFile> rewrittenManifests(); | ||
|
|
||
| /** Returns the number of manifest entries for which stats were incorrect. */ | ||
| long entryStatsIncorrectCount(); |
There was a problem hiding this comment.
should we just have one? Only one count should be populated right (depending on dryRun)?
| long entryStatsIncorrectCount(); | ||
|
|
||
| /** Returns the number of manifest entries for which stats were corrected. */ | ||
| long entryStatsRepairedCount(); |
| /** The action result that contains a summary of the execution. */ | ||
| interface Result { | ||
| /** Returns rewritten manifests. */ | ||
| Iterable<ManifestFile> rewrittenManifests(); |
There was a problem hiding this comment.
also notice the rest of API says 'repaired'. here it is 'rewritten'. let's make them the same
4 participants
Summary
Reviving #10784 by @amogh-jahagirdar (co-authored with @tabmatfournier). See #10445 for motivation. Scoped down to just repairing stats for now
Problem: manifest entry stats can disagree with the files they describe —
record_count,file_size_in_bytes, column bounds and null counts written wrong by a non-reference writer. Data files are fine, only the metadata is wrong, so you get bad pruning or read failures on wrong sizes.RewriteManifestsdoesn't help, it carries over whatever entries it finds. Finding the bad entries means stat-ing or reading the footer of every file in a live manifest entry, so this needs to be a distributed action.This defines the action interface only. Spark implementation and procedure to follow.