SEZ9 commented on PR #12608: URL: https://github.com/apache/seatunnel/pull/12608#issuecomment-5975792912
Thanks @CodeWithPravinMaske for going after the root cause instead of patching the single call site in `MySqlSchema#parseSnapshotDdl`. Overriding `MySqlAntlrDdlParser#parsePrimaryIndexColumnNames` so that only a real `PRIMARY KEY` forces NOT NULL, while a promoted unique key keeps its declared nullability, sounds like the right direction for the binlog DDL and checkpointed-history concerns, since every DDL parse goes through that parser. A few things before I can re-review: 1. I can't see the reworked code yet. Once it's pushed I'll verify the parser override against the snapshot, `parseStreamingDdl` and restore-from-checkpoint paths. Tests covering the non-snapshot paths (`CREATE TABLE` with a nullable unique key, `ALTER TABLE ... ADD UNIQUE KEY` and `CREATE UNIQUE INDEX` applied via binlog DDL, and ideally a restore case) that assert the column stays nullable and NULL is emitted rather than the type default would make that much easier. 2. With the fix moved into the parser, is `restoreNullableColumns` in `MySqlSchema` (including the `catalogTable == null` guard, the "may be null" Javadoc and the per-column `log.info`) removed? If so, those earlier remarks are moot; if any of it remains, please drop the unreachable guard, fix the Javadoc and lower the per-column logging to `debug` or a single `info` per table. 3. Docs: please add a note to the MySQL-CDC docs and a release-note / incompatible-changes entry, since sinks that previously received `0` will now get SQL NULL. Could you also confirm how a nullable column configured in `table-names-config.primaryKeys` behaves with the parser-level fix, and document the outcome either way? Thanks again, happy to take another pass once the code is up. <!-- streview-comment:1497 --> -- 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]
