Skip to content

[core] Refactor sort compact to produce COMPACT commits - #8631

Open
hbgstc123 wants to merge 23 commits into
apache:masterfrom
hbgstc123:fix_sort_compact_append_only
Open

[core] Refactor sort compact to produce COMPACT commits#8631
hbgstc123 wants to merge 23 commits into
apache:masterfrom
hbgstc123:fix_sort_compact_append_only

Conversation

@hbgstc123

@hbgstc123 hbgstc123 commented Jul 15, 2026

Copy link
Copy Markdown

Purpose

Follow up on #7595 per this review comment: sort compact should not be modeled as an OVERWRITE commit with extra base-snapshot handling. Instead, it should produce a normal COMPACT commit so the existing commit protocol can perform compact validation and conflict detection.

This PR is core-only, split from the previous ~4.5k-line change per review feedback. Flink/Spark wiring is in a stacked follow-up.

For append tables, this PR adds:

  1. SortCompactCommitMessageRewriter to rewrite write-only append commit messages into CompactIncrement messages.
  2. Base-snapshot deletion-vector metadata in SortCompactPlanMetadata, so later engine commit recovery can still clean up DVs if the base snapshot has expired.
  3. Concurrent DV drift detection at rewrite time, plus compact conflict messages, so concurrent DV writes cannot restore deleted rows.
  4. Abort helpers for written data files and newly persisted DV index files after rewrite/commit failure.

No Flink/Spark topology changes are included here.

Tests

  • SortCompactCommitMessageRewriterTest
    • rewrite to compact messages
    • multi-bucket / cross-bucket rewrite
    • deletion-vector cleanup, including concurrent DV writes and expired base snapshots
    • captured plan metadata serialization / round trip
    • compact commit success detection, including expired snapshot gaps and concurrent delete-only compact
    • inline compaction rejection
  • ConflictDetectionTest for compact + DV conflict messages

hbg and others added 5 commits July 16, 2026 09:58
Merge latest-snapshot deletion-vector metadata at rewrite time so concurrent DV writes are cleaned up, while keeping captured plan metadata for expired snapshots. Align Flink commit failure behavior with Spark by aborting write output when the COMPACT snapshot is not yet visible.

Co-authored-by: Cursor <cursoragent@cursor.com>
…sed by subclasses

Commit 0f5aecf changed commitUser, state, and write from protected to
private, but GlobalFullCompactionSinkWrite and LookupSinkWrite still access
them directly. This causes JDK 8 compilation failures. Restore protected
visibility to fix the build.

Co-authored-by: Cursor <cursoragent@cursor.com>
…pact restricts to append tables

SortCompactAction now explicitly rejects primary-key tables and only
supports bucket-unaware append tables. The dynamic-bucket test case
exercises unsupported behavior and fails on Flink 2.x CI, so remove it.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sort compact now commits as COMPACT instead of OVERWRITE. Concurrent
merge and sort compact on V2 delta row-level DV append tables can leave
duplicate visible rows, so skip that combination and keep coverage on
the V1 write path.

Co-authored-by: Cursor <cursoragent@cursor.com>
@hbgstc123
hbgstc123 force-pushed the fix_sort_compact_append_only branch from de2e634 to d462567 Compare July 16, 2026 02:01
@JingsongLi

Copy link
Copy Markdown
Contributor

There is a data correctness issue with “Sort compact” and concurrent DV updates. Since “Sort compact” generates output based on an old snapshot but deletes the latest DV from the input file upon commit, this can cause deleted data to be restored.

@hbgstc123

Copy link
Copy Markdown
Author

There is a data correctness issue with “Sort compact” and concurrent DV updates. Since “Sort compact” generates output based on an old snapshot but deletes the latest DV from the input file upon commit, this can cause deleted data to be restored.

Right, just pushed a fix

hbg and others added 3 commits July 17, 2026 14:02
Resolve conflicts in FlinkSink and FlinkSinkBuilder by keeping both
sort-compact writeProviderOverride/createAppendTableSink hooks and
master's BLOB descriptor reader factory wiring.

Co-authored-by: Cursor <cursoragent@cursor.com>
return this;
}

private void validateSortCompactInput(List<DataSplit> compactInputSplits) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this method and two options. We don't need to restrict this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

hbg and others added 2 commits July 24, 2026 00:49
Drop validateSortCompactInput and the warn/max input file options per review feedback.

Co-authored-by: Cursor <cursoragent@cursor.com>
…ct_append_only

Resolve conflict in BaseAppendDeleteFileMaintainer by keeping upstream javadoc.

Co-authored-by: Cursor <cursoragent@cursor.com>
@JingsongLi

Copy link
Copy Markdown
Contributor

dvMaintainer.persist() has already written a new DV file during the rewrite phase, but Flink/Spark’s failure cleanup only calls abort on the original writtenMessages; the new DV exists only in the rewritten compact messages and will not be cleaned up. During a delete-only compact, writtenMessages is even empty, and cleanup returns immediately. Retry attempts following conflicts or commit failures will continue to generate orphaned index files.

hbg and others added 2 commits July 27, 2026 12:06
@hbgstc123

Copy link
Copy Markdown
Author

dvMaintainer.persist() has already written a new DV file during the rewrite phase, but Flink/Spark’s failure cleanup only calls abort on the original writtenMessages; the new DV exists only in the rewritten compact messages and will not be cleaned up. During a delete-only compact, writtenMessages is even empty, and cleanup returns immediately. Retry attempts following conflicts or commit failures will continue to generate orphaned index files.

done


DataFileMeta assignSequenceNumber(long minSequenceNumber, long maxSequenceNumber);

default DataFileMeta assignFileSource(FileSource fileSource) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this method to interface default?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DataFileMeta is @public, so a default method avoids breaking external implementations (same pattern as Table#newHybridSearchBuilder). However the only implementation in Paimon project is PojoDataFileMeta, and it has a real override. Happy to make it abstract

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

hbg added 4 commits July 29, 2026 10:22
@JingsongLi

Copy link
Copy Markdown
Contributor

During a "delete-only sort compact" operation, if the commit fails but another compaction process has already deleted the same input files, isBatchCompactCommitSucceeded incorrectly determines that the current operation succeeded. Consequently, it skips the abort step, resulting in a leak of the DV index file generated during the operation. Minimal test case for stable reproduction: expected false, actual true.

hbg added 3 commits September 8, 2026 22:48
… compact

Matching success by removed input files can skip abort after a failed
delete-only commit when another compact already deleted the same inputs,
leaking the DV index files produced by rewrite.
# Conflicts:
#	paimon-core/src/main/java/org/apache/paimon/io/DataFileMeta.java
#	paimon-core/src/main/java/org/apache/paimon/io/ProjectedDataFileMeta.java
#	paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/action/SortCompactAction.java
#	paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkSink.java
#	paimon-flink/paimon-flink-common/src/test/java/org/apache/paimon/flink/source/TestChangelogDataReadWrite.java
#	paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/CompactProcedure.java
#	paimon-spark/paimon-spark-common/src/main/scala/org/apache/paimon/spark/commands/PaimonSparkWriter.scala
@JingsongLi

Copy link
Copy Markdown
Contributor

Given that this involves another 4,000 lines of changes, could you split the PR to keep the line count down?

hbg added 2 commits September 13, 2026 20:54
Move Flink/Spark wiring to a follow-up PR so the COMPACT-commit protocol can be reviewed independently.
@hbgstc123

Copy link
Copy Markdown
Author

Split per the review comment to keep the line count down.

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.

2 participants