SEZ9 commented on PR #12619: URL: https://github.com/apache/seatunnel/pull/12619#issuecomment-5986920036
Thanks @goutamadwant for the detailed follow-up in 4b9e8ee. Going through the eight points based on your description; I'll confirm each against the diff when I re-review: 1. **Byte guard (F1)** — Running the check after the name and after each kept path is the fix I was after, and `rejectsOneLargeVertexBeforeRetainingAllPaths` failing when a path is read past the limit is the right assertion. 2. **Job ID (F2)** — Accepting any non-null `long`, with `acceptsAnyNonNullJobId` covering 0 and negatives, addresses this. 3. **All-null `TablePath` (F3)** — Thanks for the pointer to `TablePath.java#L38-L45` at 309b15effe99423f73b6007cbe61b97bb6af7b24. If the constructor rejects a null/empty table name, the `""` case isn't reachable and I'm happy to drop this; I'll double-check the referenced code. 4. **Transform coverage (F4)** — I follow the `ActionUtils.java#L24-L33` reasoning for the `TRANSFORM` typing. My remaining concern is that the Phase 3 E2E isn't part of this PR, so within this change the `TRANSFORM`/`NOT_APPLICABLE` branch and the logical `JobDAGInfo` shape still aren't exercised against real `DAGUtils` output. Could you add one small unit test here that feeds a config with a transform through `DAGUtils` and asserts the projected node type and the logical flavour? Just enough that a regression in the type mapping is caught in this module. 5. **`PARTIAL` (F5)** — Good addition. Could you confirm (or point me at the test) that `REPORTED` is now only emitted when every path entry is present, so it can no longer co-occur with a metadata warning? 6. **HTTP mapping (F6)** — Documenting that the Phase 3 servlet maps the reasons to 409/413 sounds reasonable for Phase 1, since no servlet calls `project()` yet. 7. **`DEFAULT` thresholds (F7)** — Javadoc citing STIP section 4 is sufficient here. 8. **`requireNonNull(limits)` (F8)** — Thanks for adding this with a test. So the open asks are the transform/logical-flavour unit test in (4) and the confirmation in (5). I'll do a pass over the diff for the rest and follow up if anything doesn't match the above. <!-- streview-comment:1515 --> -- 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]
