DanielLeens commented on PR #11746:
URL: https://github.com/apache/seatunnel/pull/11746#issuecomment-6266183693
Thanks @li3zhi4 — I checked all four against
`056524ee04dff75ae38ad78d9194909776a90fc4` locally rather than taking the
pointers at face value, and all four hold up. Conceding each:
- **F5** — correct. `DynamicChunkSplitter.createCompositeSplit` only joins
the names for split *metadata* (`String.join(COMPOSITE_KEY_SEPARATOR,
splitKey.getFieldNames())`, `DynamicChunkSplitter.java:183`); the boundary walk
and the chunk predicates are built from the structured `SeaTunnelRowType` and
the boundary arrays. I traced every consumer of `getSplitKeyName()` in the
module: the two `FixedChunkSplitter` sites (`:363`, `:450`) are the
single-column path and quote the name as one identifier, and
`buildSingleColumnCondition` (`:1357-1361`) is only reached from the
single-column strategy. The comma-joined string never reaches SQL. No
production change needed — agreed.
- **F6** — correct. `compareArrays` (`:432-441`) delegates element-wise to
`compareCompositeElement` (`:453`), which normalises `Number`s through
`BigDecimal` and only falls back to `ObjectUtils.compare` for non-numerics;
there is no `Arrays.equals` on raw JDBC values.
`testCompareCompositeElementMixedNumericTypes` pins Integer/Long mixing,
`Integer.MAX_VALUE` vs `Long.MAX_VALUE`, and equal-value/different-scale
`BigDecimal("1.0")` vs `BigDecimal("1.00")`. Agreed.
- **F7** — correct, and better than I asked.
`docs/en/connectors/source/Jdbc.md` documents the composite-tuple behaviour,
the dialect/version gates, the NULL-component handling, an explicit "Opting
out" paragraph (`partition_column` restores the previous single-column split,
`enable_concurrent_read = false` disables split analysis), and an "Upgrading"
paragraph stating that boundaries and row-to-chunk distribution change while
each row is still read exactly once. ZH mirrors it.
- **F8** — correct. `FixedChunkSplitter.createSplits` throws
`JdbcConnectorException(ILLEGAL_ARGUMENT, ...)` when `splitKey.getTotalFields()
> 1`, with a comment explaining it is a defensive guard for the
`isUseDynamicSplitter()` gate.
`testFixedSplitterFindSplitKeyNeverReturnsCompositeKey` and
`testFixedSplitterRejectsCompositeSplitKey` (`:757-805`) cover the gate and the
fail-fast.
One item from my earlier note is now narrowed rather than closed, and I'd
like your call on it. The docs state the composite path applies when "all key
columns are numeric **or date** types", but every validated-dialect E2E fixture
still uses the same `order_id BIGINT, line_no INT` pair
(`JdbcMysqlSplitIT:744-745`, `JdbcPostgresSplitIT:110-111`,
`JdbcSqlServerSplitIT:107-108`, `JdbcSqliteSplitIT:74-75`,
`JdbcOracleSplitIT:116`). STRING is now explicitly documented as falling back
to the single-column path with a stated collation reason, so that half is fine.
The DATE half is the gap: `DATE` is allowed through the gate and then flows
through tuple comparison and boundary SQL that no test exercises on the
composite path.
Cheapest fix, your pick:
- **Option A** — add one composite case with a `DATE` component (a numeric
column paired with a `DATE` is enough) on one validated dialect, asserting
every row is read exactly once with no duplicates. Small, and it pins the
documented promise.
- **Option B** — narrow the doc to "numeric" only and add `DATE` to the
known-limitation paragraph alongside STRING, deferring date support explicitly.
Either way the docs and the gate end up telling the same story, which is
what I was worried about.
Also updating one stale note of mine: the `Build` check on this head is now
green, so my earlier "Build is also failed on this head" doesn't apply any more.
Thanks for the detailed rebuttals — checking rather than assuming was the
right instinct, and the composite-key work is in good shape.
--
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]
