SEZ9 commented on PR #11746:
URL: https://github.com/apache/seatunnel/pull/11746#issuecomment-6052004127
Thanks for the mapping, that's helpful.
- **Points 4 and 5 (upgrade note / opt-out, removed single-split-key
guard):** the Upgrading section in `docs/en`/`docs/zh` `Jdbc.md` with
`partition_column` as the documented way back to single-column splitting, plus
`FixedChunkSplitter` throwing `JdbcConnectorException` on a multi-column key
and the documented `isUseDynamicSplitter()` gate in `findSplitKey`, is the
shape I was after. I'll confirm against the diff on `02a265f51` and close both
once I've looked at the changed files and the two new tests you named.
- **Points 1-3:** acknowledged as already on the earlier heads. Two small
follow-ups:
- For `compareCompositeElement`, can you confirm it handles the SQLite
Integer-vs-Long case and compares `BigDecimal` by value rather than `equals`,
so the dedup guard isn't scale-sensitive? A pointer to the test covering this
is enough.
- For the separator, the Javadoc documents the constraint. Is there also a
runtime check rejecting column names/values containing the separator, or is
this an intentional documented limitation? Either is fine, I'd just like it
stated explicitly.
Two items from my earlier review aren't in the mapping and I don't see them
addressed yet:
1. **Oracle `FETCH FIRST` on pre-12c:** if composite splitting is enabled
unconditionally in the Oracle dialect, an existing composite-PK job on 11g
would hit a SQL syntax error on upgrade instead of falling back to the
single-column split. Could you either gate the composite boundary SQL on a
version check with fallback, or at minimum call this out in the new Upgrading
section next to the `partition_column` opt-out?
2. **STRING key collation:** the composite path admits STRING key columns,
but the database's `ORDER BY` collation and the Java-side comparison can
diverge, which could collapse the walk to a single chunk or end it early. Could
you either exclude STRING columns from the composite path, or add a
guard/warning plus a test with a non-ASCII / mixed-case key showing the walk
stays stable?
On CI: thanks for the rerun breakdown on `02a265f51`. Based on your summary,
the Hudi S3 timeouts, the Paimon flake and the `README.md` badge check from
`6de6f1b953` look outside this PR, and it's good that the JDBC SplitIT shards
are green on this head.
Once the Oracle fallback and the STRING collation question are settled (or
explicitly deferred with a note) and I've checked the diff for points 4 and 5,
I'm happy to approve.
<!-- streview-comment:1601 -->
--
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]
