Skip to content

[core] Refactor row ID reassignment manifest processing - #8887

Merged
JingsongLi merged 4 commits into
apache:masterfrom
JingsongLi:codex/refactor-row-id-reassignment
Jul 28, 2026
Merged

[core] Refactor row ID reassignment manifest processing#8887
JingsongLi merged 4 commits into
apache:masterfrom
JingsongLi:codex/refactor-row-id-reassignment

Conversation

@JingsongLi

@JingsongLi JingsongLi commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What changed

  • Extract row-ID reassignment planning from DataEvolutionRowIdReassigner into a focused planner.
  • Add reusable projected BinaryManifestEntry and BinaryDataFileMeta views.
  • Bind projected fields by schema field ID instead of relying on a planner-specific fixed layout.
  • Avoid validating or projecting manifest versions in the lightweight binary view.
  • Keep the manifest scan hot path on binary strings and serialized rows without materializing POJOs.
  • Add dedicated unit tests for both binary views, including arbitrary projections, unsupported fields, null handling, reuse, and lifecycle checks.

Why

The row-ID reassignment implementation introduced in #8882 combines orchestration, planning state, binary manifest decoding, and compact data structures in one large class. This makes the algorithm difficult to review and gives its inline binary decoding logic a schema that is specific to one caller.

The extracted binary views make projection handling reusable and keep the planner focused on row-ID assignment.

Impact

This is an internal refactor. Row-ID assignment behavior and manifest serialization remain unchanged. Accessors for projected fields implement the existing ManifestEntry and DataFileMeta contracts; access to fields that were not projected fails explicitly.

Validation

mvn -pl paimon-core -DfailIfNoTests=false -DwildcardSuites=none \
  -Dtest=BinaryDataFileMetaTest,BinaryManifestEntryTest,DataEvolutionRowIdReassignerTest test

56 tests passed. Checkstyle, Spotless, and Enforcer passed.

@JingsongLi
JingsongLi marked this pull request as ready for review July 28, 2026 08:13

@leaves12138 leaves12138 left a comment

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.

Found one compilation issue in the latest update.

assertThat(actual.externalPathDir()).isEqualTo(expected.externalPathDir());
assertThat(actual.hasFirstRowId()).isTrue();
assertThat(actual.firstRowId()).isEqualTo(10L);
assertThat(actual.firstRowIdValue()).isEqualTo(10L);

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.

firstRowIdValue() was removed from BinaryDataFileMeta in the latest commit, but this assertion still calls it, so paimon-core fails during test compilation. Please switch this to actual.nonNullFirstRowId() (or restore the helper).

@leaves12138 leaves12138 left a comment

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.

Rechecked the latest changes. The compilation issue is fixed, the manifest field projections remain consistent, and the focused core tests pass.

@JingsongLi
JingsongLi merged commit 6fa106e into apache:master Jul 28, 2026
12 checks passed
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