Skip to content

[LAKE-6732] Spark 3.5: refuse to rewrite a table with duplicate data file registrations - #6

Open
abrarsher23 wants to merge 1 commit into
1.8.1-affirm-patchfrom
abrar/guard-duplicate-file-registrations
Open

[LAKE-6732] Spark 3.5: refuse to rewrite a table with duplicate data file registrations#6
abrarsher23 wants to merge 1 commit into
1.8.1-affirm-patchfrom
abrar/guard-duplicate-file-registrations

Conversation

@abrarsher23

Copy link
Copy Markdown

Problem

A data file registered at two data sequence numbers cannot be compacted safely. This is not
something a healthy writer produces — it arises when a commit is retried after its outcome became
unknown (CommitStateUnknownException) and the retry re-registers a WriteResult that had in fact
already been applied server-side.

File identity in the rewrite path is keyed on location alone, and a file's data sequence number
lives on the manifest entry rather than on the ContentFile, so the two registrations cannot be
told apart:

  • Data side removes one of two. RewriteFileGroup.rewrittenFiles() collects into a
    DataFileSet keyed on location(). Two scan tasks for one path collapse to a single DataFile
    carrying a single manifestLocation(), canTrustManifestReferences stays true, and the other
    manifest is never opened.
  • Delete side removes two of two. Delete files come from the entries metadata table via
    SparkDeleteFile; SparkContentFile never overrides manifestLocation(), so it returns null,
    allDeletesReferenceManifests becomes false, and every manifest is filtered.

Net effect: the surviving data registration is left with no delete file covering it, and rows
that were correctly suppressed become visible as duplicates.

Observed in production. One physical file, 30 records, registered at seq 93949 and seq 93950.
rewrite_data_files removed only the 93949 registration; remove_dangling_deletes then removed
both registrations of both its delete files — including a position delete its own test correctly
judged not dangling. 23 rows across 6 keys surfaced.

Change

Adds validate-duplicate-file-registrations, default true, checked immediately after
validateAndInitOptions() and before planFileGroups() so the action fails before making any
commit:

loadMetadataTable(table, MetadataTableType.ENTRIES)
    .filter("data_file.content == 0 AND status < 2")   // live DATA entries
    .selectExpr("data_file.file_path as file_path", "sequence_number")
    .distinct()
    .groupBy("file_path")
    .count()
    .filter("count > 1")

On a hit it throws ValidationException naming up to 10 affected paths and pointing at the opt-out.

Cost is one scan of the entries table projecting two columns — cheaper than the two
RemoveDanglingDeletesSparkAction already performs.

Design notes

  • Throws rather than warns. Compacting a table in this state corrupts it, so refusing is the
    correct behaviour. Scheduled compaction will fail loudly for an affected table; that is intended.
  • Default on. A guard that is off by default would not have caught this.
  • Escape hatch retained via validate-duplicate-file-registrations=false, for compacting such a
    table deliberately.

Relationship to #4

Complementary, not overlapping. #4 makes remove_dangling_deletes err safe by retaining any delete
file attached to a live FileScanTask, which holds the damage at 2 copies per key instead of 7. It
does not prevent the initial doubling, which happens in rewrite_data_files before the dangling
action runs. This PR stops the rewrite from starting at all.

Testing

TestRewriteDataFilesAction:

  • testRejectsDuplicateDataFileRegistrations — re-appends an already-live DataFile to produce a
    genuine second registration, asserts ValidationException naming the path.
  • testDuplicateDataFileRegistrationCheckCanBeDisabled — opt-out still runs.
  • testAcceptsTableWithoutDuplicateRegistrations — no regression on a clean table.

Not built or run locally. This was authored against the merged 1.8.1-affirm-patch tree via
the GitHub API without a local Gradle build, so compilation and the three new tests are unverified
and rely on CI. Please confirm CI is green before reviewing in detail.

Upstream

Not fixed upstream. apache/iceberg#15727 is open and unmerged, and the same location-keyed
identity is present at 1.9.0, 1.10.0 and main. A version bump does not address this. Worth filing
upstream separately — the durable fix is keying file identity on
(location, data_sequence_number), which requires plumbing the manifest entry's sequence number
into the identity.

…file registrations

A data file registered at two data sequence numbers cannot be compacted safely.
File identity in the rewrite path is keyed on location alone, so the two
registrations are indistinguishable: the data side removes one of them while the
delete side removes both corresponding delete-file registrations. The surviving
data registration is then left with no delete file covering it and previously
suppressed rows become visible.

Adds validate-duplicate-file-registrations (default true), checked before
planFileGroups so the action fails before making any commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant