Repository navigation
Spark 3.5: scan-based dangling delete removal (backport of apache/iceberg#15727 / port of #4) - #9
Open
davkim wants to merge 1 commit into
Conversation
… / #4) Ports #4 onto 1.11.0-affirm-patch. Still not in upstream 1.11 (apache#15727 is unmerged), so it has to travel with us. Replaces RemoveDanglingDeletesSparkAction's old partition-min-sequence-number heuristic with a full-scan ALL-minus-LIVE set complement, which also subsumes the separate findDanglingDvs() DV-orphan check that heuristic couldn't cover. Plus the two Affirm-only bits from #4: RewriteDataFilesSparkAction now runs dangling-delete cleanup on its zero-rewrite-groups early-return path (previously only ran after a successful rewrite, so a table stuck with zero rewritable data files could never shed its dangling deletes), and DeleteFileIndex treats a DV below its data file's sequence number as not-applicable instead of throwing, since detection is itself a scan-planning call that would otherwise abort on the exact stale DVs it exists to remove. API drift from the 1.8.1 base required adapting rather than a straight cherry-pick: - RewriteDataFilesSparkAction moved from planFileGroups() to the newer FileRewritePlan/planner.plan() planning API. The Affirm helper (removeDanglingDeletesAndBuild) had to switch from taking a raw Builder to taking the already-built Result: on 1.11 a rewrite can already report a non-zero removedDeleteFilesCount on its own (e.g. orphaned DVs dropped during the rewrite commit itself), and that's a lazily-computed sum over rewriteResults(), not a plain field -- setting it via a fresh Builder would silently discard that baseline instead of adding to it (caught by testRemoveDangledPositionDeletesPartitionEvolution's v3 case). - Dropped a 1.11-native "skip if single-spec unpartitioned" fast path in execute() (assumed ManifestFilterManager already drops table-wide dangling deletes on every commit). It doesn't hold for deletes committed standalone via newRowDelta() -- the same CDC write pattern that produces dangling deletes in prod -- and testUnpartitionedTable exercises precisely that gap. - Two copies of testPartitionedDeletesWithDanglingDvs (+ its liveEntries/ allEntries helpers) ended up in the test file post-merge: one already native to 1.11, one carried in by this backport, byte-identical. Kept one. - Lists.newArrayList() instead of new ArrayList<>() to satisfy this repo's checkstyle rule, not present in the 1.8.1-affirm-patch checkstyle config. #1 (REST retry semantics, also queued for this port) fully drops out: 1.11.0-affirm-patch already contains the upstream commits it was cherry-picking (apache#12818, apache#13352) verbatim -- confirmed by diff, not just by version number. Tested: iceberg-core DeleteFileIndex tests, TestRemoveDanglingDeleteAction (15 tests, 1 skipped, 0 failures), TestRewriteDataFilesAction (171 tests, 6 skipped, 0 failures), spotlessCheck + checkstyle clean on both modules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ports #4 onto
1.11.0-affirm-patch. Second of two backports queued for this base (#1, REST retry semantics, is covered separately -- see below, it drops out entirely).Same motivation as #4:
RemoveDanglingDeletesSparkAction's old partition-min-sequence-number heuristic has structural blind spots (monotonically-increasing keys, deletes committed standalone via CDCnewRowDelta()writes) that let dangling delete files accumulate without bound on affected tables. This replaces it with apache#15727's scan-based approach -- ask the query planner which delete files a reader would actually use (LIVE), diff against every delete file in the manifests (ALL), and remove the complement. Still not merged upstream, so it has to travel with us onto every base we cut.Changes
Same file scope as #4:
RemoveDanglingDeletesSparkAction.javaALL - LIVEimplementation. This also subsumes the separatefindDanglingDvs()DV-orphan check that already existed natively on 1.11 -- a DV pointing to a data file with no liveFileScanTaskis, by construction, inALLbut notLIVE, so a dedicated DV pass is redundant once the general scan-based check is in place.RewriteDataFilesSparkAction.javaplan.totalGroupCount() == 0), not just after a successful rewrite. This is the Affirm-only fix: a table stuck with zero rewritable data files (e.g. every remaining file already correctly sized) could never shed its dangling deletes before this, since cleanup only ran after a rewrite that would never happen.DeleteFileIndex.javaValidationException. Detection is itself a scan-planning call (RemoveDanglingDeletesSparkAction#findDanglingDeletescallsscan.planFiles()), so the throw would abort cleanup on exactly the stale DVs it exists to remove. Unreachable pre-format-v3.API drift from the 1.8.1 base
This needed real adaptation, not a clean cherry-pick --
git cherry-pick -m1on the merge commit landed 3 conflicts, all from 1.11-era refactors:RewriteDataFilesSparkActionmoved offplanFileGroups()onto the newerFileRewritePlan/planner.plan()planning API sometime between 1.8.1 and 1.11. The extractedremoveDanglingDeletesAndBuildhelper had to change from taking a rawBuilderto taking the already-builtResult: on 1.11, a rewrite can already report a non-zeroremovedDeleteFilesCount()on its own (e.g. orphaned DVs dropped as part of the rewrite commit itself), and that's a lazily-computed sum overrewriteResults(), not a plain stored field. Setting it via a freshBuildersilently discarded that baseline instead of adding to it -- caught bytestRemoveDangledPositionDeletesPartitionEvolution's v3 case (expectedremovedDeleteFilesCount = 1, got0, because the DV was already gone by the time the dangling-delete pass ran and the helper overwrote the baseline with its own "nothing left to find" count instead of adding to it).execute()(comment: "ManifestFilterManager already performs this table-wide delete on each commit"). That assumption doesn't hold for deletes committed standalone vianewRowDelta()-- the same write pattern that produces dangling deletes in prod in the first place -- andtestUnpartitionedTable(native to 1.11, not part of this backport) exercises exactly that gap. Keeping the fast path would have silently reintroduced a blind spot on unpartitioned tables and failed that pre-existing test.testPartitionedDeletesWithDanglingDvs(plus itsliveEntries/allEntrieshelpers) ended up in the test file post-merge -- one already native to 1.11 (upstream added DV-orphan detection + this exact test independently between 1.8.1 and 1.11), one carried in by this backport. Byte-identical bodies; kept one, deleted the duplicate (was a compile error otherwise).Lists.newArrayList()instead ofnew ArrayList<>()to satisfy this repo's checkstyleRegexpSinglelinerule, which isn't present in the 1.8.1-affirm-patch checkstyle config.On #1 (REST retry semantics)
Confirmed by diff, not just version number:
1.11.0-affirm-patchalready contains apache#12818 and apache#13352 verbatim --ErrorHandlersthrowsCommitStateUnknownExceptionon 503/500/502/504,ExponentialHttpRequestRetryStrategyexcludes 503 from the non-idempotent retry set while keeping it for idempotent requests, andHTTPClientthreads anHttpClientContextthroughexecute(). This PR is what #1 was cherry-picking from mid-2025 upstream, and it landed in the 1.11.0 release itself. Nothing to port.Testing
iceberg-coreDeleteFileIndextests: passTestRemoveDanglingDeleteAction: 15 tests, 1 skipped, 0 failuresTestRewriteDataFilesAction(full suite): 171 tests, 6 skipped, 0 failuresspotlessCheck+checkstyleMain/checkstyleTestclean oniceberg-coreandiceberg-spark-3.5_2.12Test plan for reviewers
removeDanglingDeletesAndBuildsignature change (Builder → Result) reads correctly againstImmutableRewriteDataFiles.Result's lazy-sum semanticsflink-scala-2-12-tests-style flakiness not yet observed)