SEZ9 commented on PR #12608: URL: https://github.com/apache/seatunnel/pull/12608#issuecomment-6206488811
@CodeWithPravinMaske thanks for the detailed pointers, that helps a lot with the re-review. A few things before I can close out the open points: - **F1 / F2 (binlog DDL and checkpointed history):** moving the fix into `parsePrimaryIndexColumnNames` in 226cdde90 sounds like the right direction, and the `testNullableUniqueKeyColumnStaysOptionalInStreamingDdl` / `testRealPrimaryKeyColumnIsNotOptionalInStreamingDdl` pair plus the `testMysqlCdcNullInNullableUniqueKeyWithoutPrimaryKey` / `testMysqlCdcNullInNullableUniqueKeyAfterRestore` E2E runs are the coverage I was hoping for. I will verify this against the diff once the updated head is pushed, since the PR head is still `40f5c1e6b991` and the agreed Flink-variant trim does not seem to be on GitHub yet. - **F5 / F7 / F8 (null guard, Javadoc, per-column `log.info`):** if `MySqlSchema.java` is indeed back to `dev`, these go away by removal. I will confirm from the diff on the new head. - **F3 / F6 (docs and incompatible-changes):** good that the en/zh `incompatible-changes.md` entries are in place. Could you also confirm whether the MySQL-CDC doc mentions that NULL now flows through for promoted unique-key columns, while a nullable column listed in `table-names-config.primaryKeys` still coerces NULL to the type default? - **F4 (`primaryKeys` on a nullable unique-key column):** could you briefly describe what the `primaryKeys` test in fa70f3eb8 asserts? With that and the doc note above, I am fine keeping that path as-is. Once the new head shows up I will re-read the full diff and follow up. <!-- streview-comment:1782 --> -- 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]
