luozihen commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5747177276
Hi @DanielLeens — I need your advice on how to handle a CI failure before I make any further changes, because I would rather not widen this PR unless you think that is the right call. What fails. jdbc-connectors-it-part-1 fails on the engine-level E2E I added — JdbcMysqlMultipleTablesIT#testMysqlJdbcMultipleTableE2e, the assertion at line 210. That phase drops sink.table1 / sink.table2 so the sink creates them itself, which is what makes the resolved multi_table_config.primary_keys observable in the created DDL. Why it fails — this looks like a pre-existing gap, not the mapping logic. The key columns configured for those tables (c_int, c_integer, c_mediumint) are nullable in the source metadata, and MysqlCreateTableSqlBuilder#buildColumnIdentifySql emits an explicit NULL for every nullable column (MysqlCreateTableSqlBuilder.java:204-209). The generated DDL therefore ends up as: `c_int` int(11) NULL COMMENT '', `c_integer` int(11) NULL COMMENT '', PRIMARY KEY (`c_int`, `c_integer`) MySQL rejects that: sql/sql_table.cc raises ER_PRIMARY_CANT_HAVE_NULL (1171, "All parts of a PRIMARY KEY must be NOT NULL") when a primary key column is explicitly declared NULL — if the keyword is omitted, MySQL converts the column to NOT NULL implicitly. AbstractJdbcCatalog#createTableInternal then wraps it as Failed creating table sink.table1 (Caused by: java.sql.SQLSyntaxErrorException: All parts of a PRIMARY KEY must be NOT NULL ... in the log) and the job fails. So I do not think this is a defect in the new option: existing auto-create cases never hit it because their key columns are NOT NULL (CDC primary keys, and JdbcMySqlCreateTableIT uses id int NOT NULL), and the Postgres builder omits the nullability keyword entirely, which is why the same scenario does not fail there. What I would like your guidance on. I see two ways to make CI green and I do not want to pick the wrong one: 1. Fix it here: make MysqlCreateTableSqlBuilder emit NOT NULL for columns that are part of the primary key, plus a unit test in MysqlCreateTableSqlBuilderTest. The E2E then stays exactly as it is and doubles as the end-to-end regression. The change only turns "currently fails" into "works" — it does not alter DDL for configurations that already work. 2. Keep this PR scoped to multi_table_config: adjust the E2E so its key columns are NOT NULL (or use dedicated tables for that phase, so the shared fixture stays untouched) and track the MySQL DDL gap separately. The test would then not depend on that pre-existing issue. My own preference is (2) — keeping this PR focused on the new option and handling the builder fix separately — but I am happy to do (1) here if you think that is cleaner, or to change how the E2E observes the mapping if you would rather it did not depend on auto-created tables at all. Could you let me know which you would prefer? And if we go with (2): is there an existing issue for the MySQL DDL gap I should reference, or should I open one? For completeness: unit-test passes on all four lanes for this head (c15f9fdc5), so this is specific to the E2E auto-create path. CI: [run 35303656875 / jdbc-connectors-it-part-1](https://github.com/luozihen/seatunnel/actions/runs/35303656875/job/105471901217). This PR: [#12015](https://github.com/apache/seatunnel/pull/12015). Thanks — apologies for yet another question on this PR; I just want to avoid growing the diff in a direction you would not choose. -- 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]
