DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5749193893
Hi @luozihen, thank you for pulling the full log and for laying the evidence
out so clearly. That made this quick to confirm. I re-checked it against the
log of the fork run you linked and against `dev`.
**Confirmed**
- In fork run 35303656875 (head `c15f9fdc5`),
`JdbcMysqlMultipleTablesIT.testMysqlJdbcMultipleTableE2e` fails on all 7
containers (`Tests run: 28, Failures: 7`), always at the
`executeJob("/jdbc_mysql_source_and_sink_with_multi_table_config.conf")` call,
with `SQLSyntaxErrorException: All parts of a PRIMARY KEY must be NOT NULL`
wrapped in `CatalogException ... Failed creating table sink.table1`.
- Your diagnosis holds. `MysqlCreateTableSqlBuilder.java:205-209` on `dev`
emits an explicit `NULL` for every nullable column, and `:166` appends `PRIMARY
KEY (...)` when `create_index` is true, so a key on a nullable column is
rejected by MySQL.
- It is pre-existing and not specific to `multi_table_config`. The existing
top-level `primary_keys` option goes through the same path
(`JdbcSinkFactory.java:146-155` on `dev` builds a `PrimaryKey` over the source
columns without touching their nullability), and
`OceanBaseMysqlCreateTableSqlBuilder.java:205` has the same code. I searched
issues and PRs in this repo and could not find an existing report for it.
**My suggestion: option 2, keep this PR scoped**
1. Do not change `MysqlCreateTableSqlBuilder` here. It is a shared DDL
builder used by every MySQL sink with `create_index = true` (and the OceanBase
MySQL-mode twin has the same code), so changing it deserves its own review and
tests, and it is easier for reviewers to reason about `multi_table_config` when
this diff stays focused on the new option.
2. Make the E2E fixture valid for the scenario. Please avoid altering the
shared `source.table1` / `source.table2` columns, because the other test
methods in `JdbcMysqlMultipleTablesIT` depend on them. Creating two small
dedicated source tables for this phase, whose key columns (for example `c_int`,
`c_integer` and `c_mediumint`) are declared `NOT NULL`, and pointing the
patterns in the new `.conf` at those names, keeps your assertions on the
resolved primary keys and row counts exactly as they are.
3. Please open a separate issue for the MySQL / OceanBase DDL gap (emit `NOT
NULL` for primary-key columns, with a unit test in
`MysqlCreateTableSqlBuilderTest`). Happy to help review it, and a follow-up PR
from you would be very welcome.
**Where this leaves the PR**
- This red job is caused by the PR's own new E2E, so it is not an unrelated
flake and a rerun will not clear it. It needs the fixture change above (or
option 1) before the checks can go green.
- My earlier approval was about the code-level review of the change; it does
not cover this failing E2E, so please treat the jdbc-connectors-it-part-1 job
as a merge gate that still needs to pass on the new head.
- Once you push the fixture change, please share the new check link here if
anything else fails and I will help narrow it down.
Thanks again for the careful diagnosis, this is a nice piece of work.
--
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]
