SEZ9 commented on PR #11746: URL: https://github.com/apache/seatunnel/pull/11746#issuecomment-6332507769
Thanks for pushing this through the rounds. The automated review notes that head `ea4e9cada227` is unchanged since review `5584134111` and that the `Build` run `39367208086` completed successfully for that SHA, which is good to see. Before I merge, I'd like to close the loop on the findings from my earlier review, since the thread doesn't yet record how each one landed on this head: 1. **SQLite NULL composite-PK components (F1)** – please confirm whether `SqliteDialect` now handles rows with NULL key components in the composite split (or whether SQLite falls back to the single-column path), and point me to the test that covers it. 2. **Oracle 11g `FETCH FIRST` (F2)** – is Oracle composite splitting still unconditional? If so, I'd like either a version check with fallback to the old single-column split, or an explicit note that 11g is not supported for composite keys. 3. **STRING key collation (F3/F4)** – are STRING columns still permitted in the composite path? If yes, is there a guard or fallback when the DB ORDER BY and `compareArrays` disagree, so the walk cannot collapse to a single full-table chunk or stop early? 4. **Comma separator in composite key metadata (F5)** – has `COMPOSITE_KEY_SEPARATOR` been replaced with an unambiguous encoding, or is there a guard rejecting identifiers/values containing commas? 5. **Type-sensitive comparison on boundary tuples (F6)** – please confirm `compareArrays`/`Arrays.equals` now normalizes Integer vs Long and `BigDecimal` scale, so the dedup guard is effective and no `ClassCastException` can occur at split time. 6. **Upgrade note (F7)** – the opt-outs (`partition_column`, `enable_concurrent_read = false`) are a good answer; please add a short upgrade note in `docs/en/connectors/source/Jdbc.md` that calls out the behavior change for existing composite-PK jobs and names those opt-outs. 7. **Multi-column key guard in `FixedChunkSplitter` (F8)** – since the "only one split key" check was removed from the shared `ChunkSplitter.generateSplits`, please confirm `FixedChunkSplitter` now has its own guard (or that it can never receive a multi-column key). A quick per-item reply (with the relevant commit or test name) is enough; I'll do a final pass and merge once these are confirmed. A sync with `dev` right before merging would be appreciated but isn't blocking. <!-- streview-comment:1953 --> -- 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]
