feat(commit): align functionality with java paimon commit by lucasfang · Pull Request #397 · alibaba/paimon-cpp

lucasfang · 2026-07-02T05:16:04Z

Purpose

Linked issue: close #xxx

Tests

API and Format

Documentation

Generative AI tooling

Copilot

Pull request overview

Note

Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR expands commit conflict detection and index-file handling in FileStoreCommitImpl, adds row-id/write-column conflict checking for data-evolution scenarios, and updates/extends unit tests accordingly.

Changes:

Add ConflictDetection, ManifestEntryChanges, and RowIdColumnConflictChecker to centralize commit change collection and conflict checks (including DV/global index considerations).
Support committing index files during overwrite / filter-and-overwrite flows, and adjust commit-kind selection (e.g., DV index append => overwrite kind).
Update commit retry behavior and add/adjust tests for index files, retries, and PK table commit behavior.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/paimon/core/operation/file_store_commit_test.cpp	Adds test validating DV index append triggers overwrite commit kind
src/paimon/core/operation/file_store_commit_impl_test.cpp	Extends commit tests for index files, retry metrics, and conflict checking signature changes
src/paimon/core/operation/file_store_commit_impl.h	Introduces new conflict/index APIs and wires in `ConflictDetection`
src/paimon/core/operation/file_store_commit_impl.cpp	Implements index-aware overwrite, retry commit checking, and centralized change collection
src/paimon/core/operation/file_store_commit.cpp	Removes prior PK-commit restriction logic in `Create`
src/paimon/core/operation/commit/row_id_column_conflict_checker.h	Adds row-id/write-column overlap conflict checker interface
src/paimon/core/operation/commit/row_id_column_conflict_checker.cpp	Implements row-id/write-column overlap conflict checker
src/paimon/core/operation/commit/manifest_entry_changes.h	Adds helper to collect manifest + index changes from commit messages
src/paimon/core/operation/commit/manifest_entry_changes.cpp	Implements change collection and “changed partitions” logic
src/paimon/core/operation/commit/manifest_entry_changes_test.cpp	Adds unit tests for `ManifestEntryChanges`
src/paimon/core/operation/commit/conflict_detection.h	Adds conflict detection API covering bucket/key-range/row-id/global-index checks
src/paimon/core/operation/commit/conflict_detection.cpp	Implements conflict detection logic and row-id history scan
src/paimon/core/operation/commit/conflict_detection_test.cpp	Adds unit test coverage for conflict detection behaviors
src/paimon/common/data/binary_row.h	Adds `std::hash` for tuple key used in conflict detection
src/paimon/CMakeLists.txt	Wires new sources + tests into the build

Comments suppressed due to low confidence (1)

src/paimon/core/operation/file_store_commit.cpp:1

The PR description is still the unfilled template (e.g., linked issue #xxx, tests section empty), but this change alters user-visible behavior by removing the previous PK-table commit restriction in FileStoreCommit::Create. Please update the PR title/description to clearly state the behavioral change, link the real issue, and list the tests validating the new PK commit behavior.

/*

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+    EXPECT_CALL(*mock_fs, ReadFile(testing::_, testing::_))
+        .Times(testing::AnyNumber())
+        .WillRepeatedly(testing::Invoke([&](const std::string& path, std::string* content) {
+            return mock_fs->FileSystem::ReadFile(path, content);
+        }));
    EXPECT_CALL(*mock_fs, ReadFile(testing::StrEq(latest_hint), testing::_))
        .WillRepeatedly(testing::Invoke([](const std::string& path, std::string* content) {
            *content = "-1";
            return Status::OK();
        }));


+    ::ArrowSchema arrow_schema;
+    ASSERT_TRUE(arrow::ExportSchema(*schema, &arrow_schema).ok());


+        for (const auto& write_col : file.write_cols.value()) {
+            auto field_id_result = ResolveFieldId(file, write_col);
+            if (!field_id_result.ok()) {
+                return false;
+            }
+            const auto& field_id = field_id_result.value();
+            if (field_id.has_value() && write_range.field_ids.count(field_id.value()) > 0) {
+                return true;
+            }
+        }


    Result<bool> commit_result = CommitSnapshotImpl(new_snapshot, delta_statistics);
    if (!commit_result.ok()) {
-        // commit exception, not sure about the situation and should not clean up the files.
-        PAIMON_LOG_WARN(logger_, "You need call FilterAndCommit to retry commit for exception. %s",
+        // commit exception is uncertain; retry after checking whether this commit already exists.
+        PAIMON_LOG_WARN(logger_, "Retry commit for exception. %s",
                        commit_result.status().ToString().c_str());
-
-        // To prevent the case where an atomic write times out but actually succeeds,
-        // retrying the commit could lead to the snapshot file being committed multiple times.
-        // Therefore, retries should be handled by the upper layer,
-        // which should call FilterAndCommit to avoid duplicate commits.
-        // Therefore, we should not trigger cleanup here,
-        // as it may delete meta files from a snapshot that was just written by ourselves,
-        // leading to an incomplete or corrupted snapshot.
        guard.Release();
-        return Status::Invalid("You need call FilterAndCommit to retry commit for exception. ",
-                               commit_result.status().ToString());
+        return false;
    }


zjw1111

Reviewed the commit conflict-detection alignment against Java Paimon. The port is largely faithful (ManifestEntryChanges collection order, RetryWaiter backoff math, SequenceSnapshotProperties ADD-only max, row-tracking assignment all match Java). A few correctness gaps and divergences are worth fixing before merge — see inline.

Highest priority: RowIdColumnConflictChecker::ConflictsWith swallows field-resolution errors and can let a schema mismatch pass conflict detection.

Also, a few items not inlined:

Test coverage: RowIdColumnConflictChecker has no C++ test at all (Java has 7 cases, incl. testUsesFieldIdAcrossRename / testFailsOnUnknownNonSystemWriteColumn which cover two of the issues below); CheckRowIdExistence / CheckRowIdRangeConflicts / SequenceSnapshotProperties also lack direct tests. Please add these.
Style (docs/code-style.md): plain int in row_id_column_conflict_checker.cpp binary search (low/high/mid/index) and std::unordered_set<int> in file_store_commit_impl.cpp NumChangedBuckets — use fixed-width types.
Intentional partial ports (please confirm and add a note): DV+UNAWARE entry enrichment, pkClusteringOverride short-circuit in CheckKeyRange, partition-expire handling in CheckDeleteInEntries, and dropStats on overwrite delete entries are all omitted.
The PR description is still the unfilled template — please fill in the change summary, linked issue, and tests.

zjw1111 · 2026-07-10T03:01:21Z

+
+        for (const auto& write_col : file.write_cols.value()) {
+            auto field_id_result = ResolveFieldId(file, write_col);
+            if (!field_id_result.ok()) {


ConflictsWith swallows ResolveFieldId errors as return false (i.e. no conflict). Java's fieldId() throws on an unknown non-system write column (RowIdColumnConflictCheckerTest.testFailsOnUnknownNonSystemWriteColumn), and the constructor path (AddWriteFieldIds) already propagates the same error via PAIMON_ASSIGN_OR_RAISE, so this is inconsistent. Please make ConflictsWith return Result<bool> and propagate the error to the caller in conflict_detection.cpp — otherwise a schema mismatch can silently pass conflict detection.

zjw1111 · 2026-07-10T03:01:21Z

+        return std::optional<int32_t>(it->second);
+    }
+
+    if (SpecialFields::IsSpecialFieldName(write_col)) {


The set used via IsSpecialFieldName (_SEQUENCE_NUMBER, _VALUE_KIND, rowkind, _ROW_ID, _INDEX_SCORE) diverges from Java SpecialFields.isSystemField (_KEY_ prefix + _SEQUENCE_NUMBER, _VALUE_KIND, _LEVEL, rowkind, _ROW_ID). As a result _LEVEL/_KEY_* are treated as user columns (error instead of skip) and _INDEX_SCORE is skipped (instead of error). Please use an isSystemField-equivalent check here rather than the shared IsSpecialFieldName.

zjw1111 · 2026-07-10T03:01:21Z

+    const std::optional<std::shared_ptr<RowIdColumnConflictChecker>>&
+        row_id_column_conflict_checker,
+    const Snapshot::CommitKind& commit_kind) const {
+    if (options_.DeletionVectorsEnabled() &&


This uses GetBucket() == UNAWARE_BUCKET (== 0), but an unaware append table defaults to bucket = -1 (ResolveBucketMode maps -1 && no PK to BUCKET_UNAWARE). Java guards on bucketMode.equals(BUCKET_UNAWARE). With DV enabled on a bucket=-1 unaware table this guard is skipped and the code falls through into bucket-aware conflict logic. Please check against the resolved bucket mode instead of the raw bucket number.

zjw1111 · 2026-07-10T03:01:21Z

-        // commit exception, not sure about the situation and should not clean up the files.
-        PAIMON_LOG_WARN(logger_, "You need call FilterAndCommit to retry commit for exception. %s",
+        // commit exception is uncertain; retry after checking whether this commit already exists.
+        PAIMON_LOG_WARN(logger_, "Retry commit for exception. %s",


When CommitSnapshotImpl returns a non-OK status, the error is dropped here and the loop retries; on timeout only the generic conflict message is thrown (and it prints the configured max-retries, not the actual retry count). Java carries the underlying exception in RetryCommitResult and rethrows it in the timeout message. Please preserve the last hard-error status and surface it on abort, and distinguish conflict-retry from hard errors so the root cause isn't lost.

zjw1111 · 2026-07-10T03:01:21Z

+void RowTrackingCommitUtils::AssignSnapshotId(int64_t snapshot_id,
+                                              const std::vector<ManifestEntry>& delta_files,
+                                              std::vector<ManifestEntry>* snapshot_assigned) {
+    for (const auto& entry : delta_files) {


ManifestEntry assigned_entry = entry; is a shallow copy sharing the same DataFileMeta, so AssignSequenceNumber/AssignFirstRowId mutate the caller's delta_files in place. Java's assign* return new immutable objects. Current callers overwrite delta_files right after, so it's latent, but any other holder of the same shared_ptr<DataFileMeta> gets silently polluted. Please clone the DataFileMeta before assigning (or have assign* return new objects).

zjw1111 · 2026-07-10T03:01:21Z

+Result<std::vector<ManifestEntry>> CommitScanner::ReadAllEntriesFromPartitions(
+    const Snapshot& snapshot,
+    const std::vector<std::map<std::string, std::string>>& partitions) const {
+    auto scan_filter = std::make_shared<ScanFilter>(/*predicate=*/nullptr, partitions,


The overwrite scan passes no bucket filter. Java OverwriteChangesProvider.newScan() applies bucket -> bucket >= 0 when numBucket != POSTPONE_BUCKET to exclude postpone-bucket files; without it, pending postpone-bucket files get DELETE entries on a normal overwrite. ScanFilter::bucket_filter_ (optional<int32_t>) also can't express bucket>=0 today. Please add a way to exclude negative buckets for the overwrite scan.

zjw1111 · 2026-07-10T03:01:21Z

+                                  entry.TotalBuckets(), entry.File());
+    }
+
+    for (const auto& change : changes_) {


This dedup (skip an ADD whose identifier already exists) diverges from Java buildResult(), which just addAll(newChanges). On an identifier collision the DELETE(X) is kept while ADD(X) is dropped, so after FileEntry::MergeEntries X is net-deleted (data loss). Unlikely to trigger given unique file names, but please align with Java (drop the dedup) or skip both the delete and the add together.

zjw1111 · 2026-07-10T03:01:21Z

+                   std::vector<IndexManifestEntry>* index_entries) const override;
+
+ private:
+    const std::vector<ManifestEntry>& delta_files_;


These providers hold delta_files/changelog_files/index_entries as const& members while being handed out as a shared_ptr (same for OverwriteChangesProvider, which also captures this by raw pointer). Current construction sites keep the referenced vectors alive during the synchronous TryCommit, so it's safe today, but it's a UAF footgun and differs from Java's value/GC capture. Please hold by value + std::move.

lucasfang added 2 commits July 2, 2026 11:47

feat(commit): align functionality with java paimon commit

1f2871c

fix

12d8869

Copilot AI review requested due to automatic review settings July 2, 2026 05:16

Copilot AI reviewed Jul 2, 2026

View reviewed changes

lucasfang changed the title ~~Dev~~ feat(commit): align functionality with java paimon commit Jul 2, 2026

Copilot started reviewing on behalf of lucasfang July 2, 2026 11:43 View session

lucasfang added 13 commits July 6, 2026 09:40

fix

be07d75

fix

2dc2d47

fix

d9913c3

fix

64b5a7a

Merge branch 'main' into dev

d75d029

fix

aada5b9

fix

b6ea562

fix

4af593b

fix

50ce127

fix

bb34ff0

fix

989a544

fix

456112c

fix

6eb2720

zjw1111 reviewed Jul 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(commit): align functionality with java paimon commit#397

feat(commit): align functionality with java paimon commit#397
lucasfang wants to merge 15 commits into
alibaba:mainfrom
lucasfang:dev

lucasfang commented Jul 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

zjw1111 left a comment

Uh oh!

zjw1111 Jul 10, 2026

Uh oh!

zjw1111 Jul 10, 2026

Uh oh!

zjw1111 Jul 10, 2026

Uh oh!

zjw1111 Jul 10, 2026

Uh oh!

zjw1111 Jul 10, 2026

Uh oh!

zjw1111 Jul 10, 2026

Uh oh!

zjw1111 Jul 10, 2026

Uh oh!

zjw1111 Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		::ArrowSchema arrow_schema;
		ASSERT_TRUE(arrow::ExportSchema(*schema, &arrow_schema).ok());

Uh oh!

Conversation

lucasfang commented Jul 2, 2026

Purpose

Tests

API and Format

Documentation

Generative AI tooling

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

zjw1111 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants