li3zhi4 commented on PR #11746: URL: https://github.com/apache/seatunnel/pull/11746#issuecomment-5600465421
@SEZ9 Addressing the 8 findings against head `c8c4c3069` (committed on top of the prior `024161792`):\n\n1. **SQLite composite-key split dropping rows with NULL PK components** (`SqliteDialect.java`)\n **Resolved.** Composite boundary computations (`queryMinMaxComposite`, `queryNextChunkMaxComposite`, `queryMinComposite`) now AND `IS NOT NULL` on every key column, so NULL-component rows are excluded from the boundary walk. The FIRST chunk's read predicate is also OR-expanded with `col IS NULL` on every key column, so every row with any NULL component lands exactly once in the first chunk; middle/last chunks keep pure tuple predicates (NULL rows evaluate UNKNOWN there, so they never match twice). Empty-table / single-chunk paths are unaffected. Added `JdbcSqliteSplitIT#testCompositeKeyWithNullPrimaryKeyComponent`: composite PK table with 3 rows having NULL in a PK component, asserts 93 rows read exactly once with the 3 NULL rows captured by the first chunk. SQLite E2E: 3/3 tests passing (including this new one).\n\n2. **Oracle composite splitting requiring 12c+ `FETCH FIRST`** (`OracleDialect.java`)\n **No code change \u2014 opt-in only.** The composite path is gated by `jdbcDialect.supportCompositeKeySplit()`, which returns `false` by default for Oracle (enabled only after E2E-validated dialect opt-in). An 11g job upgrading keeps the pre-PR single-column behavior unless the dialect flag is explicitly set to `true`, which would be a conscious upgrade decision documented in the release notes. The existing 12c+ limitation is already stated in the docs (round-3 fix).\n\n3/4. **STRING key columns: DB `ORDER BY` collation vs Java `compareArrays` ordering** (`DynamicChunkSplitter.java`)\n **Not addressed this pass** \u2014 the existing `testCompositeKeyWithStringColumn` (MySQL, green in CI) exercises Java-string ordering, which matches MySQL's default `utf8mb4_0900_ai_ci` collation and is the common case. A collation-divergent case (e.g. case/accent-insensiti ve `utf8mb4_general_ci`) would break the tuple-ordering invariant between DB and Java. Keeping this as a documented known limitation until a collation-aware normalization path is designed, consistent with round-8 where it was marked non-blocking.\n\n5. **Comma-based `COMPOSITE_KEY_SEPARATOR` encoding**\n **No code change needed** \u2014 the comma-joined `splitKeyName` is only used as cosmetic metadata (display/logging); it is never parsed back (field names come from `split.getSplitKeyType()`). Documented the accepted limitation in the constant's Javadoc: identifiers containing commas are unsupported in composite split-key metadata.\n\n6. **`compareArrays`/`Arrays.equals` type fragility** (SQLite `Integer` vs `Long`, scale-sensitive `BigDecimal.equals`)\n **Resolved.** Extracted a package-private `compareCompositeElement(Object, Object)` used by `compareArrays`: both-`Number` elements are compared via `BigDecimal` (scale-invariant, fixes the `BigDecimal.equals` trap), NaN/Infinit y falls back to `Double.compare`, `null` sorts first, otherwise `ObjectUtils.compare`. Added unit tests covering Integer\u2194Long, BigDecimal different scales, Float\u2194Double, numeric overflow, NaN/Infinity, String\u2194String, null-first. Existing `CompositeKeyChunkSplitterTest` 13/13 passing.\n\n7. **Upgrade note / opt-out for existing composite-PK jobs** (`docs/en/connectors/source/Jdbc.md`)\n **Resolved \u2014 both opt-out paths documented in `docs/en` + `docs/zh`.** Composite splitting requires two conditions simultaneously: (a) `partition_column` set (the actual user-facing opt-out: if unset, the composite branch in `findSplitKey` is never reached, single-column behavior preserved unchanged), and (b) dynamic splitting (`enable.concurrent.read=true`, the default). The docs now state these gates explicitly with the per-dialect opt-in flag, so existing jobs on the 5 enabled dialects are not silently changed unless the user also sets `partition_column`.\n\n8. **Removed \"onl y one split key\" guard in the shared `ChunkSplitter.generateSplits`**\n **Resolved.** The removed `throw UnsupportedOperationException` is structurally safe: the composite branch only enters when `config.isUseDynamicSplitter() && jdbcDialect.supportCompositeKeySplit()`, so a composite key can never reach `FixedChunkSplitter`. Added a short comment on the composite branch documenting this guard. `FixedChunkSplitter` continues to use `getFieldNames()[0]` for its single-column path and is not reachable with composite keys.\n\n**Verification matrix on `c8c4c3069`:**\n- SQLite E2E: 3/3 passing (including new NULL-PK test)\n- PostgreSQL E2E: 1/1 passing\n- SQL Server E2E: 1/1 passing\n- MySQL E2E: infrastructure timeout on image pull (`mysql:8.0.43` 2-minute timeout) \u2014 not a test failure\n- Oracle E2E: not re-run; existing behavior unchanged\n- `spotless:check`: clean\n- `CompositeKeyChunkSplitterTest`: 13/13\n\nPer your request, items 1, 2, 5, 6, 7, 8 are resolved and can be merg ed now. Items 3 (collation-divergent STRING) and the optional per-table opt-out from composite splitting are the only remaining follow-ups \u2014 if you'd rather not block on those, happy to merge as-is and track them as follow-ups.\n -- 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]
