CodeWithPravinMaske commented on PR #12597: URL: https://github.com/apache/seatunnel/pull/12597#issuecomment-6118879878
Thanks @DanielLeens and @SEZ9 for the careful re-review. Updated: **Primary key configured in `table-names-config.primaryKeys` (Daniel #1, SEZ9 #1/#2).** I went with guarding the primary key branch too (Daniel's Option A), so the docs and the code say the same thing. - `getSplitColumn` now runs the same `isColumnNullable` check on the first primary key column. It skips the column with a WARN, then the unique keys are tried, then the single-split fallback is used. Real primary keys are NOT NULL in the database, so only a user-configured nullable key is affected. - This also closes the exactly-once concern Daniel raised on #12608: with this PR, a NULL can never be in the split key. - The behavior change Daniel pointed out (a job with `exactly_once = true` and a nullable configured `primaryKeys` column now fails at startup, even if the column holds no NULLs) is written in `incompatible-changes.md` (en/zh). The exactly-once error now says `not found non-nullable primary key or unique key`. - The MySQL-CDC docs (en/zh) now say that a column declared via `table-names-config.primaryKeys` must be NOT NULL in the database. - New tests: `splitColumnSkipsNullableConfiguredPrimaryKey` and `splitColumnPrefersNotNullUniqueKeyOverNullablePrimaryKey`. Both fail without the guard. **Fallback when the name match misses (Daniel #2, SEZ9 #3).** `isColumnNullable` keeps the exact match first, then accepts a case-insensitive match, and logs a WARN naming the table and column before falling back to the parsed column. An empty `IS_NULLABLE` still counts as nullable, the safe direction. New test: `isColumnNullableMatchesNameIgnoringCaseWithoutExactRow` (it fails without the case-insensitive match). **E2E compares ids only (Daniel #3, SEZ9 #4).** It now compares `id, name`. On the NULL -> `0` value itself: that is the pre-existing bug tracked in #12599 and fixed in #12608 (it also happens on binlog events on `dev`), so it's kept separate from this PR. The test comment links #12599. **Javadoc (Daniel #4, SEZ9 #5).** It now says when the method is called (for each candidate split column during split generation), that the default runs one `DatabaseMetaData#getColumns` query per call, and that a dialect may override it. **Metadata cost (SEZ9 #6).** I kept the lookup in the default method. It is one query per candidate column, only at split generation, next to the existing `getPrimaryKey` / `getUniqueKeys` metadata calls of the same method. As Daniel noted, it doesn't need restructuring in this PR, and a dialect with a cheaper source can override it. `JdbcSourceChunkSplitterTest` (13) and `AbstractJdbcSourceChunkSplitterTest` (3) pass. CI: the `ElasticsearchIT` failures come from `dev` (#12711). -- 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]
