SEZ9 commented on PR #10990: URL: https://github.com/apache/seatunnel/pull/10990#issuecomment-5140456717
Thanks @DanielLeens for the thorough recheck and for confirming the `Build` gate is now green on `74ba70f22665`. Since the head is unchanged, my earlier findings still stand as the remaining asks before I can approve: 1. **`SourceUri` credential leakage** — the docs contract should explicitly require producers to strip userinfo/credentials from URIs before writing `SourceUri`, or at minimum call out the risk. A one-paragraph addition to both EN/ZH `metadata.md` would be sufficient. 2. **Widened `isMetadataField` semantics** — generic key names like `Deleted` now return `true` engine-wide via `MetadataUtil.isMetadataField`. Please either namespace the keys, scope the check, or add an explicit statement (code comment + docs) acknowledging this widening is intentional so downstream callers are not surprised. 3. **Hard-coded nullability in `toMetadataColumn()`** — some of these fields (e.g. `DocumentId`) are arguably non-nullable by contract; either derive nullability per field or document why everything is nullable. 4. **Linear `values()` scan** — minor, but a static name-keyed map would avoid the repeated scan on hot paths. Items 1–3 can be resolved either with a code update or a narrower, explicit contract statement in the docs — I'm fine with either, but one of them needs to land on this PR. Two housekeeping notes: - The branch is `behind_by=192` against `dev`; a rebase alongside the fixes above would be appreciated so we're validating against current `dev`. - Once the update lands, I'll do the re-review and refresh/dismiss the stale `changes requested` state — no separate action needed from your side, Daniel. The overall direction (contract-only, well-tested mappings, clear docs on what the transform does *not* generate) remains good. Looking forward to the next revision. <!-- streview-comment:13 --> -- 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]
