SEZ9 commented on PR #11550: URL: https://github.com/apache/seatunnel/pull/11550#issuecomment-5225134528
Thanks @DanielLeens for the recheck, and glad we're aligned with @SEZ9 on the core pruning logic — I agree the restore-path filtering is correctly scoped and is a no-op for jobs whose table set hasn't changed. To the author, the concrete remaining asks from my side before this can be approved: 1. **Sync with latest `dev` and rerun CI.** The branch is 26 commits behind, so the current `Build` failure on `133ca2cc7298` isn't an actionable signal yet. If the same lanes stay red after the rebase, please post the fresh failing job links and we'll help narrow it down. 2. **Remove the unrelated `docs/en/transforms/llm.md` change** from this PR — it should go in its own PR so this fix stays focused and easy to backport. 3. **Clarify `checkpointTables` handling.** The other three collections (`tableIds`, `completedSnapshotSplitInfos`, `historyTableChanges`) are pruned but `checkpointTables` is passed through as-is. If that's intentional, a short comment in `pruneRemovedTables()` explaining why would help future readers; if not, it should be filtered consistently. 4. **Note the enumerator-side gap.** Pruning currently happens only in the reader while the restored `HybridPendingSplitsState`/`IncrementalPhaseState` is untouched. I'm fine with tracking that as a follow-up issue rather than expanding this PR, but let's make that decision explicit rather than implicit. Once the branch is rebased, CI is green, and points 2–4 are addressed (or explicitly deferred with a rationale), this looks mergeable to me. Thanks again for the fix and the unit test coverage on the pruning path. <!-- streview-comment:85 --> -- 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]
