[core] Optimize data evolution compaction planning - #9177
Conversation
47dff91 to
fc9c6f1
Compare
|
I found a correctness issue in the two-phase planning flow when a dedicated file spans multiple normal row-id ranges. A A minimal example is:
The candidate collector correctly returns
I reproduced this deterministically and also found it with a 20,000-iteration differential test comparing:
The normal-only differential test passed, while the blob version found this mismatch. I think candidate boundaries must be preserved in phase two. One possible fix is to group normal files by their own contiguous row-id coverage first, and only then attach blob/vector files to their anchor normal range, so dedicated files cannot define normal-range connectivity. It would also be valuable to add Two additional concerns:
|
|
Thanks for the detailed reproduction. Addressed in
Added regression coverage for the bridge case, disjoint-task rejection, manifest pruning, and legacy one-scan behavior. The related 174 core tests, dependent Flink/Spark compile, Spotless, and |
|
Rechecked the latest update at The previously reported correctness blocker is resolved:
I reran the affected 174 core tests successfully. I also reran the 20,000-iteration normal and BLOB differential checks against full planning; both passed with the fix. The original spanning-BLOB reproduction now produces two independent normal tasks and no row-id-shifting task. I did not find another correctness issue in the updated implementation. The removal of |
leaves12138
left a comment
There was a problem hiding this comment.
Rechecked the latest update. The reported correctness issue is resolved, and the affected tests and differential checks pass.
|
Added the explicit DV materialization path in 3946674: Flink and Spark now expose CALL sys.materialize_deletion_vectors(...). Normal data-evolution compaction still preserves row IDs and rejects data-evolution.compaction.rewrite-row-ids=true; the new procedure physically applies DVs, reassigns surviving row IDs, drops affected global indexes, and enables snapshot conflict checks. Core, Flink end-to-end, Spark 3.5 DataEvolutionDeletionTest (16 tests), docs completeness, and Spotless all pass locally. The PR description and user docs have been updated. |
leaves12138
left a comment
There was a problem hiding this comment.
I found a blocker in the Spark multi-batch execution path for deletion-vector materialization.
DataEvolutionDeletionVectorMaterializeCoordinator intentionally returns full-metadata work in batches. However, CompactProcedure.executeDataEvolutionCompaction commits every taskPlanner.get() result separately, while every batch runs DataEvolutionCompactionCommitPreparation(table, snapshot) against the same original snapshot.
This breaks when two batches from the same partition touch deletion vectors stored in the same DV index file:
- Batch 1 materializes its range. The preparation rewrites the shared DV index file, removing batch 1's DV while preserving batch 2's DV, and the commit replaces the original index file.
- Batch 2 still prepares from the original snapshot. It therefore rewrites the already-replaced index file and also carries forward batch 1's now-stale DV.
- The second commit fails deterministically with a file-deletion conflict; my reproduction reaches
Trying to create deletion vector on file ... which is not previously added.
I reproduced the actual planner/commit loop by using one partition with two disjoint manifest groups, putting both DVs in one index file, and reducing FILES_PER_BATCH to force two plan() calls. The first batch commits and the second batch fails. With the production limit, the same path is reached when an earlier manifest-group batch contains at least 100,000 live files and a later group in the same partition shares a DV index file.
The existing tests collect every planned task and prepare/commit them once, so they do not exercise the Spark behavior at lines 561-639.
Please either prepare and commit all materialization batches atomically (similar to the Flink non-parallel preparation operator), or maintain DV rewrite state correctly across batches without reloading the original DV index state. The latter must still reject an external concurrent DV update. A configurable batch-size regression test would be valuable. The same stale-snapshot pattern should also be checked for regular Spark data-evolution compaction when a DV index file spans planner batches.
leaves12138
left a comment
There was a problem hiding this comment.
Rechecked the update at 1e12e1e759 and the previous Spark multi-batch correctness blocker is resolved.
The Spark loop now advances the snapshot used for DV/index commit preparation after each successful batch, so a shared DV index file is maintained from the state produced by the preceding batch. At the same time, materialization continues to use the original planning snapshot as the row-ID conflict baseline, which preserves detection of external concurrent updates instead of treating earlier batches from this operation as conflicts.
I verified both new Spark 3.5 regressions independently:
- deletion-vector materialization across planner batches;
- normal data-evolution compaction preserving deletion vectors across planner batches.
I also reran 65 relevant core tests covering candidate collection, range batching, planner boundaries, dedicated-file association, DV rewriting/materialization, and serialization. All passed.
The overall separation now looks sound: projected metadata selects logical ranges, full metadata is read only for selected batches, normal-file connectivity is established before attaching dedicated files, and commit preparation centrally maintains DVs and invalidates global indexes. The optimistic commit provider also refreshes global-index deletions from the latest snapshot on retries. I did not find another correctness blocker in the current design.
leaves12138
left a comment
There was a problem hiding this comment.
The DV-first batching direction is a good optimization, and I rechecked the Spark/Flink separation and the existing multi-batch regressions. However, there is still a correctness blocker when a dedicated file spans beyond the selected DV anchor range. Please see the inline comment for a concrete reproduction.
I locally verified that the existing core materialization tests and both Spark 3.5 multi-batch tests still pass, while the dedicated-file boundary reproduction fails consistently.
| missingFiles); | ||
|
|
||
| ranges = Range.sortAndMergeOverlap(ranges); | ||
| rangeScan.withRowRanges(ranges); |
There was a problem hiding this comment.
The selected ranges are derived only from DV anchor files, but the resulting scan can include a dedicated file whose physical range extends beyond those anchors without including all normal files needed to cover that dedicated file. This creates an incomplete logical read group.
A local reproduction is:
- normal ranges:
[0,4],[5,9],[10,14]; - write a partial BLOB update as one file covering
[5,14]; - create a DV only for the anchor in
[5,9]; - run materialization.
This scan selects [5,9], includes the BLOB file [5,14] because it intersects, but excludes the normal file [10,14]. DataEvolutionMaterializeDeletionCompactTask then fails in DataEvolutionSplitRead with The merged rowCount 10 of blob file bunch should be aligned with normal files 5.
Please expand the batch to the complete normal-file coverage required by every included dedicated file (and repeat to closure if necessary), or otherwise ensure a materialization task never contains a partial dedicated-file read group. A regression with a BLOB/vector file spanning adjacent normal ranges would be useful.
Summary
DataEvolutionCompactRangePlanner; a single logical range may exceed it, while legacy manifests without row-id bounds use one full scan instead of repeatedly rescanning the same groupDataEvolutionNormalCompactTaskdata-evolution.compaction.rewrite-row-idsoption for compatibility, but rejecttrue; normal data-evolution compaction continues to preserve row IDs and logical deletionsmaterialize_deletion_vectorsprocedure for Flink and Spark to physically apply deletion vectors, assign new row IDs, remove the applied DVs, and drop affected global indexesWhy
DataEvolutionCompactCoordinatorpreviously loaded fullManifestEntryandDataFileMetaobjects for every live file before deciding which files needed compaction. Large data-evolution tables could therefore consume several gigabytes of heap and fail with OOM even when only a small subset of files were compact candidates.The new two-phase planning flow first scans only the projected fields needed to identify exact candidate row-id ranges. Full file metadata is then loaded only for selected ranges. For manifests with row-id bounds, each range batch carries only intersecting manifests. For legacy manifests, all candidate ranges are consolidated into one full scan to avoid repeated full-group scans. Candidate metadata uses primitive arrays with 32 bytes of payload per live file, substantially reducing the planning footprint.
The full-metadata result is not a strict 100,000-file bound: one logical candidate range, or files intersecting a selected range, may exceed the soft target. This preserves row-range atomicity and is now explicit in the implementation.
The former rewrite-row-ids compaction option mixed two operations with different contracts. Regular compaction must retain stable row IDs and logical deletion state, while physically removing deleted rows necessarily assigns new row IDs. The new procedure makes that destructive operation explicit:
Both Flink and Spark support optional
partitions,options, andwherearguments (partitionsandwhereare mutually exclusive). The operation batches work by row-id/manifest ranges, rewrites only batches containing deletion vectors, and uses the existing data-evolution compaction commit preparation to remove DVs and invalidate affected global indexes. It also enables snapshot-based row-ID conflict detection so concurrent changes fail instead of committing stale results. Vector-store file materialization is rejected until it has a safe implementation.Memory benchmark
A lightweight retained-heap benchmark used live normal files in one row-id/manifest group, which represents the old planner's problematic large-group case. Old and new modes ran in separate JVMs with Serial GC and full GC before and after allocation. The 500,000-file run used a 1.5 GiB heap; the 5,000,000-file run used a 4 GiB heap.
The old representation is a
List<ManifestEntry>containing fullDataFileMetaobjects aftercopyWithoutStats. The new retained value is the measured heap forCompactCandidateRangeCollector. The radix-array peak model adds its auxiliary sort array and counters; for 5 million files, sorting is bounded to one 1-million-entry chunk at a time.This benchmark isolates the candidate metadata retained during planning rather than measuring whole-process RSS. Actual coordinator memory also depends on manifest grouping, deleted-file identifiers, partitions, selected ranges, and the full metadata intersecting the current range batch, but the per-live-file reduction is representative of the OOM-sensitive part changed by this PR.
Impact
data-evolution.compaction.rewrite-row-ids=truefail fast and direct users to the explicit procedureValidation
mvn -pl paimon-core -Pfast-build -DwildcardSuites=none -Dtest=DataEvolutionCompactCoordinatorTest,CompactCandidateRangeCollectorTest,DataEvolutionCompactRangePlannerTest,DataEvolutionDeletionVectorTest,DataEvolutionTableTest,FullTextSearchBuilderTest,VectorSearchBuilderTest test(174 tests)mvn -pl paimon-core -Pfast-build -DwildcardSuites=none -Dtest=DataEvolutionDeletionVectorTest#testMaterializeDeletionVectors+testCompactRejectsRewriteRowIdsOption test(2 tests)mvn -pl paimon-flink/paimon-flink-common -Pfast-build -Pflink1 -DwildcardSuites=none -Dtest=DataEvolutionDeleteSqlITCase#testMaterializeDeletionVectorsProcedure testmvn -pl paimon-spark/paimon-spark-3.5 -am -Pfast-build -Pspark3 -DfailIfNoTests=false -DwildcardSuites=org.apache.paimon.spark.sql.DataEvolutionDeletionTest -Dtest=none test(16 tests)mvn -pl paimon-docs -am -Pfast-build -DfailIfNoTests=false -DwildcardSuites=none -Dtest=ConfigOptionsDocsCompletenessITCase testmvn -pl paimon-api,paimon-core,paimon-flink/paimon-flink-common,paimon-spark/paimon-spark-common,paimon-spark/paimon-spark-ut -Pflink1 -Pspark3 -DskipTests spotless:check