SEZ9 commented on PR #11746: URL: https://github.com/apache/seatunnel/pull/11746#issuecomment-5594662572
Thanks @DanielLeens — appreciated, and the CI explanation makes sense. Understood that the apache-side `Build` check showing `cancelled`/failed is the fork-run-pointer artifact, and that on the fork run `33202704220` (head `0241617929f`) `updated-modules-integration-test-part-2 (8, ubuntu-latest)` / `(11, ubuntu-latest)`, `unit-test`, and `Code style` are all green, with `doris-connector-it (11)` red only due to the pre-existing `DorisErrorIT.testDoris:78` failure in `connector-doris-e2e`. I'm fine treating that Doris failure as unrelated to this diff. Before I pull the trigger, I want to close the loop on the findings from my earlier review rather than assume they landed in `0241617929f`. Could you (or the author) confirm the status of each, with a pointer to where it was addressed or a short note on why it's deferred: 1. **SQLite composite-key split dropping rows with NULL PK components** (`SqliteDialect.java`) — is NULL handling in the composite boundaries covered now? 2. **Oracle composite splitting requiring 12c+ `FETCH FIRST`** (`OracleDialect.java`) — is there a fallback (or version gate) so 11g composite-PK jobs don't fail with a syntax error on upgrade? 3. / 4. **STRING key columns: DB `ORDER BY` collation vs Java `compareArrays` ordering** (`DynamicChunkSplitter.java`) — I see `JdbcMysqlSplitIT#testCompositeKeyWithStringColumn` is green, which is encouraging; does that test exercise a collation-divergent case, or is there a guard/normalization for string keys now? 5. **Comma-based `COMPOSITE_KEY_SEPARATOR` encoding** — is the metadata encoding robust to identifiers/values containing commas, or is this consciously accepted? 6. **`compareArrays`/`Arrays.equals` type fragility** (SQLite `Integer` vs `Long`, scale-sensitive `BigDecimal.equals`) — has the comparison been normalized? 7. **Upgrade note / opt-out for existing composite-PK jobs** (`docs/en/connectors/source/Jdbc.md`) — is there a documented way to keep the old single-column split behavior? 8. **Removed "only one split key" guard in the shared `ChunkSplitter.generateSplits`** — does `FixedChunkSplitter` now defend against a multi-column split key on its own? If these are all resolved, a quick per-item confirmation is enough and I'll proceed with the merge. If any are intentionally deferred, I'd like that stated on the thread so we can track it as a follow-up. Thanks again for the diligence here. <!-- streview-comment:899 --> -- 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]
