SEZ9 commented on PR #12597: URL: https://github.com/apache/seatunnel/pull/12597#issuecomment-6136814515
@CodeWithPravinMaske thanks for the update. Guarding the primary-key branch in `getSplitColumn` with the same `isColumnNullable` check (skip with WARN → try unique keys → single-split fallback) is the direction I was hoping for, and it should bring the code in line with what `incompatible-changes.md` and the MySQL-CDC docs say about `table-names-config.primaryKeys`. I haven't looked at the updated diff yet, so I'll confirm the first two points from my review once the changes are pushed. Your comment also got cut off at `spl` — could you paste the full name of the second new test so I know what it covers? A few items from the earlier review aren't mentioned in your update: 1. **`isColumnNullable` fallback** — when the exact `TABLE_NAME`/`COLUMN_NAME` match in `DatabaseMetaData.getColumns` misses (identifier case, catalog/schema mismatch), it still silently falls back to `Column#isOptional()`, which the Javadoc itself calls unreliable. Please either skip the candidate key with a WARN in that case, or at least log that the metadata lookup missed and the parsed value is being used. 2. **NULL unique-key values arriving as `code = 0`** — the E2E compares only `id`, so the sink-side value for rows with a NULL unique key isn't verified. Please extend the assertion to compare `code` as well (expecting NULL); if it does come through as `0`, that should be fixed in this PR rather than masked by the test. 3. **SPI Javadoc on `isColumnNullable`** — please document when the method is called, that the default issues one `getColumns` round trip per candidate key, and that catalog-backed dialects may override it. 4. **Per-key metadata round trip** — one `getColumns` call per candidate key per table during split generation adds up on Oracle. Caching per table (one `getColumns` call for all columns) or letting dialects that already hold the catalog table short-circuit would address this cheaply. Happy to do a final pass once those are in — or let me know if you think any of them shouldn't be. <!-- streview-comment:1704 --> -- 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]
