JingsongLi commented on PR #10098:
URL: https://github.com/apache/paimon/pull/10098#issuecomment-5953237057
Reviewed exact head `9f001773e373ddf40559c5f58671e4b146788308`. The in-place
conversion has strong end-to-end value, and the latest stale-handle,
sequence-normalization and interrupted-conversion fixes pass their regressions.
Two additional historical/recovery paths still break the converted table.
1. **[P1] Guard the Data Evolution boundary when rolling back a
row-tracking-only table.** Both
`AbstractFileStoreTable.checkRollbackKeepsRowTracking` and the
`FileStoreCommitImpl.rollbackToAsLatest` guard only compare
`row-tracking.enabled`. A snapshot from before DE conversion can already have
row tracking enabled while retaining the incompatible row-count sequences that
this PR now normalizes. In a real 20-row, two-file table, conversion changed
the old files' max sequences from `[1,19]` to `[1,1]`, and all 10 subsequent
column updates were visible. `rollbackTo(oldSnapshot)` and
`rollbackTo("before")` both succeeded under the current DE schema, restored
`[1,19]`, and a successful subsequent column-update commit applied **0/10**
updates. `rollbackToAsLatest` also reproduced 0/10 after a real DE compaction
replaced the original files; the same-file case was a passing negative control
because it kept the normalized current metadata. Refuse the DE-disabled ->
DE-enabled rollback bo
undary as well, or safely normalize the target metadata before allowing it.
Please cover snapshot, tag and as-latest paths on a row-tracking-only source
table.
2. **[P1] Validate restored compaction files, not only the options of the
committer handle.** The new commit guard rejects a committer loaded before
conversion, but a newly loaded DE committer can restore a checkpoint's
compaction messages produced by the previous RT-only writer. I ran a real
`AppendCompactTask`, serialized its pending `ManifestCommittable`, converted
the table, applied a DE column update, then restored through
`filterAndCommitMultiple(..., true)`. The old `COMPACT` output has schema 0 and
no `firstRowId`; restoration committed it and replaced the converted source
files. Read results changed from `(1,new-a,0), (2,b,1)` to `(NULL,new-a,0),
(1,a,NULL), (2,b,NULL)`. Without the column update, both original rows simply
lost their row IDs. Flink's `RestoreCommittableStateManager.recover` ->
`StoreCommitter.filterAndCommit` uses this same path when a job is resubmitted
with the converted schema; `RestoreAndFailCommittableStateManager`
intentionally fails only **after**
the restored commit, so that later failure does not undo the corruption.
Reject incompatible restored compaction output before publication, or convert
it with proven preservation of row identity. Please add a serialized-message
recovery regression using a freshly loaded table/committer. An isolated guard
rejecting the incompatible missing-row-ID COMPACT output preserved both updated
rows in the same probe.
Validation: 139 focused Core tests passed with normal JDK 8 Maven checks;
all 24 selected conversion/row-tracking/sub-field Spark tests passed on each of
Spark 3.5.8 and Spark 4.1.2, including actual SQL conversion,
update/merge/delete, historical reads and incremental BTree index use. The 6
procedure/action/streaming tests also passed on each of Flink 1.20.1 and Flink
2.2.0 after aligning the local reactor test classpath from Avro 1.11.3 to the
Paimon compiler's 1.11.4; the initial mismatch failed before conversion. The
additional actual-table probes above reproduce both P1s on this head.
Exact-head CI is green. Older-version writers still must be stopped as
documented; these findings also occur with this version's supported
rollback/recovery APIs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
