li3zhi4 commented on PR #11746: URL: https://github.com/apache/seatunnel/pull/11746#issuecomment-6058729876
Thanks for the careful final pass. I checked each point against the current diff and pushed one follow-up commit: `eca4a3c4e`.\n\n**Point-by-point status:**\n\n- **Points 4 and 5:** confirmed as you summarized. The upgrade/opt-out note is in `docs/en` and `docs/zh` `Jdbc.md`; `findSplitKey` keeps the dynamic-only composite gate and `FixedChunkSplitter` rejects a multi-column key. The 20-test `CompositeKeyChunkSplitterTest` suite includes both fixed-path tests.\n- **Point 3 (`compareCompositeElement`):** the SQLite Integer-vs-Long case is handled by the `Number` branch: `compareNumeric()` converts finite values through `new BigDecimal(number.toString()).compareTo(...)`, so it compares numeric value rather than runtime type or `BigDecimal.equals()` scale. The explicit tests are `CompositeKeyChunkSplitterTest#testCompareCompositeElementMixedNumericTypes` (including `Integer`/`Long` and `1`/`1.0d`) and its `new BigDecimal(\"1.0\")` vs `new BigDecimal(\"1.00\")` assertion at the same t est; `testCompareNumericNonFiniteFloatingPoint` covers the NaN/Infinity fallback.\n- **Point 2 (`COMPOSITE_KEY_SEPARATOR`):** there is deliberately **no runtime check** for commas. The separator is only used to populate the display/metadata `splitKeyName`; the split key is carried structurally as `SeaTunnelRowType` and is never decoded by splitting that string. The Javadoc explicitly calls comma-containing identifiers an unsupported/documented limitation. I agree this is preferable to validating boundary values, since values are never encoded in that field.\n- **Point 1 (Oracle pre-12c):** the runtime gate is present and unchanged at `OracleDialect.supportCompositeKeySplit(DatabaseMetaData)`: it reads `getDatabaseMajorVersion()`, returns false with a WARN for `< 12`, and the `ChunkSplitter.supportCompositeKeySplit()` resolver falls back to the single-column path on metadata/connection failure. The existing `testOracleCompositeSplitGatedOnDatabaseVersion` covers 11/12/23.\n\n**New fo llow-up (`eca4a3c4e`): STRING composite-key safety.** I chose the conservative option you offered: a composite primary key is only admitted to the composite path when every component is non-STRING. If any component is STRING, `findSplitKey()` now falls back to the first supported single-column primary-key component, avoiding the database-collation vs Java `String.compareTo` ordering hazard rather than merely warning while returning potentially unsafe boundaries. The English and Chinese known-limitation paragraphs now state the enforced fallback. `testFindSplitKeyFallsBackToSingleColumnForStringCompositeKey` pins the behavior. This is a behavioral safety fallback, not an exclusion of ordinary single-column STRING splitting.\n\nVerification on `eca4a3c4e`: `CompositeKeyChunkSplitterTest` **20/20**, `spotless:check` **SUCCESS**, and `git diff --check` clean. A fresh CI run is now triggered on the new head; I'll post the link/result here.\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]
