SEZ9 commented on PR #12608: URL: https://github.com/apache/seatunnel/pull/12608#issuecomment-6008027371
Thanks for the detailed walkthrough, and no worries about the ordering — 226cdde90 is what I'll review against. **F1 / F2 (snapshot-only fix, streaming DDL and table history bypass):** Based on your description, moving the change into the `MySqlAntlrDdlParser` override at `parsePrimaryIndexColumnNames` sounds like the right place, since it should put snapshot DDL, `parseStreamingDdl` and the `TableChange` saved in the table history on one code path. I'll confirm that against the diff. The tests you list (`testNullableUniqueKeyColumnStaysOptionalInStreamingDdl`, `testRealPrimaryKeyColumnIsNotOptionalInStreamingDdl`, the snapshot tests, and the `uk_added_null` E2E with `ALTER TABLE ... ADD UNIQUE KEY` in the binlog phase) look like good coverage for the streaming side. On your question: yes, please add the savepoint/restore E2E. The restore path was the core of F1, and the tests described prove the parser output but not that a schema rebuilt from the checkpointed history keeps the column optional end to end. It can be short — snapshot `uk_added_null`, savepoint, restore, insert a NULL, asse rt the sink holds NULL. Two small asks on the override: (a) a header comment stating it is a copy of Debezium 1.9.8 and naming `parsePrimaryIndexColumnNames` as the only modified method, so a future Debezium bump knows what to re-apply; (b) a brief comment on that method explaining why nullable unique-key columns are no longer promoted to NOT NULL. **F5 / F7 / F8 (unreachable null guard, misleading Javadoc, per-column `log.info`):** Removing `restoreNullableColumns` and bringing `MySqlSchema.java` back to `dev` would close all three; I'll verify in the diff. **F3 / F6 (docs, incompatible change):** The `incompatible-changes.md` entry plus the en/zh MySQL-CDC note should cover both, and calling out that older checkpoints keep the old schema is a good addition. I'll check the wording when I go through the files. **F4 (nullable column in `table-names-config.primaryKeys` still emitted as `0`):** Your explanation makes sense — if the parser keeps declared nullability and `mergeCatalogTableConfig` only overrides key names, the silent `0` disappears. Since that is currently stated in the description rather than asserted in a test, please add one assertion (unit level is fine) that a nullable column configured via `table-names-config.primaryKeys` yields NULL, not the type default, so the doc statement is backed by code. So remaining: the restore E2E, the two comments on the parser override, and the F4 assertion. Once those are in and the diff matches the description, I'm happy to approve. <!-- streview-comment:1542 --> -- 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]
