DanielLeens commented on PR #12079: URL: https://github.com/apache/seatunnel/pull/12079#issuecomment-5852831590
Thanks for holding the line here, @SEZ9 -- to be clear: my previous comment (2026-09-26T12:28:30Z) is not actually cut off in the stored comment body. I re-fetched it via the API just now to confirm, and the F2/F4, F3, and F6 answers are present in full right after the F1 test-name list, followed by the F5/F7/F8 recap and the CI note. If GitHub's web UI folded it behind a "Show more" toggle because of its length, that's a rendering artifact, not something missing from what was posted. To remove any ambiguity, here they are again, restated directly rather than by reference back to that comment: **F1 (duplicate output IDs)** -- tests at the current head (`3df75448c`): `ConfigParserUtilTest.java:356` (`testDuplicateOutputsReleaseEachMissingInputOnlyOnce`), `MultipleTableJobConfigParserTest.java:773` (`testDuplicateUnnamedOutputsWaitForOtherJoinInput` -- this class lives in `seatunnel-engine-client`, not `-core`), and the dry-run mirror `SeaTunnelConfValidateCommandTest.java:291` (`testConnectDryRunDuplicateUnnamedOutputsWaitForOtherInput`). All three build a scenario with two transforms defaulting to the same output ID feeding a third, multi-input dependent, and assert the dependent is only scheduled once its other, genuinely distinct input has actually arrived. **F2 / F4 (multi-transform `findLast` fallback for an omitted `plugin_input`)** -- `TransformDependencyScheduler.java:140`: `emptyInputFallback = transform.inputIds.isEmpty() || transform.inputOmitted`, with `inputOmitted` (`:236`) set from `!readonlyConfig.getOptional(PLUGIN_INPUT).isPresent()`. That condition applies regardless of how many transforms are in the job -- an omitted `plugin_input` on the last transform of a multi-transform job still reaches the legacy last-inserted-table fallback; it is not limited to single-transform jobs. Tests: `MultipleTableJobConfigParserTest.java:728` (`testTerminalOmittedInputUsesNamedTransformSchemaAndEdge`, asserts the real produced edge and that the downstream schema flowed from the correct upstream transform) and `:733` (`testOmittedInputPrefersExistingDefaultOverLastNamedOutput`, the case where a live `DEFAULT_ID` entry exists and is correctly preferred over the positionally-last output). Dry-run mirrors: `SeaTunnelConfValidateCommandTes t.java:245` and `:273`. **F3 (scheduling vs. validation ID source)** -- one path, not two. `ConfigParserUtil.getInputIds` (`ConfigParserUtil.java:269`) is the only place `plugin_input` is resolved from `ReadonlyConfig`; `ScheduledTransform`'s constructor calls it directly (`TransformDependencyScheduler.java:235`). After the F8 change, `DryRunConnectValidator.validateTransform` no longer calls any ID-resolution helper itself -- it reads the already-resolved `scheduledTransform.getInputIds()`/`getOutputId()`. Scheduling and validation cannot diverge because there is only one computation left, not two call sites of the same helper. **F6 (single-transform self-cycle)** -- `TransformDependencyScheduler.java:292`: the cycle-detection edge builder only skips an input-equals-output pair when `transform.inputOmitted` is true (the implicit legacy-chaining case). An explicit self-reference (`inputOmitted=false`) keeps a real self-edge with in-degree 1, which Kahn's algorithm can never drain, so it falls through to the `"Transform dependency cycle detected: x -> x"` exception at `:337-340` -- on both the runtime and dry-run paths, since `checkGraph`'s simple-graph branch always routes through `scheduleTransforms` first. Tests: `ConfigParserUtilTest.java:221` (`testSimpleGraphRejectsExplicitSelfCycle`), `:387` (`testSingleExplicitSelfCycleCannotUseLegacyFallback`), `:240` (`testImplicitSingleTransformIsNotAnExplicitSelfCycle`, the negative case proving implicit chaining is untouched), the dry-run mirror `SeaTunnelConfValidateCommandTest.java:260` (`testConnectDryRunRejectsSingleExplicitSelfCycle`), and the client-to-m aster `JobExecutionIT.java:90` (`testRejectsExplicitTransformSelfCycleBeforeSubmission`), bounded by `assertTimeoutPreemptively` at `:101` so a regression back to the old hang would fail the test instead of hanging CI. On F5/F8 -- no new evidence needed beyond what I already posted; you noted the shape sounds right and want to confirm against the files directly, which is the correct instinct, so I will not restate it here. On F7 -- the `incompatible-changes.md` entries genuinely did not change between `d8045271fcf6` and `3df75448c` (confirmed via the file-scoped diff on that range in my last comment), so there is nothing new in this specific commit range to check. The entries themselves were added in the `d796232d2954` round and I re-verified their content against the code as recently as my `d8045271fcf6` review. Nothing here changes my `APPROVED` conclusion from `3df75448c` -- this is the same evidence already in the thread, just pulled back out in case the length of my last comment made it hard to follow inline. Let me know if any specific line still does not check out once you have looked at the diff yourself. -- 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]
