SEZ9 commented on PR #12597: URL: https://github.com/apache/seatunnel/pull/12597#issuecomment-6159357184
Thanks for the detailed rundown, @CodeWithPravinMaske. I'd like to confirm each point against the code in `4a0cc3d2e` rather than infer it from the description, so a few pointers would help: - **F3 / F5 (`isColumnNullable` fallback + Javadoc)**: could you point me to the case-insensitive retry and the WARN log that names the table and column before the fallback, and to the updated Javadoc describing when the method is called, the one `getColumns` query per call, and that dialects may override it? Once I can see those in the diff I can close both. - **F6 (per-key metadata round trip)**: keeping the per-table lookup is fine with me as long as the Javadoc note about catalog-backed dialects overriding it is in place; please point me to it. - **F4 (E2E assertion)**: comparing `id, name` is reasonable while the NULL-arriving-as-`0` issue is being handled separately. Please add a short comment next to the E2E assertion explaining why `code` is excluded and referencing the follow-up, so it isn't read as hiding a regression, and point me to the changed assertion. - **F1 / F2 (primary-key branch guard + docs)**: does the primary-key branch of `getSplitColumn` now run the nullability check too, so a nullable column injected via `table-names-config.primaryKeys` is skipped (or rejected with a clear error) instead of becoming the split column? If yes, please point me to that guard and the `splitColumnPrefersNotNullUniqueKeyOverNullablePrimaryKey` test, and the incompatible-changes entry and migration guide are accurate. If the PK branch is still unguarded, either add the guard or narrow the docs to what is actually fixed. Once I can verify these in the diff and the E2E comment is in, I'm happy to approve. <!-- streview-comment:1728 --> -- 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]
