DanielLeens commented on code in PR #11746:
URL: https://github.com/apache/seatunnel/pull/11746#discussion_r3800825010
##########
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/source/ChunkSplitter.java:
##########
@@ -356,7 +354,35 @@ protected Optional<SeaTunnelRowType>
findSplitKey(JdbcSourceTable table) {
PrimaryKey pk = schema.getPrimaryKey();
if (pk != null) {
- for (String pkField : pk.getColumnNames()) {
+ List<String> pkColumnNames = pk.getColumnNames();
+ // Composite primary key: use all key columns (tuple-ordered
split). Only the dynamic
+ // splitter supports multi-column boundaries, and only for
dialects whose composite-PK
+ // path is validated by an official E2E (see
JdbcDialect.supportCompositeKeySplit(),
+ // default false); unvalidated dialects fall back to the
single-column behavior below.
+ // All key columns must also be splittable (Comparable) types - a
composite PK
+ // containing e.g. BINARY/VARBINARY would otherwise fail
compareArrays with a
+ // ClassCastException, so such keys also fall back to the
single-column path.
+ if (pkColumnNames.size() > 1
+ && config.isUseDynamicSplitter()
+ && jdbcDialect.supportCompositeKeySplit()) {
+ List<Column> pkColumns = new ArrayList<>();
+ for (String pkField : pkColumnNames) {
+ Column column = columnMap.get(pkField);
+ if (column != null && isSupportSplitColumn(column)) {
Review Comment:
@LeonYoah - you're right, and this changes my earlier assessment.
`isSupportSplitColumn()` (ChunkSplitter.java:426-443) allows
TINYINT/SMALLINT/INT/BIGINT/DOUBLE/FLOAT/DECIMAL/STRING/DATE as split-column
types, and `findSplitKey()` (:365-384) uses that exact predicate to decide
whether a composite PK is eligible for the dynamic tuple-ordered path - nothing
there restricts composite keys to numeric columns. But I checked the
composite-key E2E fixtures added across all five validated dialects, and every
one of them uses the same `order_id BIGINT, line_no INT` pair:
JdbcMysqlSplitIT.java:744-745, JdbcPostgresSplitIT.java:110-111,
JdbcSqlServerSplitIT.java:107-108, JdbcSqliteSplitIT.java:74-75,
JdbcOracleSplitIT.java:116. None of them exercise a STRING or DATE column
inside the composite key, so the tuple comparison
(`DynamicChunkSplitter.compareArrays()`, :414-423, backed by
`ObjectUtils.compare()`) and the boundary/predicate SQL generation for those
types are unverified on the composite path, even though the type system says
they're supported.
This is a fair correction to my "ready to merge" note from earlier - I'll
treat it as a blocking item alongside the other open threads: please add at
least one composite-key case per validated dialect (or a dedicated case on one
dialect, e.g. MySQL or PostgreSQL, if per-dialect coverage is too heavy) that
pairs a numeric column with a STRING or DATE column, and assert the boundary
walk and final chunk predicates still read every row exactly once with no
duplicates. If STRING/DATE composite keys are intentionally out of scope for
this PR, that restriction needs to be enforced explicitly rather than left as
an untested code path, and documented on `supportCompositeKeySplit()`.
--
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]
