SEZ9 commented on PR #11746: URL: https://github.com/apache/seatunnel/pull/11746#issuecomment-5924372818
Thanks for syncing the branch with dev. I re-checked the new head (`27e0c53013`) against the earlier review scope. One caveat up front: the latest review notes that the merge also pulled in a dev change to the split-metadata queries that the composite path sits next to, so I'm not treating the head as simply "unchanged versus `a6a8eb7c2f`". Could you confirm how the composite boundary queries interact with that change (and whether any test covers it)? **Likely addressed, but I'd like a pointer to the hunk to confirm** - Oracle composite splitting gated on 12c+ at runtime, so Oracle 11g keeps the previous single-column split rather than failing on `FETCH FIRST`. - Documented caveat for STRING key columns where database ORDER BY and Java-side ordering can diverge. I'm fine treating this as a documented limitation rather than a hard guard, as long as the docs make the degradation mode clear. **Still open — please confirm status or push a follow-up** 1. **SQLite NULL PK components** (`SqliteDialect`): SQLite allows NULL inside a composite primary key, and the tuple-range predicates won't match those rows, so they are silently dropped. Either exclude SQLite from the composite path when a PK column is nullable, or add explicit NULL handling in the boundary/range predicates, plus a test with a NULL key component. 2. **Comma-based `COMPOSITE_KEY_SEPARATOR`** (`DynamicChunkSplitter`): identifiers or boundary values containing a comma will be mis-split on decode. A structured representation in the split metadata or an escaped encoding would fix this. 3. **`compareArrays` / `Arrays.equals` on JDBC tuples** (`DynamicChunkSplitter`): SQLite can return Integer for one probe and Long for the next on the same column, and BigDecimal equality is scale-sensitive, so the comparison can throw or the dedup guard can never fire. Normalising numeric types before comparing would address both. 4. **Upgrade note / opt-out** (`docs/en/connectors/source/Jdbc.md`): the behaviour change for existing composite-PK jobs is described, but there is no upgrade note and no documented way to return to the single-column split. A config switch (or a documented fallback) plus a short upgrade section would be enough. 5. **Removed single-split-key guard in `ChunkSplitter.generateSplits`**: `FixedChunkSplitter` now has no protection against a multi-column split key. Please either restore the check on the fixed path or make `findSplitKey` only return a composite key when the splitter is dynamic. If any of these were already handled and I missed it, a pointer to the relevant hunk is all I need. Otherwise a follow-up commit covering items 1–5 (with tests for 1 and 3) should get this to approval. <!-- streview-comment:1453 --> -- 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]
