SEZ9 commented on PR #11746: URL: https://github.com/apache/seatunnel/pull/11746#issuecomment-5612110868
Thanks @li3zhi4 for completing the matrix — MySQL `JdbcMysqlSplitIT` 7/7 and Oracle `JdbcOracleSplitIT` 3/3 green on `c8c4c3069`, plus the other three dialects, `CompositeKeyChunkSplitterTest` 13/13 and spotless clean, is exactly what I was waiting on for the dialect coverage. Good to know the earlier MySQL error was only a local image-pull timeout and not a regression. On the STRING column test passing on MySQL: that's a useful data point, but it doesn't by itself close the collation concern I raised on `DynamicChunkSplitter` (DB `ORDER BY` vs Java `compareArrays` ordering divergence). Could you confirm whether the MySQL STRING case exercises a collation where the DB ordering and Java ordering actually differ (e.g. case-insensitive collation with mixed-case keys), or whether it only covers the case where they happen to agree? If the latter, I'd still like either a guard or a test that shows the walk doesn't collapse/terminate early when they disagree. On the SQLite NULL-PK-component finding: I agree with @DanielLeens that a green `testCompositeKeyWithNullPrimaryKeyComponent` on this head doesn't clear it yet, given the fixture only uses 3 distinct `order_id` values. The remaining asks there are the ones he spelled out — add the `buildNotNullKeyCondition(columnNames)` guard to the `isLastSplit` and middle-split branches of `buildCompositeCondition`, tighten the unit test to assert the actual guard clause, and give the SQLite E2E fixture enough distinct leading-column values so a NULL row lands strictly inside a middle split. Still open from my previous review, no new comments yet: - Oracle 11g: composite splitting is enabled unconditionally while the boundary SQL needs 12c+ `FETCH FIRST`. Does the oracle-free image used by `JdbcOracleSplitIT` cover 11g, or is a version check / fallback to the single-column split still needed? - Upgrade note in `docs/en/connectors/source/Jdbc.md` and a documented way to opt back into the old single-column split behavior. - The removed "only one split key" guard in the shared `ChunkSplitter.generateSplits` — `FixedChunkSplitter` still needs its own defense against a multi-column split key. - The comma-based `COMPOSITE_KEY_SEPARATOR` encoding and the `compareArrays`/`Arrays.equals` type-sensitivity (Integer vs Long on SQLite, `BigDecimal` scale) — a short note on how you want to handle these is enough to move forward. Once those land I'll do a scoped re-check rather than another full round. <!-- streview-comment:945 --> -- 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]
