CodeWithPravinMaske commented on PR #12608: URL: https://github.com/apache/seatunnel/pull/12608#issuecomment-6051819408
Thanks @SEZ9. To make the review easier, here is where each point lives at the head commit `fa70f3eb8`. The key point is that the PR no longer changes `MySqlSchema`: the fix moved from post-processing in `MySqlSchema` into the DDL parser itself (commit 226cdde90), so every path that parses DDL gets it. **The fix** - [`MySqlAntlrDdlParser#parsePrimaryIndexColumnNames`](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/main/java/io/debezium/connector/mysql/antlr/MySqlAntlrDdlParser.java#L424-L448): `realPrimaryKey` is true only for `PRIMARY KEY (...)` and `ALTER TABLE ... ADD PRIMARY KEY`, and only then are the columns forced NOT NULL ([L448](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/main/java/io/debezium/connector/mysql/antlr/MySqlAntlrDdlParser.java#L448)). When Debezium promotes a unique key (`UNIQUE KEY`, `ALTER TABLE ... ADD UNIQUE KEY`, `CREATE UNIQUE INDEX`), the columns keep their declared nullability. - This class overrides Debezium's class of the same name (same mechanism as the existing `MySqlStreamingChangeEventSource` override), so it is used by every `MySqlDatabaseSchema`, for snapshot and streaming DDL alike. **How each path reaches it** - **Snapshot**: [`MySqlSchema#parseSnapshotDdl` → `databaseSchema.parseSnapshotDdl`](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/utils/MySqlSchema.java#L160) → the parser above. - **Binlog DDL**: [`MySqlStreamingChangeEventSource#handleQueryEvent` → `parseStreamingDdl`](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/main/java/io/debezium/connector/mysql/MySqlStreamingChangeEventSource.java#L666) → the same parser. - **Checkpointed table history**: [`JdbcSourceFetchTaskContext#registerDatabaseHistory` deserializes](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/source/reader/external/JdbcSourceFetchTaskContext.java#L192) the `TableChange` that was built by the parser in one of the two paths above and saved in the checkpoint, so it already carries the declared nullability. There is nothing to restore after deserializing. - **`table-names-config.primaryKeys`**: [`MySqlSchema` L165](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/utils/MySqlSchema.java#L165) calls [`CatalogTableUtils#mergeCatalogTableConfig(Table, CatalogTable)`](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/utils/CatalogTableUtils.java#L120), which only overrides the primary key *names*; it never changes column nullability. The nullability comes from the parser, so a nullable configured key column stays optional and NULL is emitted as NULL. **Tests for those paths** - Streaming DDL: [`testNullableUniqueKeyColumnStaysOptionalInStreamingDdl`](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/test/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/utils/MySqlSchemaTest.java#L217) calls `MySqlDatabaseSchema#parseStreamingDdl` with `ALTER TABLE ... ADD UNIQUE KEY` and `CREATE UNIQUE INDEX`; [`testRealPrimaryKeyColumnIsNotOptionalInStreamingDdl`](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/test/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/utils/MySqlSchemaTest.java#L237) checks that a real primary key stays NOT NULL. - `primaryKeys`: [`testNullableColumnConfiguredAsPrimaryKeyEmitsNull`](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/test/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/utils/MySqlSchemaTest.java#L191) goes through `MySqlSchema` and checks that a NULL value converts to NULL. - E2E with a unique-key DDL in the streaming phase: both [`testMysqlCdcNullInNullableUniqueKeyWithoutPrimaryKey`](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-e2e/seatunnel-connector-v2-e2e/connector-cdc-mysql-e2e/src/test/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/AbstractMysqlCDCITBase.java#L391) and [`testMysqlCdcNullInNullableUniqueKeyAfterRestore`](https://github.com/apache/seatunnel/blob/fa70f3eb83f4252074dccd4137f1ac6bf22fa2d8/seatunnel-e2e/seatunnel-connector-v2-e2e/connector-cdc-mysql-e2e/src/test/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/AbstractMysqlCDCITBase.java#L421) run `ALTER TABLE uk_added_null ADD UNIQUE KEY uk_code (code)` during the binlog phase, then insert NULL; the second one then savepoints and restores, so the restored schema comes from the checkpointed history. **Smaller items**: `restoreNullableColumns` no longer exists, and with it the `catalogTable == null` guard, the "may be null" Javadoc and the per-column `log.info`. `MySqlSchema.java` is identical to `dev` again, which is why it does not appear under "Files changed". -- 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]
