li3zhi4 commented on PR #11746: URL: https://github.com/apache/seatunnel/pull/11746#issuecomment-5989967431
Thanks @SEZ9 \u2014 both remaining items are pushed on head `02a265f51`, and below is the pointer list you asked for.\n\n**The dev-sync caveat you raised first \u2014 good catch, and it's a real (but non-correctness) asymmetry.** The upstream change the merge pulled in is `ChunkSplitter.applyWhereCondition(JdbcSourceTable)` wrapping the metadata queries with `config.getWhereConditionClause()` (backed by `SqlWhereConditionHelper.applyWhereConditionWithWrap` and a new `JdbcSourceTable.withQuery()` copy-constructor). It is wired into the **single-column** paths only \u2014 `queryMin`, `queryMinMax`, `validateStringRangeSplit`, `sampleDataFromColumn`, `approximateRowCntStatement`, `queryNextChunkMax`. The composite boundary queries (`queryMinMaxComposite`, `queryNextChunkMaxComposite`, `queryMinComposite`) build their FROM from `jdbcDialect.tableIdentifier(table.getTablePath())` and do not apply it, while the composite **read** path does (`createDynamicSplitQuerySQL` wraps the where c ondition). So boundaries are computed over the unfiltered table while reads are filtered.\n\nNo rows are lost or duplicated \u2014 tuple ranges are contiguous half-open intervals covering the whole key domain, and a filter only removes rows, so every row of the filtered set still lands in exactly one chunk. The impact is chunk-size skew and possibly empty chunks when the filter is selective \u2014 the same imbalance that upstream's change fixes for the single-column path. It is reachable: a dynamic job reading a plain table source (blank `table.getQuery()`) with `where_condition` set does reach the composite branch. You are right that no test covers the combination, and I've left it as a **recommended follow-up** rather than folding a behavioural change (composite FROM wrapping while preserving the `IS NOT NULL` guards) into this PR without its own E2E.\n\n**Items 1-3 (already implemented, now with pointers at `02a265f51`):**\n\n1. **SQLite NULL PK components** \u2014 helpers `build NotNullKeyCondition` / `buildNullKeyCondition` at `DynamicChunkSplitter.java:1487` / `:1504`; `IS NOT NULL` appended in `queryMinMaxComposite:227`, `queryNextChunkMaxComposite:304`, `queryMinComposite:386`; read predicates in `buildCompositeCondition:1399` with the first-split `IS NULL` disjunct at `:1418` and the middle/last `IS NOT NULL` guards at `:1428` / `:1436` (rationale in the Javadoc at `:1387-1393`). Tests: `CompositeKeyChunkSplitterTest.java:512` and `JdbcSqliteSplitIT#testCompositeKeyWithNullPrimaryKeyComponent` (`connector-jdbc-e2e-part-3`, line 260) \u2014 the fixture is built so the NULL rows land strictly inside a middle chunk's range, and it asserts each NULL row is read exactly once.\n2. **`COMPOSITE_KEY_SEPARATOR`** \u2014 constant at `DynamicChunkSplitter.java:125`, with the metadata-only / never-decoded Javadoc at `:115-124`. It is only joined for display at `:184`; the split key is taken back from `splitKey.getFieldNames()` in memory at `:1046`, never by parsin g the string.\n3. **`compareArrays`** \u2014 now delegates per element to `compareCompositeElement` (`:459`), which normalizes any `Number` pair through `compareNumeric` (`:474`, BigDecimal compare, with a `Double.compare` fallback for NaN/Infinity) and sorts null first. Tests at `CompositeKeyChunkSplitterTest.java:463`, `:488`, `:495`.\n\n**Items 4-5 (pushed in `02a265f51`):**\n\n4. **Upgrade note / opt-out** \u2014 added an \"Upgrading\" paragraph to `docs/en/connectors/source/Jdbc.md:309` and the matching \u5347\u7ea7\u63d0\u793a to `docs/zh/connectors/source/Jdbc.md:305`, right after the opt-out discussion and before the collation limitation. It states that existing composite-PK jobs now get composite splitting when the dialect supports it, that the rows read stay identical while chunk boundaries/distribution may differ, and that `partition_column` remains the explicit opt-out to force the previous single-column split. The collation caveat itself is at `docs/en/...:311` / `docs/ zh/...:307`.\n5. **`FixedChunkSplitter` guard** \u2014 both of your options are now satisfied. The composite branch in `findSplitKey` was already gated on `config.isUseDynamicSplitter()` (`ChunkSplitter.java:431-433`), so a composite key is only ever produced on the dynamic path \u2014 I've added a comment at `:428-430` making that explicit. On top of that, `FixedChunkSplitter.createSplits` now throws `JdbcConnectorException` on a multi-column key (`FixedChunkSplitter.java:65-76`) instead of silently splitting on field 0. New tests: `testFixedSplitterFindSplitKeyNeverReturnsCompositeKey` and `testFixedSplitterRejectsCompositeSplitKey`.\n\nVerification: `CompositeKeyChunkSplitterTest` 19/19, and a broader regression run of the chunk-splitter/sql-helper tests 164/164 (including `FixedChunkSplitterTest` and `DynamicChunkSplitterTest`) to confirm the new guard regresses nothing. `spotless:check` clean on `connector-jdbc`. The CI rerun of `Dead links` / `changes` is in flight \u2014 I'll post the 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]
