JingsongLi commented on PR #9790: URL: https://github.com/apache/paimon/pull/9790#issuecomment-5956781362
Reviewed `d5a453d831`. Requirement fit: **SUPPORTED**. Implementation: **FINDINGS**. Publishing append-table sort compaction as COMPACT has clear end-to-end value: a partition-filtered real-data probe retained concurrent appends and nested values for Parquet/ORC, with deletion vectors both enabled and disabled. Two failure paths still need changes before production use. **[P1] Spark can delete files still owned by a tag after an already successful COMPACT commit.** `SortCompactSparkCommit.java:52-53` treats a negative `isBatchCompactCommitSucceeded` result as permission to abort both writer output and compact output. The proof helper only scans surviving ordinary snapshots. I reproduced the following with real Parquet files in a supported `bucket=-1` append table and a real `TableCommitImpl` synchronous maintenance callback: sort COMPACT snapshot 2 publishes F2; a tag is created at 2; another actual COMPACT snapshot 3 replaces all F2; expiration removes ordinary snapshot 2 while the tag and F2 remain readable; then maintenance throws after publication. The helper returns false, the Spark abort removes F2, and reading the tag now fails with `FileNotFoundException`. Latest snapshot 3 remains readable, so this is specifically corruption of retained tagged data. An otherwise identical control suppressing abort preserves the tag. Missing historical ev idence must remain an unknown outcome, rather than proving the original commit never published; protect tag/branch references or use a durable, reliable commit witness before deleting output. Please add this tagged, concurrently compacted/expired publication regression. **[P2] The earlier Flink replay fix is incomplete for rewrite failures.** The commit-failure path now correctly retains writer output, as requested in the previous review. However, `SortCompactCommitter.java:126-129` still calls `abortWrittenQuietly` if rewrite fails. One transient read failure on the DV index manifest during `validateNoDeletionVectorDrift` deletes the sorted writer output before any commit. I verified this using the actual `CommitterOperatorFactory` and `OneInputStreamOperatorTestHarness`: batch `Long.MAX_VALUE` committable, first `endInput` fails once; close the whole harness/committer/TableCommit; recreate only the downstream harness and replay the same committable after storage recovers. The second attempt permanently fails with `Cannot recover ... files ... have been deleted`. Removing only this rewrite-catch abort makes the identical operator replay succeed, advancing snapshot 2 to 3 and replacing two input files with one. Existing committed input remains in tact, but the job cannot recover without regenerating upstream output. Preserve replayable writer files on this path and clean only newly generated rewrite artifacts. Cover a transient rewrite/DV-manifest error followed by downstream-only batch replay, in addition to the existing commit-failure regression. Validation: 24 core rewriter tests passed under JDK 8; 60 Flink 2 tests passed (committer, operator, append sort action, and SQL compact procedure). The normal Flink build first failed Spotless on the changed `SortCompactCommitterTest.java`; behavior tests were then run with only `spotless.check.skip=true`, plus local Avro dependency alignment. The same formatting failure is present in the current Flink 1 CI job and still needs correction. All 3 Spark commit-helper tests and 6 actual Spark 4.1.2 sort-compact SQL cases also passed, including partitions, partition filters, row-tracking rejection, and null clustering values; the SQL subset was executed directly with the freshly compiled Maven runtime after the Maven test-name selector unexpectedly expanded the scope. The initial Spark helper test used the incompatible local Avro 1.12.1 and passed after alignment with the core build’s 1.11.4. Actual Flink 1.20.1 operator failure/recovery probes and the Parquet tagged-publication fai lure/control probes above also ran. Ordinary injected snapshot/manifest I/O errors did not trigger the Spark destructive abort; the P1 is the deterministic loss of the surviving ordinary-snapshot witness, not a generic I/O hypothesis. -- 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]
