sharpca commented on PR #13083: URL: https://github.com/apache/seatunnel/pull/13083#issuecomment-6607320700
Thanks @DanielLeens, and thanks for re-reading the IT at the new head instead of assuming the last commit covered it — you are right, cabfb80dd only touched the unit test, and Issue 1 is still open. I have now changed the IT instead of the production code, so the diff outside the IT is unchanged. **Issue 1 (blocking) — the IT now asserts on the split bounds** The IT now generates the splits twice from the same table, once under `Locale.ROOT` and once under `tr-TR`, and compares the two lists of `(splitStart, splitEnd)`. The row-identity check stays as an extra guard. Your trace is exactly right, and the interesting part is *why* the row check cannot see the bug. `CollationBasedSplitter#charToIndex` returns `supportedChars.indexOf(c) + 1`, and `indexOf` is `-1` for a character that is not in the collation sequence, so an unknown character encodes as **0 — the same index as the padding character**: ```java int result = (c == '\u0000') ? 0 : supportedChars.indexOf(c) + 1; ``` Under `tr-TR`, `"I1".toLowerCase()` gives `ı1`, `ı` is not in the sequence, so MIN (`I1`) encodes as the padding index instead of the index of `i`. MAX (`I9`) is affected the same way. The encoded range therefore collapses towards zero, and because `decodeNumericRangeToString` maps index 0 back to `\u0001`, every interior bound comes back as `"\u00013"`, `"\u00014"`, ... instead of `"i3"`, `"i4"`, .... The first split still starts at the literal MIN and the last still ends at the literal MAX, so the rows are still read exactly once — they just all land in one split. That is why the previous assertions passed with and without the fix, and it also means the decoded bounds contain a control character, which is easy to assert on. **Issue 2 — the collation is now set explicitly, and it is not cosmetic** The test config now sets `split.string_split_mode_collate= utf8mb4_general_ci`. Besides documenting where `isCaseInsensitive` comes from, it makes the fixture actually exercise the branch we are fixing: `FixedChunkSplitter` calls `getCollationSequence(connection, collate)`, which sorts 32..126 with that collation and treats the sequence as case-insensitive only when it matches `.*[aA][Aa].*`. With the option unset, that decision was left to the server/driver default, so on a server whose default collation does not tie `A` with `a` the lower-casing line would not run at all and the test would pass without testing anything. I left a comment in the test explaining the split of responsibilities: the table collation orders MIN/MAX, the connection collation decides case-insensitivity. To catch that failure mode instead of silently going inert, the test also asserts that every interior bound is already folded (`bound.equals(bound.toLowerCase(Locale.ROOT))`). Under the fixed code the decoded bounds only contain characters from the lower-cased sequence, so an uppercase `I` in a bound means the case-insensitive path was not taken and the fixture needs attention. **Issue 3 — fixed** `mySqlCatalog.open()` and `getTable(tablePath)` now run before `Locale.setDefault(tr-TR)`; only `generateSplits` and the split statements run under the Turkish default. So the test no longer depends on `MySqlCatalog` from #13076, and that no longer needs a comment to be safe. **Scope** The PR description now lists all 39 touched files (35 `src/main`, 4 `src/test`) with the call site and the reason each one is locale sensitive, checked against the full diff rather than against the thread: 27 are real `i`/`I` folds, 1 is mixed (`PostgresDialect` has a real type fold plus a `where` check), 7 are hardening only because the token they fold (`where`) has no `i` and cannot change behaviour today, and 4 are tests. The hardening rows are labelled as such so they can be reviewed quickly — I would rather write "no behaviour change" than imply I fixed seven more bugs. **CI** Thank you for the correction, I had that wrong: I was reading `action_required` as an approval gate a maintainer had to clear. The build has to happen in the fork, and `sharpca/seatunnel` has no runs yet, which is why the check reports "Workflow run detection failed". The fix is on my side — enable Actions on the fork and run the `Build` workflow for this branch (or push an empty commit) — after which the PR check should adopt the run within about 15 minutes. Same for #13081. Until that run exists there is no result for this head: a missing result, not a red one. The branch is 3 commits behind `dev` and I have not re-synced, since those commits do not touch the JDBC files. I have not claimed anywhere that I ran the IT: this change is derived from reading the splitter and its two helpers, and I would rather have CI confirm it than say it works. If `generateSplits` turns out to hold state across the two calls, the fallback is two splitter instances, one per locale. -- 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]
