[yzeng1618]

Purpose of this pull request

Chunk-split sampling sorted the sampled split keys on the Java side with Arrays.sort (String.compareTo, i.e. UTF-16 code-unit order), while the snapshot/split chunk reads filter rows with WHERE key >= ? AND NOT(key = ?) AND key <= ?, which the database evaluates under its own collation. For string split keys (varchar/uuid primary keys) the two orderings disagree (e.g. en_US.UTF-8 on PostgreSQL, utf8mb4_0900_ai_ci on MySQL, SQL_Latin1_General_CP1_CI_AS on SQL Server), so:

• chunk ranges overlap: the same row matches two chunks and is read twice (duplicate records at the sink),
• or invert: some chunks stay permanently empty,
• and rows can concentrate into a single giant chunk (the unbounded last chunk swallows most of the key space), overloading one subtask — we observed a 130M-row single chunk OOM a TaskManager in production.

This PR adds ORDER BY <split column> to the sampling query and removes the Java-side Arrays.sort, in all five CDC connectors and in the JDBC batch source, so the sample, the chunk boundaries and the chunk WHERE filters all live in the same (database) order. For numeric split keys nothing changes (Java numeric order equals database order); the fix matters for string keys.

PostgreSQL uuid detail: the SELECT list keeps the convertType cast ("id"::text, matching the split-key type the snapshot WHERE filters compare), but the ORDER BY deliberately uses the RAW column. An ORDER BY on the cast expression cannot use the uuid btree index and would force a seq scan plus a server-side external sort on huge tables; the raw uuid column orders by uuid byte order, which is identical to the hex-text order that ::text comparisons use, so the boundaries stay consistent while the planner can satisfy the ORDER BY with an index scan.

Additional cleanups in the same area:

• The JDBC batch sampling now skips NULL sample values: a null boundary breaks the chunk range construction (and crashed the removed Arrays.sort with an NPE). NULL keys never match any range predicate anyway.
• The unused MOD-based sampling variants in the CDC utils (dead code — the splitters only call the streaming variant) are removed; the streaming sampling methods are renamed to sampleDataFromColumn (skipReadAndSampleData for PostgreSQL, which has a column-aware overload).

Does this PR introduce any user-facing change?

No configuration-surface change (no options added/renamed/removed). Behaviour: for tables with string split keys that reach the sampling split path, chunk boundaries are now computed in database collation order, eliminating duplicate reads across chunk boundaries, permanently empty chunks and single-giant-chunk skew. On engines without a servable index for the split column (e.g. a user-configured non-leading snapshotSplitColumn, or MPP engines using the default dialect), the ORDER BY introduces a server-side sort of the sampled column — the same trade-off the per-chunk boundary queries (queryNextChunkMax) already make today.

How was this patch tested?

Unit tests added/extended:

• PostgresUtilsTest (extended): uuid sampling SQL shape — SELECT "id"::text FROM ... ORDER BY "id" (cast kept in SELECT, raw column in ORDER BY); non-uuid SQL shape; the sample preserves database collation order without a Java re-sort; auto-commit state is restored after sampling.
• Db2UtilsTest (extended): sampling SQL contains ORDER BY on the raw column.
• DuckDBDialectTest (extended, runs against a real DuckDB): NULL sample values are skipped (this test fails with an NPE on the old code); the sample is returned in database order rather than physical insertion order.
• The existing DuckDBDialectTest#testSampleDataFromColumn and the Oracle dialect sampling IT keep passing — ORDER BY does not change their results.

The sampling split path cannot be reached by the existing E2E suites (their tables are far below the split.sample-sharding.threshold), so no E2E changes were needed. spotless:apply was applied to all touched modules; remaining verification is left to CI.

[nzw921rx]

split.sample-sharding.threshold
This is configurable and requires adding e2e to cover it.

[DanielLeens]

Thanks for the thorough work on this, @yzeng1618. Chasing a duplicate-read / giant-chunk problem down to a Java-vs-database ordering mismatch is a sharp diagnosis, and the PR description is very clear. Below is a full review of head fbef7247.

What Problem Does This PR Solve?

User pain point: With string split keys (varchar/uuid primary keys), chunk boundaries were derived from a Java Arrays.sort (UTF-16 code-unit order), while the snapshot/split reads filter with WHERE key >= ? AND key < ?, which the database evaluates in its own collation. For mixed-case keys the two orders disagree, so chunks can overlap (duplicate rows at the sink), invert (permanently empty chunks), or collapse into one giant last chunk (the reported 130M-row OOM).

Fix approach: Push the ordering into SQL (ORDER BY <split column>) in the sampling query of the five CDC connectors (MySQL, Oracle, PostgreSQL, SQL Server, DB2) and in the JDBC batch source (JdbcDialect default plus the MySQL, OceanBase-MySQL, Oracle and YashanDB overrides), and drop the Java re-sort. It also skips NULL samples in the JDBC path and removes dead MOD-based sampling helpers.

One-sentence summary: The sample, the chunk boundaries and the chunk WHERE filters now all use the database's order instead of Java's.

1. Code Change Review

1.1 Core Logic Analysis

Before (all affected paths):

"SELECT %s FROM %s"
...
Object[] resultsArray = results.toArray();
Arrays.sort(resultsArray);
return resultsArray;

After:

"SELECT %s FROM %s ORDER BY %s"
...
return results.toArray();   // already in database order

Runtime path (normal path does reach the changed code, but only when sampling sharding is taken):

• CDC: AbstractJdbcSourceChunkSplitter (connector-cdc-base, the two branches that call sampleDataFromColumn(jdbc, tableId, splitColumn, inverseSamplingRate), around lines 207 and 252) -> connector *ChunkSplitter.sampleDataFromColumn -> *Utils.sampleDataFromColumn / PostgresUtils.skipReadAndSampleData -> efficientShardingThroughSampling (picks sampleData[(int)((i+1)*approxSamplePerShard)] as chunk ends, so it relies on the array being sorted in the same order the WHERE filters use).
• JDBC: DynamicChunkSplitter (around line 499) -> JdbcDialect#sampleDataFromColumn (or a dialect override) -> efficientShardingThroughSampling.
• Sampling is only chosen when split.sample-sharding.threshold < shardCount (or the CDC distribution-factor > 1 branch), so small tables and typical E2E tables never reach it. This matches the PR description.

Key findings:

• The root-cause analysis is correct: efficientShardingThroughSampling indexes into the sample positionally, so any order mismatch with the DB predicate directly produces overlapping/inverted ranges. Moving the sort to the DB is the right location (more upstream and more robust than trying to emulate collations in Java).
• The PostgreSQL uuid handling is thoughtful: SELECT keeps the ::text conversion (matching the split-key type used by the WHERE filters), ORDER BY uses the raw column so a btree index can serve it. I could not run this against a real PostgreSQL, so the claim "uuid byte order == hex-text order under the column's text comparison" is accepted from the analysis, not verified by me (see Issue 1).
• Removing the MOD-based sampleDataFromColumn overloads: I found no remaining callers in seatunnel-connectors-v2 or seatunnel-e2e after the splitters are updated by this PR, so they were indeed dead code. Renaming skipReadAndSortSampleData to sampleDataFromColumn / skipReadAndSampleData touches only public static methods of connector-internal *Utils classes; I found no external references.
• NULL skipping in the JDBC path is a reasonable hardening, and correct: a NULL boundary would break range construction.

1.2 Compatibility Impact

Fully compatible. No option is added/renamed/removed, no checkpoint/state/serialization format changes (chunk ranges are computed at enumeration time; already-persisted splits are unchanged). Behavior changes only for string split keys that take the sampling path, and only toward correctness. For numeric keys, sorted order is identical.

1.3 Performance / Side Effects

• Previously the DB streamed the table unordered and Java sorted a ~1/N-sized array. Now the DB must produce an ordered stream of the whole column. When the split column is the leading column of an index (the default primary-key case) this is an index scan with no extra sort. When it is not (user-configured non-indexed snapshotSplitColumn, MPP/columnar engines using the default dialect, or a custom query wrapped as (query) AS T), the database must sort the full column, which on a multi-billion-row table can mean a long-running sort with temp-space spill. The PR description already acknowledges this trade-off; I record it as Issue 2 so it is visible in the release notes/docs rather than a blocker.
• Java-side memory is improved (no copy + sort of the array), and the streaming fetch-size behavior is unchanged.

1.4 Error Handling and Logging

No new swallowed exceptions; the interrupt check in the loops is preserved. Formal issues:

Issue 1 - Real-database verification of the ordering claim is missing

• Location: whole PR; tests in PostgresUtilsTest, Db2UtilsTest (mocks) and DuckDBDialectTest.
• Problem: The new CDC tests assert SQL text against a mocked Statement/ResultSet; the PostgreSQL "preserves database order" test just feeds a pre-ordered mock and asserts it is not re-sorted. Nothing exercises a real DB collation against the real chunk WHERE predicates, which is the exact bug being fixed. Only the DuckDB test touches a real engine, and DuckDB's default ordering is binary, so it cannot show a collation divergence.
• Potential risk: A subtle mismatch (e.g. uuid raw-order vs ::text comparison, or collation-equal-but-distinct values such as Apple/apple adjacent in the sample, which efficientShardingThroughSampling de-duplicates with equals, not collation equality) would go undetected.
• Best improvement: Add one mixed-case string-PK scenario that forces the sampling path (e.g. set split.sample-sharding.threshold low and a small chunk-size/split.size so shardCount exceeds it) against a case-insensitive collation (MySQL default collation is enough) and assert no duplicate/missing rows. The configuration is user-adjustable, so the claim in the PR that E2E cannot reach this path can be addressed by lowering the threshold in the test config.
• Severity: Medium
• Raised by another reviewer: Yes (@nzw921rx asked for an E2E covering split.sample-sharding.threshold). I agree with that request and add the collation-focused detail above.

Issue 2 - Full-column sort cost on non-indexed split columns is undocumented for users

• Location: JdbcDialect#sampleDataFromColumn, Mysql/OceanBase/Oracle/Yashan overrides, and the five CDC *Utils.sampleDataFromColumn.
• Problem: The ORDER BY is now unconditional. For non-indexed split columns the DB performs a full sort of the column.
• Potential risk: Long enumeration time / temp-space pressure on very large tables for users who previously had a fast streamed read plus Java sort of a small array.
• Best improvement: One sentence in the split.sample-sharding.threshold / snapshotSplitColumn docs (docs/en and docs/zh) noting that the split column should be indexed for sampling-based splitting. No code change required.
• Severity: Low
• Raised by another reviewer: No

2. Code Quality

2.1 Coding Standards

Good: the key behavioral points (why ORDER BY on the raw column, why no Java re-sort, why NULLs are skipped) are documented with clear comments and Javadoc. Spotless is reported as applied. No missing comments on new non-trivial methods that I could find. Style nit only: the same long explanatory block is repeated in each of five CDC utils; that is acceptable here since the utils are intentionally connector-local and keep the change out of shared classes.

2.2 Test Coverage and Test Stability

Rating: Stable (as to flakiness). The added tests are deterministic mocks plus an in-process DuckDB; I see no timing, port or container dependence.

Coverage observations (not stability):

• Good: SQL-shape assertions would fail if ORDER BY or the uuid raw-column ordering were removed; the "no Java re-sort" test would fail if Arrays.sort returned; the DuckDB NULL test fails with an NPE on the old code, so it has real failure power.
• Gaps: no unit tests were added for the MySQL, Oracle and SQL Server CDC utils or for the Mysql/OceanBase/Oracle/Yashan dialect overrides (only the shared default path is tested in DuckDB), so a typo in one override's ORDER BY would not turn anything red. Because the overrides are near-copies, a small parameterized SQL-shape test would be cheap (Issue 3).

Issue 3 - Dialect overrides and three CDC utils lack a test that would catch a dropped/misplaced ORDER BY

• Location: MysqlDialect, OceanBaseMysqlDialect, OracleDialect, YashanDbDialect (sampleDataFromColumn); MySqlUtils, OracleUtils, SqlServerUtils.
• Problem: Only JdbcDialect default (DuckDB), Postgres and DB2 are covered.
• Potential risk: A regression in an untested override silently returns to unordered samples.
• Best improvement: Add mock-based SQL-capture tests similar to Db2UtilsTest, covering both the table-path and custom-query variants.
• Severity: Low
• Raised by another reviewer: No

2.3 Documentation

No option or default changed, so a docs update is not strictly required. See Issue 2 for a recommended one-line note in both docs/en and docs/zh (keep parity if added).

3. Architectural Soundness

3.1 Precise fix / Temporary workaround / Long-term solution

Precise fix (root-cause level). It removes the divergent ordering instead of masking it, and places the change in the sampling layer where the order is produced.

3.2 Maintainability

Net -192/+380 with mostly tests and comments; dead MOD helpers are removed, and the rename makes the contract ("returns DB-ordered sample") visible. The five-way duplicated comment is a mild maintenance cost, but consistent with the existing connector-local structure.

3.3 Extensibility

New dialects inheriting the JdbcDialect default get correct ordering automatically; dialect overrides must repeat the ORDER BY (they do in this PR). Not touching shared engine/API classes keeps the blast radius contained.

3.4 Historical-version compatibility

No persisted-state or config change. Jobs restored from checkpoint reuse already-computed splits. Jobs re-enumerating on restart will compute boundaries with the corrected order, which is the intended fix. Residual edge: values that are equal under a case-insensitive collation but different as strings (e.g. Apple and apple) may appear as consecutive samples; efficientShardingThroughSampling de-duplicates by equals, so an empty-ish chunk could remain. This is pre-existing and far less severe than the bug being fixed, but worth being aware of when designing the E2E in Issue 1.

4. Issue Summary

Number	Issue	Location	Severity
1	No real-DB / collation verification of the ordering fix (E2E with lowered sampling threshold requested)	tests across PR; E2E suites	Medium
2	Unconditional ORDER BY means full-column sort on non-indexed split columns; not documented	JdbcDialect, dialect overrides, CDC utils	Low
3	No tests for 4 JDBC dialect overrides and MySQL/Oracle/SQL Server CDC utils	dialect and utils classes	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers (non-CI): none are hard correctness blockers in the code itself; the change is correct by my reading of the call chain.

Recommended before merge (non-blocking):

• Issue 1: add an E2E (or at least a real-database IT) with a lowered split.sample-sharding.threshold and mixed-case string keys, as also requested by @nzw921rx.
• Issue 3: small SQL-shape tests for the untested overrides.
• Issue 2: one documentation sentence (docs/en and docs/zh).

Overall assessment: Good, well-scoped root-cause fix with clear comments, compatible with existing jobs. CI "Build" is currently green on this head; I did not run builds or tests locally, so my conclusions come from reading the code and the CI result. Risk is low for correctness and medium-low for performance on unindexed split columns.

Alternative considered: keeping the Java sort but sorting with a collation-aware comparator is not viable because the database collation cannot be reproduced reliably client-side; the SQL ORDER BY approach chosen here is the better one.

Thanks again for the careful work.

[DanielLeens]

Thanks for adding the requested coverage and documentation. I re-read the full diff at 9dab9a1, including the sampling callers and the actual range-query builder. The new MySQL test exercises generated split boundaries against a real database, the previously uncovered overrides have SQL-shape tests, and both JDBC guides explain the sorting cost. I have no remaining blocking source finding; current-head Build is green.

What Problem Does This PR Solve?

User pain point: Sampling-based split boundaries were sorted in Java while the database evaluated range predicates in its own collation. Different orders can overlap ranges, miss balanced boundaries and concentrate work in large chunks.

Fix approach: Order the sampled column in SQL and preserve that result order across five CDC implementations and the JDBC default/overridden samplers. Remove the obsolete arithmetic sampling helpers and skip null JDBC sample values.

One-sentence summary and example: A database that orders a mixed-case key sequence differently from Java now supplies boundaries in the same order used by its >= and < predicates.

1. Code Change Review

1.1 Core Logic Analysis

The CDC abstract splitter reaches sampling for eligible uneven distributions and the existing gapped-ID branch; connector splitters delegate to their updated utilities. JDBC DynamicChunkSplitter reaches the default/dialect sampler when sampling is enabled/supported and estimated shard count exceeds the threshold. Positional sample boundaries feed efficientShardingThroughSampling without a second Java sort. JDBC query-based sampling orders the outer projected column; Oracle/Yashan retain their no-AS derived-table syntax.

Before:

SELECT key FROM table;
Arrays.sort(sampledKeys);

After:

SELECT key FROM table ORDER BY key;
return sampledKeysInDatabaseOrder;

Key Findings

• All five CDC splitter callers use the renamed sampling helpers; no internal caller remains for the removed skipReadAndSortSampleData methods. The removed MOD helpers had no remaining production callers.
• JDBC default, MySQL, OceanBase-MySQL, Oracle and Yashan samplers all add ORDER BY for both table and query forms.
• PostgreSQL selects the converted UUID text but orders the raw column; existing cursor/auto-commit restoration remains.
• JDBC samplers discard null boundary values. This does not add a general guarantee that nullable split-key rows are covered by range predicates or cure every empty-sample edge.
• Explicit string strategy selection bypasses the legacy sampling path; the new test leaves split.string-strategy unset, and the default string split mode is SAMPLE.
• The new MySQL fixture lowers the sampling threshold to 3 with split.size=4 and 24 analyzed mixed-case primary keys.

Complete runtime path

CDC source enumerator -> AbstractJdbcSourceChunkSplitter
 -> selected connector sampler -> ordered database cursor -> every Nth key
 -> positional ChunkRange boundaries -> SnapshotSplit -> database range read
JDBC source enumerator -> DynamicChunkSplitter -> legacy SAMPLE path
 -> dialect.sampleDataFromColumn -> ordered samples -> JdbcSourceSplit
 -> createDynamicSplitQuerySQL + prepareDynamicSplitStatement -> JDBC reader

Adjacent boundaries drawn in the database order make the half-open range predicates coherent. The MySQL regression checks count, distinct count and exact key set across all generated ranges; its first/middle/last predicates and parameter order match the production query builder. It is a real-database splitter/predicate IT, not a full SeaTunnel job or a five-database CDC acceptance test. Collation-equal but Java-distinct samples can still produce redundant empty ranges, an existing boundary-deduplication limitation. PostgreSQL UUID SQL shape is verified by mocks; no universal claim about arbitrary custom text collations or measured index plans is made.

1.2 Compatibility Impact

Partially incompatible. Job options/defaults, split serialization and checkpoint formats are unchanged. Already persisted split boundaries are reused rather than rewritten. Fresh enumeration changes physical sampling positions as well as ordering, including numeric samples from a previously unordered scan. Connector-internal public utility method renames/removals can affect external code directly calling those helpers; this is not a blanket Java binary-compatibility promise. No connector/factory/distribution registration is removed.

1.3 Performance / Side-Effect Analysis

The database now orders the full column before client-side every-Nth-row sampling. A suitable leading-column index can help avoid an explicit sort, but custom queries/planner choices may still sort or spill. Java still allocates the returned sample array; removing Arrays.sort removes sorting work, not that array allocation. Cursor/fetch-size behavior and interruption checks are retained. Both JDBC guides now expose the index/sort tradeoff.

1.4 Error Handling and Logging

Sampling SQL failures propagate through enumeration failure handling. Existing result-set/statement cleanup and PostgreSQL auto-commit restoration remain; no new exception suppression or retry loop is added. JDBC null filtering prevents null values from becoming boundaries, within the pre-existing split-key semantics.
No blocking source issues found. No numbered source findings remain for this head.

2. Code Quality Assessment

2.1 Coding Standards

The order contract is documented at every overridden sampler and in the shared JDBC method. PostgreSQL separates selected expression from ordering expression explicitly. The changes preserve connector-local quoting, streaming and cleanup behavior rather than adding a generic collation emulator.

2.2 Test Coverage and Test Stability

Test stability: Stable. New SQL-capture tests cover every touched CDC utility and both table/query variants of the four JDBC overrides; PostgreSQL additionally verifies retained result order and auto-commit restoration. DuckDB tests cover non-null sample order and null filtering in process. The MySQL test uses the existing pinned mysql:8.0.43 fixture with mapped ports, 24 deterministic keys and ANALYZE TABLE, then asserts complete nonduplicated range coverage. It does not assert that every resulting range is nonempty, despite its introductory wording. No new sleeps, time-of-day assumptions or unbounded polling were introduced. No local tests or containers were run.

Local analysis: fetched the head, created independent branch seatunnel-review-12991, read the complete 28-file diff and traced affected callers with git show/git grep. No local compilation, unit tests, integration tests, E2E, containers, or job execution was performed, as requested. These are source-analysis conclusions; execution evidence is the current-head CI below.

2.3 Documentation Updates

English and Chinese JDBC guides now explain ORDER BY and recommend an index led by the split column. A similar note beside CDC sampling options would be useful documentation follow-up, but is not a new code or merge blocker. No new configuration option is introduced.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. The database owns the ordering used by SQL comparisons, so ordering at sample production is the correct boundary. Emulating arbitrary database collations in a Java comparator would add complexity without establishing the same ordering contract.

3.2 Maintainability

SQL-shape tests now protect the duplicated overrides. Keep the result array in database order through boundary construction; a future Java sort would restore the original mismatch.

3.3 Extensibility

New samplers must return keys ordered consistently with their eventual range predicate, including any cast or explicit collation used by that predicate. The common JDBC default supplies the ordering for inheriting dialects.

3.4 Historical-Version Compatibility

Configuration and stored-state compatibility are retained. Existing checkpoints containing already computed bad ranges are not repaired by this change; fresh enumeration uses corrected ordering. No live restore/upgrade or full-scale sort-cost validation is claimed.

4. Issue Summary

Number	Issue	Location	Severity
—	No formal issues found	Complete changed-file set	—

5. Merge Recommendation

Conclusion: Ready to merge

1. Blockers — must be fixed
   • No source blocker identified. No source or current-head CI blocker remains. Effective maintainer approval is still required.
2. Recommended fixes — non-blocking
   • No additional unrelated changes requested.

Overall assessment: The original ordering mismatch is fixed at its source, and the requested real-database coverage plus override tests are now present.

Existing-review reconciliation: +1 to nzw921rx's request to lower the configurable threshold in a real-database test: JdbcMysqlSplitIT now implements that scenario, so the old coverage issue is resolved rather than duplicated. My previous override-test and user-doc requests are also addressed. I correct the earlier Fully compatible label for direct consumers of renamed public internal helpers, and the earlier suggestion that removing sort removes the sample-array allocation. No independent maintainer APPROVED/CHANGES_REQUESTED event was present in the refreshed history. Author replies have been incorporated into this single full review.

Head 9dab9a15bdedc5c9369f96f6b6f5c0a8c91cd861. Live base dev at 3ac2d1cec6aa7b3741c4dacdd23020f778e23c02; comparison diverged, ahead 2 / behind 57; merge state BLOCKED. Behind/diverged alone is recorded metadata, not a request to synchronize upstream. Checks: Signal pull request update = SUCCESS; Build = SUCCESS

Current-head Build and PR signal are SUCCESS, with fork run 41021846488 showing successful updated-module integration lanes. This is remote CI evidence; it is not a claim that I ran the suites locally. No upstream-sync or retry request is warranted from the current green checks.

Daniel has READ repository permission. This COMMENT records the technical conclusion; it is not an effective maintainer approval or a dismissal of any historical requested-changes entry. A write-capable maintainer owns the final repository approval gate.