Repository navigation
Conversation
The JDBC dialect type converters convert the source type name with String#toUpperCase() / String#toLowerCase() without a locale before matching it against the dialect type constants. Under a Turkish or Azerbaijani default locale a lower-case "integer" upper-cases to "INTEGER" with a dotted capital I and an upper-case "INT4" lower-cases to "int4" with a dotless i, so the type no longer matches any constant and the converter falls through to its unsupported-type branch. Pass Locale.ROOT to every remaining case conversion in connector-jdbc so that the same source type maps to the same SeaTunnel type on any worker. The MySQL, OceanBase and SQL Server converters, JdbcDialect and CatalogUtils are already covered by apache#13063, apache#13076 and apache#13077 and are not touched here.
650c9fe to
5336a0b
Compare
|
While preparing the patch I noticed the scan results are broader than the ten converters, so the PR also fixes the same pattern in the remaining type mappers/converters ( |
…ions in connector-jdbc The quick scan of the rest of connector-jdbc requested on apache#13079 showed that the row count WHERE checks, the case insensitive collation normalisation and the where_condition prefix validation still lower-case the argument with the JVM default locale. Pass Locale.ROOT to those calls as well, so the remaining conversions in connector-jdbc behave like the type converters fixed in this PR. The six dialects use the same pattern that PostgresDialect already follows here; CollationBasedSplitter and JdbcSourceFactory are included because the locale changes their comparison result rather than only the spelling.
|
Added a follow-up commit for the scan requested in #13079.
Verified locally on JDK 17: |
DanielLeens
left a comment
DanielLeens
left a comment
There was a problem hiding this comment.
What Problem Does This PR Solve?
User pain point: JDBC type discovery depends on the JVM's language settings. For example, under Turkish, integer.toUpperCase() contains a dotted capital I and no longer matches the SQL type constant.
Fix approach: Use Locale.ROOT at JDBC type, catalog, URL-property, query-keyword and string-split normalization sites.
One-sentence summary: Database tokens should mean the same thing on machines with different default languages.
Thank you for contributing to Apache SeaTunnel and for following through on the wider scan. I reviewed the complete 39-file diff at 958bf389f7b58b798a3f0f472be43b32e9584f2f, including the additional split-boundary change. The normalization direction makes sense; the remaining source-side request is focused integration coverage, described below.
1. Code Change Review
1.1 Core Logic Analysis
The changes consistently add Locale.ROOT to existing case conversions; they do not replace type switches, change option names, or introduce a new converter registration. The new JdbcTypeConverterLocaleTest exercises ten converter instances under Turkish and asserts exact SeaTunnel types.
Before:
String db2Type = typeDefine.getDataType().toUpperCase();
String str = isCaseInsensitive ? s.toLowerCase() : s;After:
String db2Type = typeDefine.getDataType().toUpperCase(Locale.ROOT);
String str = isCaseInsensitive ? s.toLowerCase(Locale.ROOT) : s;Key findings:
- Normal JDBC source schema discovery reaches the catalog/type-mapping changes; this is not solely an exception-path fix.
- Query-prefix and row-count keyword checks keep their existing SQL parsing limitations. This PR makes case folding deterministic; it does not claim to implement a SQL parser.
CollationBasedSplitter.java:144also changes normalization of actual data used to calculate split boundaries. That deserves behavioral coverage distinct from type-token conversion.- No new production resource, thread, state serializer or public API is added.
Execution paths (paths below are under seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/):
JdbcSourceFactory.createSource [source/JdbcSourceFactory.java:57]
-> JdbcSource construction [source/JdbcSource.java:65]
-> JdbcCatalogUtils.getTables [utils/JdbcCatalogUtils.java:72]
-> table/query catalog lookup [utils/JdbcCatalogUtils.java:249-275]
-> AbstractJdbcCatalog.getTable [catalog/AbstractJdbcCatalog.java:216]
-> buildColumnsReturnTablaSchemaBuilder [catalog/AbstractJdbcCatalog.java:257]
-> buildColumn [catalog/AbstractJdbcCatalog.java:278]
-> database-specific type normalization and conversion
-> produced CatalogTable used by source readers
Metadata-backed dialect mapping
-> JdbcDialectTypeMapper.mappingColumn [internal/dialect/JdbcDialectTypeMapper.java:89]
-> BasicTypeDefine from driver metadata [same file:129]
-> mappingColumn(typeDefine) [same file:141]
-> converter-specific switch with Locale.ROOT
String-key range enumeration
-> FixedChunkSplitter.getJdbcSourceStringSplits [source/FixedChunkSplitter.java:119]
or DynamicChunkSplitter.charsetBasedColumnSplitChunks [source/DynamicChunkSplitter.java:312]
-> derive case-insensitivity and ordered charset from database collation
-> CollationBasedSplitter.encodeStringToNumericRange
-> stringToAsciiString [source/CollationBasedSplitter.java:144]
-> numeric subdivision and decode back to string bounds
-> JDBC source splits read by workers
The last path is conditional on string-range/collation splitting; numeric and hash splitting do not gain coverage from the changed lowercasing. The source split/state classes and restore serializers are unchanged. The SQL Server change normalizes property keys, leaving connection-property values intact. Catalog identifier folding retains its existing fold-to-upper/lower behavior while removing host-language dependence.
1.2 Compatibility Impact
Compatible API, configuration names/defaults, transport protocol and serialized state shapes. Behavior intentionally changes for affected locale-sensitive inputs. Existing jobs that accidentally depended on the machine locale may derive different string split boundaries on fresh enumeration; the result-set completeness of that path should be protected by the integration case below. No incompatible API/state-format migration is introduced.
1.3 Performance and Side Effects
Case conversion remains linear in input length with the existing string allocation pattern. No new buffers, network calls, locks, retry loops, workers or connection ownership are introduced. CPU/memory/GC costs remain at the same operations. Sink transaction/idempotency and checkpoint persistence are unchanged. The test mutates global JVM locale and restores it in finally; parallel test execution or distinct pre-existing FORMAT/DISPLAY defaults require extra care.
1.4 Error Handling and Logging
Unsupported-type handling, catalog exception wrapping and resource closing remain unchanged. No new catch-and-ignore path, success report or credential logging is introduced.
Issue 1: Cover the changed JDBC entry and split paths in the existing integration suite
- Location:
seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/JdbcTypeConverterLocaleTest.java:42; production split change atseatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/source/CollationBasedSplitter.java:144. - Problem: The sole new test calls converter singletons directly. It does not exercise catalog/driver metadata discovery or either string-range splitter, and this PR does not update the existing JDBC IT suite. In particular, direct converter assertions cannot detect missing/overlapping rows from a locale-dependent split-boundary regression.
- Risk: The wider source entry/split behavior can regress while all ten new assertions remain green. This is a coverage gap, not a claim that this head has demonstrated data loss.
- Best improvement: Extend the existing JDBC IT with a focused Turkish-locale source discovery case and a case-insensitive string-range case containing I/i. Assert the complete row identities and multiplicities across splits. Extend the existing
CharsetBasedSplitterTestfor deterministic encoding/decoding if needed; avoid another broad test framework or a database-per-converter matrix. - Root cause / best location: Validation belongs at the existing connector entry/split integration boundary, alongside focused converter checks. The minimal safe fix is that small IT extension; a global locale-testing framework adds scope without being required here.
- Severity: Medium; blocking coverage requirement.
- Previously raised by another reviewer: No; the retrieved review and inline-comment histories were empty.
Issue 2: Preserve locale test isolation
- Location:
seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/JdbcTypeConverterLocaleTest.java:43-79. - Problem: The test changes process-global locale without a resource lock and restores only the base default. If FORMAT/DISPLAY defaults differed before the test, that distinction is lost; parallel locale-sensitive tests could also interfere.
- Risk: Test isolation or environment-dependent failures. Parallel execution is not established for this module, so this is a conditional risk, not an observed CI failure.
- Best improvement / location: Preserve all three locale defaults in this test's cleanup and use the standard JUnit locale resource lock if parallel execution is supported. Local test-fixture isolation is the smallest safe and best-scoped solution; changing production locale would be inappropriate.
- Severity: Medium; non-blocking under the observed test configuration.
- Previously raised by another reviewer: No.
2. Code Quality Assessment
2.1 Coding Standards
Imports and normalization changes follow existing Java conventions. The new test has the ASF header and an explanatory class comment. No new core production methods or nontrivial production fields are introduced. Please use the repository's multiline Javadoc style when touching the test comment; this is non-blocking.
2.2 Test Coverage and Stability
The ten exact type assertions defend the converter defect and the finally cleanup runs even after an assertion failure. They do not replace component/connector IT coverage (Issue 1). Engine checkpoint E2E expansion is unnecessary for this change because the state protocol is unchanged.
Stability rating: Risk present, non-blocking. At JdbcTypeConverterLocaleTest.java:43-79, process-global Locale.setDefault has no resource lock. Current repository searches did not find parallel JUnit execution enabled for this module, so I am not claiming an observed race. If tests are parallelized, use JUnit's locale resource lock; also preserve/restore FORMAT and DISPLAY defaults if they can differ from the base default. There are no sleeps, port allocations, container-start assumptions or asynchronous assertions in this new test.
Local analysis results: git fetch upstream dev refs/pull/13083/head fetched the reviewed object and advanced upstream/dev to b434d2e4f46; git diff upstream/dev...958bf389f7b58b798a3f0f472be43b32e9584f2f showed 39 files, +182/-42. git diff --check passed. Exact-head git show and git grep established the call paths above. No local compilation, unit test, IT, Docker or job execution was performed, as requested. The author's local pass counts are author-reported evidence, not execution by this review.
2.3 Documentation
No new option or setup step is introduced. The locale-independent token interpretation is a bug fix rather than a new configuration contract. If the string-split behavior needs a user-facing compatibility note after its integration case, keep English and Chinese guidance aligned; no unrelated documentation rewrite is requested.
3. Architectural Assessment
3.1 Solution and Best Change Location
This is a targeted normalization fix at consumers of database tokens. That is the appropriate responsibility boundary: changing the process-global locale would affect unrelated jobs and libraries. The smallest safe implementation and the broader correct approach agree on explicit locale selection at each semantic conversion. No shared engine workaround is warranted. Data-derived collation folding remains a separate validation concern, covered by Issue 1.
3.2 Maintainability
Explicit Locale.ROOT makes assumptions visible without adding another abstraction or dependency. Keep the stated scope consistent with both commits; the PR description's statement that sibling WHERE checks were left untouched is superseded by the actual diff and the author's follow-up comment.
3.3 Extensibility
Future dialects can follow the same token-normalization convention without engine or SPI changes. Database collation semantics should continue to be validated against the actual database rather than inferred from the JVM locale.
3.4 Historical Compatibility
No versioned API or checkpoint payload changes. Normalization is intentionally corrected under affected locales; no assertion of live upgrade/recovery acceptance is made.
4. Issue Summary
| Number | Issue | Location | Severity |
|---|---|---|---|
| 1 | Existing JDBC integration coverage does not exercise the changed discovery/split behavior | JdbcTypeConverterLocaleTest.java:42, CollationBasedSplitter.java:144 |
Medium, blocking |
| 2 | Preserve locale test isolation | JdbcTypeConverterLocaleTest.java:43-79 | Medium, non-blocking |
Completeness check: all 39 diff files, source/catalog/dialect and string-split callers, configuration/registration impact, state compatibility, resource/error paths, tests/stability, docs and live CI/merge metadata were considered before this consolidated finding. Issue 2 is non-blocking under the observed execution configuration.
5. Merge Recommendation
Conclusion: Ready to merge after fixes
- Blockers — must be fixed: Issue 1, the focused connector integration coverage. Current-head CI must also execute successfully: the Apache PR signal run is
action_requiredfor this exact SHA, while the head check rollup and fork run query are empty. A maintainer should inspect/approve that workflow gate first. This evidence does not establish that Actions is disabled, and no contributor code patch or upstream-sync experiment is requested to address it. - Recommended fixes — non-blocking: Issue 2: Isolate/fully restore locale state if parallel execution or category-specific defaults are supported; adjust the short class Javadoc and stale scope sentence when updating the tests/description.
Overall assessment: The normalization direction is sound. Please keep the next revision focused on the existing JDBC integration coverage and required workflow gate. There were no other reviewer approvals/change requests in the retrieved history to reconcile. Daniel has READ repository permission; this technical COMMENT is not an effective maintainer approval or dismissal of historical reviews.
Snapshot 2026-11-01T09:55:40.328438+00:00: base dev, head 958bf389f7b58b798a3f0f472be43b32e9584f2f; comparison diverged, ahead 2, behind 2; merge state blocked / BLOCKED, review decision REVIEW_REQUIRED. Behind/diverged is recorded independently and is not a sync request.
|
Thanks for covering the rest of
I merged the heads locally onto current Could you drop these three files from this PR so the PRs stay disjoint, as the description intends? With that, this PR together with #13063, #13076 and #13077 leaves only one bare case conversion in |
Drop MySqlCatalog, OceanBaseMySqlCatalog and SqlServerURLParser: those files are fixed by apache#13076 and apache#13077, and the OceanBase edit conflicts with the removal of the second toUpperCase() there, so leave them to those PRs. Add RedshiftCatalogLocaleTest, which drives RedshiftCatalog.buildColumn with a mocked result set under tr-TR, so catalog schema discovery is covered and not only the converters. Extend CharsetBasedSplitterTest with an I/i case that asserts the case insensitive fold of upper case I is i and that the encoded range decodes back to i. Restore the base, DISPLAY and FORMAT locale defaults and mark the locale mutating tests with @ResourceLock, so they cannot leak state into other tests.
|
Thanks both — separate answers below. Overlap with #13076 / #13077 (@SEPURI-SAI-KRISHNA) — agreed, and dropped in Issue 1 — coverage (@DanielLeens) — added for both paths you named:
What I did not add: the container-backed JDBC integration test with row identities and multiplicities across splits. This machine has no Docker daemon, so I could not run one, and adding an IT I cannot execute here would be unreviewable. If you want that on top, say so and I will add it as a follow-up commit and have CI run it. Issue 2 — locale isolation (@DanielLeens) — done in all three locale-mutating tests: base, Verification (JDK 17, The description was updated: the sibling CI is still |
DanielLeens
left a comment
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks @sharpca for the clear follow-up, and thanks @SEPURI-SAI-KRISHNA for identifying the overlap. I re-read the complete 38-file current diff and the catalog/type/split callers at dfb9ff2cdc24b62418fab0732b206414def5bf40. The overlap and locale isolation are fixed; the existing integration-coverage request remains, with a smaller concrete next step below.
What this PR solves
User pain: database type/identifier tokens and case-insensitive string split encoding depend on the JVM language. Approach: ROOT folding at current catalog/type/keyword/split consumers. One sentence/example: driver metadata integer resolves to INT under Turkish exactly as under English, and case-insensitive I/i string bounds use the same encoding.
1. Code Change Review
1.1 Core Logic Analysis
All production hunks add Locale.ROOT to existing normalizations. The full current set includes seven catalog/builders, the dialect converter/type-mapper families, seven row-count WHERE checks, Postgres default-value type classification, CollationBasedSplitter:144 and JdbcSourceFactory:188. Type switches, metadata fields, identifiers and caller responsibilities remain intact. Before/after:
// before
String type = typeDefine.getDataType().toUpperCase();
String str = isCaseInsensitive ? s.toLowerCase() : s;
// after
String type = typeDefine.getDataType().toUpperCase(Locale.ROOT);
String str = isCaseInsensitive ? s.toLowerCase(Locale.ROOT) : s;Complete affected chains:
JdbcSourceFactory.createSource -> JdbcSource -> JdbcCatalogUtils.getTables
-> table/query lookup -> AbstractJdbcCatalog.buildColumnsReturnTablaSchemaBuilder
-> database-specific buildColumn -> ROOT normalization -> type converter
-> CatalogTable / source row schema -> reader
JdbcDialectTypeMapper.mappingColumn -> driver ResultSetMetaData
-> mapping(typeDefine) / mapper -> existing type switch
FixedChunkSplitter.getJdbcSourceStringSplits:119 or
DynamicChunkSplitter.charsetBasedColumnSplitChunks:312
-> database collation/case-insensitivity and charset
-> CollationBasedSplitter.encodeStringToNumericRange
-> stringToAsciiString:144 ROOT fold -> numeric subdivision -> decode bounds
-> split predicates -> workers read rows
Dialect row-count estimate -> existing WHERE substring decision -> metadata/count path
JdbcSourceFactory WhereConditionValidator -> ROOT prefix check -> config validation
Key findings: normal metadata discovery reaches the fix; the data-derived string-range path is distinct from enum conversion; ROOT does not implement database collation or a SQL grammar; no state/serializer/resource-lifecycle change is made. MySqlCatalog, OceanBaseMySqlCatalog and SqlServerURLParser are absent from this head's diff, resolving the actual overlap with #13076/#13077. The new Redshift test enters real buildColumn with mocked ResultSet; the new charset assertion pins I/i encoding and decoding but does not execute database split predicates.
Evidence: independent review branch seatunnel-review-13083, head dfb9ff2cdc24b62418fab0732b206414def5bf40. Local Git fetch/diff and source/caller searches produced the complete changed-file evidence; I ran no local build, test, container or job.
1.2 Compatibility Impact
Fully compatible for intended existing tokens and unchanged API/config/state contracts. No existing option/default, enum values, signature, protocol or checkpoint shape changes. Fresh string split bounds under affected locales intentionally change to locale-independent values. Coverage of complete row identities/multiplicities remains important; no claim of database collation equivalence is inferred from a JVM string assertion.
1.3 Performance / Side Effects
Same startup/metadata-time string operations and linear complexity; no new worker, thread, lock, retry loop, network operation or per-row allocation. Converters still allocate their prior normalized strings. Range enumeration retains existing subdivision. Sink commits, checkpoint persistence, connection closing and logging ownership are unchanged.
1.4 Error Handling and Logging
Unsupported-type and catalog error handling remain intact; no new logging or credential exposure.
Issue 1 — Medium, blocking, carried over: existing JDBC integration coverage still does not exercise the changed entry/split behavior.
- Location:
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/source/CollationBasedSplitter.java:144, new assertions insrc/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/source/CharsetBasedSplitterTest.java:196andcatalog/redshift/RedshiftCatalogLocaleTest.java:39under the same connector. - Description: the new mocked buildColumn and direct encode/decode assertions close valuable component gaps. They do not run real metadata discovery or database predicates across string-range splits, and no existing JDBC IT is changed. A correct local encoding assertion alone cannot detect gaps/overlap in actual row selection.
- Risk: a result-set completeness regression can pass every added unit assertion. This is a coverage requirement, not observed data loss.
- Best improvement: add one focused case to the existing JDBC IT, with locale applied in the actual discovery/splitting JVM, a case-insensitive supported collation and I/i data. Compare complete row identities and multiplicities across the selected splits. Extend the existing fixture; GitHub CI can execute it even when the contributor has no local Docker daemon. No database-per-converter matrix or new harness is requested.
- Already raised by another reviewer: no; same Daniel Issue 1 from the previous review, not a newly introduced blocker.
Previous Issue 2 is resolved: all three changed locale-mutating classes capture/restore base, DISPLAY and FORMAT and share ResourceLock("java.util.Locale").
2. Code Quality Assessment
2.1 Coding Standards
Explicit imports, focused changes and ASF headers are present. The new class comments use multiline Javadoc. No new core production method or nontrivial production field was introduced. The three overlapping files were actually removed from the diff, rather than only described as excluded.
2.2 Test Coverage and Test Stability
Stability rating: Stable. RedshiftCatalogLocaleTest, JdbcTypeConverterLocaleTest and CharsetBasedSplitterTest use scoped locale mutation, all-default restoration, shared resource lock and deterministic expectations. The catalog test mocks result metadata and does not contact Redshift. Existing charset tests include random data, but the added regression uses fixed I/i input; no new random correctness dependency is introduced. Issue 1 remains the integration boundary. No sleeps, asynchronous waits or port/container assumptions were added. Contributor-reported local test/Spotless results are not execution by this review; no local tests or job were run.
2.3 Documentation Updates
No new user option or setup is introduced. The PR description now matches the broader WHERE-check scope and records ownership by the sibling PRs. A user compatibility note would be warranted if the real collation/split integration demonstrates a behavioral limitation; no unrelated bilingual rewrite is requested.
3. Architectural Soundness
3.1 Elegance
A precise machine-token parsing correction at its consumers; changing the JVM default in production would affect unrelated libraries and is unnecessary.
3.2 Maintainability
Explicit ROOT selection keeps the semantic assumption visible without a new shared framework. Keep type-token normalization and database data-collation validation distinct; the focused existing IT is the smallest useful remaining addition.
3.3 Extensibility
Future converters can use the same explicit locale convention without modifying engine or SPI behavior. No broad parser/normalizer refactor is required.
3.4 Historical-Version Compatibility
Existing API, configuration defaults and serialized job/checkpoint contracts remain. The corrected split-folding behavior must be verified against a real database; static source analysis is not historical upgrade or runtime acceptance.
4. Issue Summary
| ID | Issue | Location | Severity |
|---|---|---|---|
| 1 | Missing real JDBC discovery/string-range IT | CollationBasedSplitter.java:144; CharsetBasedSplitterTest.java:196 | Medium, blocking |
Previous Issue 2 is resolved, not renumbered or repeated.
+1 to @SEPURI-SAI-KRISHNA on keeping overlapping catalog/URL edits in #13076/#13077; the current diff excludes those three files. Thanks @sharpca for adding the useful buildColumn and charset tests. I agree those component scenarios are stronger now, but the explicitly unadded container-backed test still leaves Daniel Issue 1 open. Lack of local Docker need not block authoring a small fixture that repository CI will execute. No separate effective maintainer approval is present.
5. Merge Recommendation
Conclusion: can merge after fixes
- Blockers, must fix: Existing Issue 1: add the focused real JDBC discovery/string-range integration assertion; obtain required current-head CI and effective maintainer review.
- Suggested improvements, nonblocking: No new nonblocking source request. Keep the new unit/component tests and avoid expanding to a database-per-converter matrix.
The previous head's gate snapshot is superseded by this new SHA. Current Apache check rollup and fork run query are empty. The author reports an action_required PR signal; workflow approval is a maintainer action, not evidence of passing Build or disabled fork CI. Branch comparison is diverged, ahead 3 / behind 3 from dev, without reported content conflict. Drift is recorded separately; no unrelated failed-CI/sync cure is established. No empty commit, unrelated workflow patch or base-age-only sync is requested.
The production direction and new component coverage are sound. One small existing-fixture integration extension remains; the resource-isolation and overlap concerns are now closed. Keep the next change scoped to that coverage.
Daniel has READ permission. This technical conclusion does not dismiss historical requested changes or satisfy a write-reviewer gate.
Snapshot 2026-11-01T15:43:34.912721+00:00: head dfb9ff2cdc24b62418fab0732b206414def5bf40, base dev; compare diverged, ahead 3 / behind 3; merge blocked / BLOCKED, review decision REVIEW_REQUIRED. Drift is recorded separately; it does not itself request synchronization.
Add a focused case to JdbcMysqlSplitIT: a case insensitive utf8mb4_general_ci table holding mixed I/i identifiers, discovered and split with the default locale set to tr-TR, then read through the real split statements. Every row must be read exactly once across all splits, so a locale dependent fold that shifts the split boundaries shows up as a missing or duplicated row. The locale is applied before catalog discovery and restored afterwards.
|
Added the focused IT case you asked for, in the existing fixture rather than a new harness. What was added —
What it asserts — more than one split is produced; the set of row identities read across all splits equals the inserted set; and every row is read exactly once. A boundary fold that shifted between locales therefore shows up as a missing row or a duplicate, which is the gap your Issue 1 describes. The split/read API usage deliberately mirrors the neighbouring What I could not do here, stated plainly — I did not compile or run this case. Two separate blockers on this machine:
So this needs CI to compile and run it, as you suggested. If it fails there I will fix it from the CI log rather than guess; the assertions and the data shape are the part I would want reviewed first. Nothing further from me on #13081 — thanks for the clearance there. |
DanielLeens
left a comment
DanielLeens
left a comment
There was a problem hiding this comment.
Thank you @sharpca for adding the integration case so quickly, and for keeping this PR disjoint from the sibling PRs. I re-reviewed the complete current diff at 704af8840297933b6a6775fd7fa035f3cfdaf97b (39 files, including the new commit that only adds JdbcMysqlSplitIT#testFixedStringSplitsReadAllRowsUnderTurkishLocale).
First, a sincere apology and a carryover from me: in my previous round I suggested "an I/i case-insensitive string-range case that compares complete row identities across splits" as the integration check. After tracing the splitter line by line for this round, I now see that this kind of assertion cannot detect the locale bug, because of how the split boundaries are built (details in Issue 1). That suggestion was incomplete on my side, and you followed it faithfully. The fix is small and I describe it below.
What Problem Does This PR Solve?
User pain point: JDBC type/identifier discovery and case-insensitive string split encoding use String.toUpperCase()/toLowerCase() with the JVM default locale. Under Turkish, "integer".toUpperCase() contains a dotted capital I and no longer matches the SQL type constants, and "I".toLowerCase() becomes a dotless i that is not in the collation charset.
Fix approach: use Locale.ROOT at the JDBC type converters/mappers, catalog classes, row-count WHERE checks, where_condition prefix validation and CollationBasedSplitter.
One sentence: the same database metadata gives the same SeaTunnel types and split bounds on every machine, regardless of its language.
1. Code Change Review
1.1 Core Logic Analysis
All production hunks only add Locale.ROOT to existing conversions. I scanned the full production diff: no type switch, option, registration or signature changes.
// before
String str = isCaseInsensitive ? s.toLowerCase() : s;
// after
String str = isCaseInsensitive ? s.toLowerCase(Locale.ROOT) : s;Runtime path of the most behavior-relevant hunk (string-range splitting):
FixedChunkSplitter.createSplits -> getJdbcSourceStringSplits (FixedChunkSplitter.java:~119)
-> MIN/MAX of the partition column (partitionStart / partitionEnd, literal DB values)
-> jdbcDialect.getCollationSequence(connection, collate) -> isCaseInsensitive if sequence has "aA"
-> CollationBasedSplitter.encodeStringToNumericRange -> stringToAsciiString (:144, lower-cases when ci)
-> createNumberColumnSplits (numeric batches over [min,max])
-> decodeNumericRangeToString for interior bounds
-> split statement: id >= ? AND id <= ? -> workers
Under the old code in tr-TR, 'I' lower-cases to U+0131, which is not in the charset, so charToIndex returns 0 (the padding index) and the encoded numeric range (and therefore the interior split bounds) shifts. With Locale.ROOT the encoding is the same on every machine. The other hunks are on normal metadata discovery paths (AbstractJdbcCatalog.buildColumn, JdbcDialectTypeMapper.mappingColumn), so this is a main-path fix, not an exception-path one.
Verified: the files still containing a bare toUpperCase()/toLowerCase() in connector-jdbc/src/main are MySqlCatalog, OceanBaseMySqlCatalog, SqlServerURLParser, MySqlTypeConverter, OceanBaseMySqlTypeConverter, SqlServerTypeConverter, CatalogUtils, JdbcDialect and SqlWhereConditionHelper:553. The first six belong to #13076/#13077 and the next two to #13063, all three still open; the last one is unaffected because where contains no i. So the module is only fully covered once those PRs land as well.
1.2 Compatibility Impact
Fully compatible for config, API, SPI and checkpoint/state shapes (split state classes are untouched). The only intended behavior change: fresh string-split boundaries under affected locales become locale-independent.
1.3 Performance / Side Effects
Same linear string operations, no new allocation pattern, thread, lock or I/O.
1.4 Error Handling and Logging
Unchanged. Numbered issues:
Issue 1: The new IT cannot detect the locale bug it is meant to guard (carryover of my earlier Issue 1)
- Location:
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-7/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlSplitIT.java, the addedtestFixedStringSplitsReadAllRowsUnderTurkishLocale(asserts at the end of the method). - Problem: The test only checks that the union of all splits equals the inserted ids, each exactly once, and that more than one split exists. In
getJdbcSourceStringSplitsthe first split always starts at the literalpartitionStart(MIN) and the last ends at the literalpartitionEnd(MAX), and the interior bounds come from numerically adjacent batches (JdbcNumericBetweenParametersProvider), so the splits stay contiguous whatever the encoding is. Tracing the data in the test (MINI1, MAXi9, max length 2, partition number 4): with the fix the batches are digit ranges inside theiprefix and the interior bounds arei3,i4/i5, ... ; without the fixI1encodes to a first char of index 0, so the numeric range starts near zero, the interior bounds fall on much smaller first characters, and everyi/Irow lands in the last split. All 20 ids are still read once andsplitCountis still 4, so the assertions pass in both cases. This is my inference from reading the code and tracing the batch layout by hand; I did not run it, and neither could the author (no Docker), so the test has not been executed anywhere yet. - Risk: The suite would stay green if someone reverted the
Locale.ROOTinCollationBasedSplitter, so it gives a false sense of coverage for the split path. - Best improvement: Keep the existing fixture, but assert the property that the bug changes: the split bounds. For example, generate the splits once under
Locale.ENGLISH(orLocale.ROOT) and once undertr-TRfrom the same table and assert the two lists of(splitStart, splitEnd)are identical; or assert that every interior bound starts with the same first character as the data (i). That turns red on the old code and green on the new code, and the row-identity check can stay as an extra guard. Note theCharsetBasedSplitterTestunit assertion you added already pins the encoding itself and does fail on the old code, so that part is good. - Severity: Medium (blocking: the integration check is the one thing this PR was asked to add).
- Raised by another reviewer: No.
Issue 2: IT comment/setup implies the table collation drives case-insensitivity (non-blocking)
- Location: same test,
CREATE TABLE ... COLLATE utf8mb4_general_ci. - Problem:
isCaseInsensitiveis derived fromjdbcDialect.getCollationSequence(connection, split.string_split_mode_collate), which sorts printable ASCII with the connection collation (noCOLLATEis applied because the option is unset), not with the table collation. The table collation only affects MIN/MAX and the predicates. - Risk: Misleading to future maintainers; the case-insensitive branch is taken because of the server/connection default.
- Best improvement: Either set
split.string_split_mode_collateto the intended ci collation in the test config so the branch is explicit, or adjust the description. - Severity: Low.
- Raised by another reviewer: No.
2. Code Quality
2.1 Standards
Imports, formatting (no over-long lines) and ASF headers are fine. The tests use multiline Javadoc and the locale state is captured/restored for base, DISPLAY and FORMAT with @ResourceLock("java.util.Locale"), which resolves my earlier isolation point.
2.2 Tests
JdbcTypeConverterLocaleTestandRedshiftCatalogLocaleTest: exact type assertions under tr-TR through the real converter /buildColumn; they fail on the old code. Good.CharsetBasedSplitterTest#testCaseInsensitiveEncodingUnderTurkishLocale: fails on the old code (I folds to dotless i). Good.- IT: see Issue 1.
Stability: Stable for the unit tests (scoped locale, fixed inputs, no timing). The IT is deterministic in data, but it is unexecuted and its pass/fail does not depend on the fix.
CI: the Apache PR signal run for this head is action_required, which needs a maintainer to approve the workflow; the fork (sharpca/seatunnel) shows no run for this head. Neither is a pass or a failure. Please check whether Actions is enabled on your fork. The upstream drift (behind by 3, docs and a Kafka test only) does not touch the JDBC files, so no sync is requested.
2.3 Docs
No new option or setup, no docs change needed.
3. Architectural Soundness
3.1 Fix Type
A precise root-cause fix at each consumer of database tokens; changing the JVM default locale globally would be wrong.
3.2 - 3.4
Maintainable (explicit locale, no new abstraction), no state-format change, no historical compatibility concern.
4. Issue Summary
| # | Issue | Location | Severity |
|---|---|---|---|
| 1 | New IT passes with or without the fix (compares rows, not split bounds) | JdbcMysqlSplitIT#testFixedStringSplitsReadAllRowsUnderTurkishLocale |
Medium, blocking |
| 2 | Table collation does not drive the case-insensitive branch | same test | Low |
5. Merge Recommendation
Conclusion: Ready to merge after fixes
Blockers:
- Issue 1: make the IT assert on the split bounds (en vs tr-TR equality, or interior bounds share the data prefix), then get the required CI executed (maintainer approval of the Apache workflow, or enable Actions on the fork).
Recommended non-blocking fixes:
- Issue 2.
The production change is clean and consistent, and the unit tests are discriminating. Thank you for the careful, well-scoped work.
… test Run each dialect converter as its own parameterized invocation and carry the converter name into the assertion message, so a regression in a single dialect fails naming that converter instead of one combined assertion. Capture the base, DISPLAY and FORMAT defaults before switching to tr-TR and restore them in @AfterEach, which JUnit runs even when the conversion throws, so a Turkish default cannot leak into other tests in the same JVM.
DanielLeens
left a comment
DanielLeens
left a comment
There was a problem hiding this comment.
Thank you @sharpca for the new commit. I re-reviewed the complete current diff at the new head cabfb80dd00f1998b726374cae2aa20a0058b59a (39 files). The only change since my last round (704af8840) is cabfb80dd, which turns JdbcTypeConverterLocaleTest into a parameterized test (one invocation per converter, @BeforeEach/@AfterEach locale handling, a failure message naming the dialect). That is a nice improvement: a regression in one dialect now fails with that dialect's name, and the locale restore runs even when the converter throws.
Sorry again for the carryover: my Issue 1 from the last round (the new IT cannot detect the bug) is still open, because cabfb80dd does not touch the IT. I re-read the IT at this head to confirm, and the trace below still holds.
What Problem Does This PR Solve?
User pain point: JDBC type, catalog and string-split code uses toUpperCase()/toLowerCase() with the JVM default locale. Under Turkish or Azerbaijani, "integer".toUpperCase() yields a dotted capital I and no longer matches the SQL type constants, and "I".toLowerCase() yields a dotless i.
Fix approach: use Locale.ROOT at the remaining connector-jdbc conversion sites (type converters/mappers, catalog classes, row-count WHERE checks, where_condition prefix validation, CollationBasedSplitter).
One-sentence summary: the same database metadata gives the same SeaTunnel types and split bounds on every machine, whatever its language.
1. Code Change Review
1.1 Core Logic Analysis
Production hunks only add Locale.ROOT to existing conversions; I saw no change to type switches, options, registration or signatures.
// before
String str = isCaseInsensitive ? s.toLowerCase() : s;
// after
String str = isCaseInsensitive ? s.toLowerCase(Locale.ROOT) : s;The most behavior-relevant path is string-range splitting, a normal user path (not an exception path):
FixedChunkSplitter.createSplits -> getJdbcSourceStringSplits
-> MIN/MAX of partition column (literal DB values)
-> jdbcDialect.getCollationSequence -> isCaseInsensitive
-> CollationBasedSplitter.encodeStringToNumericRange -> stringToAsciiString (lower-cases when ci)
-> createNumberColumnSplits (numeric batches between encoded min/max)
-> decodeNumericRangeToString for interior bounds
-> split statement: id >= ? AND id <= ? -> workers
Type converters and catalogs are also hit on every metadata discovery (JdbcDialectTypeMapper.mappingColumn, AbstractJdbcCatalog.buildColumn).
Remaining bare toUpperCase()/toLowerCase() in connector-jdbc/src/main belong to the sibling PRs (#13063, #13076, #13077) as the description states; the module is fully covered only after those land.
1.2 Compatibility Impact
Fully compatible. No config, API, SPI, checkpoint or state-shape change. The only intended behavior change: fresh string-split boundaries under affected locales become locale-independent.
1.3 Performance / Side-Effect Analysis
Same linear string operations; no new allocation pattern, thread, lock or I/O.
1.4 Error Handling and Logging
Unchanged.
Issue 1: The IT still cannot detect the locale bug it is meant to guard (carryover, still open)
- Location:
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-7/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlSplitIT.java,testFixedStringSplitsReadAllRowsUnderTurkishLocale(the final assertions). - Problem: the test asserts that the union of all splits equals the inserted ids, each read exactly once, and that
splitCount > 1. IngetJdbcSourceStringSplitsthe first split starts at the literalpartitionStart(MIN), the last ends at the literalpartitionEnd(MAX), and interior bounds come from numerically adjacent batches, so the splits stay contiguous whatever the encoding is. With the bug,Ilower-cases to a dotless i that is not in the charset,charToIndexreturns the padding index, and the interior bounds shift; but every row is still read exactly once and the split count is unchanged, so the assertions pass with or without the fix. This is my inference from reading the code and tracing the batches by hand; I did not run it. - Risk: reverting the
Locale.ROOTinCollationBasedSplitterwould keep the suite green, which gives a false sense of coverage for the split path. - Best improvement: keep the fixture and assert the property the bug changes, the split bounds: generate the splits once under
Locale.ROOT(or ENGLISH) and once undertr-TRfrom the same table and assert the two lists of(splitStart, splitEnd)are identical, or assert that every interior bound starts with the same first character as the data. The row-identity check can stay as an extra guard. TheCharsetBasedSplitterTestcase you added already pins the encoding itself and does fail on the old code. - Severity: Medium (blocking: this IT is the one thing the PR was asked to add in the previous round).
- Raised by another reviewer: No.
Issue 2: Table collation does not drive the case-insensitive branch (non-blocking)
- Location: same test,
CREATE TABLE ... COLLATE utf8mb4_general_ci. - Problem:
isCaseInsensitivecomes fromjdbcDialect.getCollationSequence(connection, split.string_split_mode_collate), which orders ASCII with the connection collation (the option is unset in the test), not the table collation. - Risk: misleading to future maintainers.
- Best improvement: set
split.string_split_mode_collateexplicitly in the test config, or adjust the description. - Severity: Low.
- Raised by another reviewer: No.
Issue 3: IT calls MySqlCatalog.getTable under tr-TR while MySqlCatalog is not yet locale-fixed (Low, non-blocking)
- Location: same test, the
mySqlCatalog.open()/getTable(tablePath)calls made afterLocale.setDefault(tr-TR). - Problem:
MySqlCatalogis intentionally left to #13076. The table only has aVARCHARcolumn, so noiis folded today, but the test's pass/fail would silently depend on that other class if the column type ever changed. - Best improvement: build the catalog table (or switch the default locale) only around the splitter call, or add a one-line comment explaining why this is safe.
- Severity: Low.
- Raised by another reviewer: No.
2. Code Quality Assessment
2.1 Coding Standards
Imports, formatting and ASF headers look fine; the tests use multiline Javadoc and capture/restore base, DISPLAY and FORMAT locales with @ResourceLock("java.util.Locale"). The IT restores locales in a finally, but does not use the ResourceLock; harmless unless parallel execution is enabled.
2.2 Test Coverage and Test Stability
JdbcTypeConverterLocaleTest(now parameterized): exact type assertions under tr-TR for all ten converters; fails on the old code.junit-jupiter-paramsis already used in this module.RedshiftCatalogLocaleTest,CharsetBasedSplitterTest#testCaseInsensitiveEncodingUnderTurkishLocale: discriminating, fail on the old code.- IT: see Issue 1.
- Stability: unit tests Stable; the IT is deterministic in data but, as written, does not depend on the fix (Risk present). It has also not been executed anywhere yet.
2.3 Documentation Updates
No option or setup change; no docs needed.
3. Architectural Soundness
3.1 Fix Type
Precise root-cause fix at each consumer of database tokens (Precise fix); changing the JVM default locale globally would be wrong.
3.2 Maintainability
Explicit locale, no new abstraction.
3.3 Extensibility
Sibling PRs cover the remaining files; keeping the four PRs disjoint is the right call.
3.4 Historical-Version Compatibility
Split state classes are untouched; no compatibility concern.
4. Issue Summary
| ID | Issue | Location | Severity |
|---|---|---|---|
| 1 | New IT passes with or without the fix (compares rows, not split bounds); carryover | JdbcMysqlSplitIT#testFixedStringSplitsReadAllRowsUnderTurkishLocale |
Medium, blocking |
| 2 | Table collation does not drive the case-insensitive branch | same test | Low |
| 3 | IT depends on an unfixed MySqlCatalog under tr-TR |
same test | Low |
5. Merge Recommendation
Conclusion: Ready to merge after fixes
Blockers:
- Issue 1: make the IT assert on the split bounds (en vs tr-TR equality, or interior bounds share the data prefix). Also, CI must run: the Apache-side
PR signalrun for this head (https://github.com/apache/seatunnel/actions/runs/41394473712) isaction_required, which needs a maintainer to approve the workflow, and I could not see a fork Build run for this head. If Actions is not enabled on your fork, please enable it so the IT and unit tests actually execute. No failure has been observed; this is a missing result, not a red one.
Recommended non-blocking fixes: Issues 2 and 3.
The branch is 3 commits behind dev; I traced them as not touching the JDBC files, so no sync is requested. The production change is clean and the unit tests are discriminating. Thank you for the careful, well-scoped work.
…string split IT The IT only compared the rows it read, which cannot observe the locale bug: the first split still starts at the literal MIN and the last one still ends at the literal MAX, so every row is read exactly once whatever the encoding is. Under tr-TR the interior bounds decode to strings that start with the U+0001 placeholder instead of "i" - that is the difference a caller actually sees. Generate the splits once under Locale.ROOT and once under tr-TR and compare the boundaries. Additionally: - pin split.string_split_mode_collate, because isCaseInsensitive is derived from the connection collation (JdbcDialect#getCollationSequence) and, when the option is unset, a server default that does not tie A with a leaves the lower-casing branch untested; - assert that interior bounds are already folded, so the fixture fails loudly if that branch is ever skipped instead of silently going inert; - build the catalog table before switching the locale, so the test does not depend on MySqlCatalog from apache#13076.
|
Thanks @DanielLeens, and thanks for re-reading the IT at the new head instead of assuming the last commit covered it — you are right, cabfb80 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 Your trace is exactly right, and the interesting part is why the row check cannot see the bug. int result = (c == '\u0000') ? 0 : supportedChars.indexOf(c) + 1;Under Issue 2 — the collation is now set explicitly, and it is not cosmetic To catch that failure mode instead of silently going inert, the test also asserts that every interior bound is already folded ( Issue 3 — fixed Scope CI 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 |
DanielLeens
left a comment
DanielLeens
left a comment
There was a problem hiding this comment.
Thank you @sharpca for the very clear write-up and for reworking the IT around the split bounds. I re-reviewed the current head 5dd1edd72c. The only change since my last round (cabfb80dd) is JdbcMysqlSplitIT; all 35 production files are byte-for-byte the same as what I reviewed before, and the PR still does not touch the files owned by #13063, #13076 and #13077.
The direction of the new IT is exactly right, and your explanation of why a row-identity check cannot see the bug (unknown character encoded as the padding index, literal MIN/MAX at both ends) matches my trace. Unfortunately one of the new assertions will fail on the fixed code itself, so the IT is still blocking, for a different reason than last round.
What Problem Does This PR Solve?
- User pain: under a Turkish or Azerbaijani JVM default locale, bare
toUpperCase()/toLowerCase()turnintegerinto a dotted capital I andIinto a dotless i, so JDBC type names stop matching the dialect constants (unsupported-type errors) and case-insensitive string splits compute different bounds. - Fix approach: pass
Locale.ROOTto the remaining conversions inconnector-jdbc(type converters/mappers, catalogs, row-countwherechecks,where_conditionvalidation,CollationBasedSplitter). - Example:
DB2TypeConvertermapsintegertoINTon atr-TRJVM just as onen-US.
1. Code Change Review
1.1 Core Logic Analysis
Production hunks are unchanged and still only add Locale.ROOT. String-split runtime path (normal user path for split.string_split_mode = charset_based):
FixedChunkSplitter.generateSplits -> getJdbcSourceStringSplits
-> findSplitStringColumnRange: SELECT MIN/MAX (literal stored values)
-> getCollationSequence(connection, split.string_split_mode_collate) -> isCaseInsensitive
-> CollationBasedSplitter.encodeStringToNumericRange (lower-cases with Locale.ROOT when ci)
-> createNumberColumnSplits
-> split[0].start = literal MIN, split[last].end = literal MAX,
every other bound = decodeNumericRangeToString(...)
1.2 Compatibility Impact
Fully compatible: no option, API, SPI or state change. On default locales nothing changes; on tr/az locales type mapping and split bounds now match every other locale.
1.3 Performance / Side Effects
None; same string operations.
1.4 Error Handling, Logging and Issues
Previous items:
- Issue 1 (IT could not detect the bug): the new comparison of
rootBoundsagainstturkishBounds(JdbcMysqlSplitIT.java:990-993) does discriminate: on the old code MINI1folds to a dotless i, encodes as index 0, and the interior bounds decode with theU+0001placeholder instead ofi, so the two lists differ. Addressed in intent, but see new Issue 4. - Issue 2 (collation not driving the case-insensitive branch): Fixed,
split.string_split_mode_collate = utf8mb4_general_ciatJdbcMysqlSplitIT.java:933with a comment explaining which collation decides what. - Issue 3 (IT depended on unfixed
MySqlCatalogundertr-TR): Fixed, catalog work moved before the locale switch (JdbcMysqlSplitIT.java:947-956).
Issue 4: the "interior bound is folded" assertion also checks the literal MIN and fails on the fixed code
- Location:
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-7/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlSplitIT.java:994-1004, withsplitBounds(...)atJdbcMysqlSplitIT.java:1023-1035. - Problem: each entry of
rootBoundsis the formatted pairsplitStart + ".." + splitEnd, and the loop checks entries0 .. size-2. Entry 0 starts with the literal MIN fromgetJdbcSourceStringSplits(index == 0 ? partitionStart : decode(...)). With theutf8mb4_general_citable and this fixture, MIN is the stored valueI1(upper case), so entry 0 is"I1..<decoded>", andtoLowerCase(Locale.ROOT)of it is"i1...", which is not equal. The loop only excludes the last entry (whose end is the literal MAXI9), not the first entry's start. - Risk: the IT fails deterministically with the fix in place, so CI will go red for the wrong reason (this is a reading of the code; nothing has executed it yet).
- Best improvement: collect only the decoded interior bounds, for example
for (int i = 0; i < splits.size() - 1; i++) { String bound = splits.get(i).getSplitEnd().toString(); ... }, and assert those are already folded (optionally also that each starts withi, which states the bug even more directly). The list-equality check and the row-identity guard can stay as they are. - Severity: Medium; blocking (the IT would fail on correct code).
- Raised by another reviewer: No.
2. Code Quality
2.1 Coding Standards
The new Javadoc on the IT method and the inline comments for the collation option and the catalog ordering explain the why well. Locale base/DISPLAY/FORMAT are captured and restored in finally.
2.2 Tests and Stability
JdbcTypeConverterLocaleTest,RedshiftCatalogLocaleTest,CharsetBasedSplitterTest#testCaseInsensitiveEncodingUnderTurkishLocale: unchanged, discriminating, Stable.JdbcMysqlSplitIT#testFixedStringSplitBoundsAreLocaleIndependent: data and ordering are deterministic, but it currently contains an assertion that cannot pass with the fix (Issue 4). High risk until that loop is narrowed. CallinggenerateSplitstwice on the same splitter is fine: the split bounds do not depend on any state carried between calls.
2.3 Documentation
No user-facing option or behavior on default locales changes; no docs needed. The updated PR description with the 39-file table (bug vs hardening) is very helpful for reviewers.
3. Architecture
Fixing each consumer of database type names and keywords with Locale.ROOT is the right root-cause fix; changing the JVM default locale would be wrong. Keeping the four sibling PRs disjoint is the right call.
4. Issue Summary
| ID | Issue | Location | Severity | Status |
|---|---|---|---|---|
| 1 | IT compared rows, not split bounds | JdbcMysqlSplitIT.java:990-993 |
Medium | Addressed (see 4) |
| 2 | Collation option not pinned | JdbcMysqlSplitIT.java:933 |
Low | Fixed |
| 3 | IT depended on MySqlCatalog under tr-TR |
JdbcMysqlSplitIT.java:947-956 |
Low | Fixed |
| 4 | Fold assertion includes the literal MIN I1 and fails on the fixed code |
JdbcMysqlSplitIT.java:994-1004 |
Medium | Open, blocking |
5. Merge Recommendation
Conclusion: Ready to merge after fixes
- Blocking: Issue 4 (check only the decoded interior bounds). After that, CI has to actually run: there is still no fork
Buildrun for this head onsharpca/seatunnel, and the Apache-sidePR signalrun isaction_required, so no compile, unit or IT result exists yet. Please enable Actions on the fork and runBuildfor this branch. - Non-blocking: none.
The production change is clean and the unit tests are discriminating; once the IT asserts on the interior bounds only and CI is green, I expect this to be ready. The branch is 8 commits behind dev and still mergeable; the newer dev commits I checked do not overlap the touched production files, so no sync is needed for review.
|
CI is running now that Actions is enabled on the fork: Build / fix/jdbc-type-converter-locale (workflow_dispatch, head 5dd1edd). I will report the result here when it finishes. |
…nds IT The fork build failed at Check code style: google-java-format reflowed the Javadoc paragraph and wanted the splitBounds signature on one line. Replace the Javadoc with line comments (not reflowed) and join the signature.
DanielLeens
left a comment
DanielLeens
left a comment
There was a problem hiding this comment.
Thank you @sharpca for enabling Actions on the fork and for linking the run, that was exactly what was needed. I re-reviewed the current head 24c2d87839. The fork Build has now run on this head (run 41675888169), and it gives us a real answer: the core assertion of the new IT passes, and the IT fails only on the fold check I flagged as Issue 4 last round, with exactly the predicted value (expected: <i1..i3> but was: <I1..i3>, JdbcMysqlSplitIT.java:992, same on JDK 11 and JDK 17). It also settles the open question from your comment: calling generateSplits twice on the same splitter is fine, because assertEquals(rootBounds, turkishBounds) at JdbcMysqlSplitIT.java:986-989 ran before it and passed. So you do not need two splitter instances.
What Problem Does This PR Solve?
- User pain: under a
tr/azJVM default locale, baretoUpperCase()/toLowerCase()produce a dotted capital I or a dotless i, so JDBC type names stop matching the dialect constants (unsupported-type errors) and case-insensitive string splits compute different bounds. - Fix approach: pass
Locale.ROOTto the remaining conversions inconnector-jdbcthat are not owned by #13063 / #13076 / #13077. - Example:
DB2TypeConvertermapsintegertoINTontr-TRexactly as onen-US.
1. Code Change Review
1.1 Core Logic Analysis
Since my last review (5dd1edd72c) the only change is the spotless commit 24c2d87839 on JdbcMysqlSplitIT: the method Javadoc became // line comments and the splitBounds(...) signature was rewrapped. All 35 production files are unchanged and still only add Locale.ROOT; the PR still does not touch the files owned by the sibling PRs.
1.2 Compatibility Impact
Fully compatible: no option, API, SPI, default or state change. Default locales behave as before.
1.3 Performance / Side Effects
None.
1.4 Error Handling, Logging and Issues
Previous items, checked against 24c2d87839:
- Issue 1 (IT compared rows, not bounds): Fixed, and now CI-proven. The
rootBoundsvsturkishBoundscomparison passed on the fixed code. - Issue 2 (collation not pinned): Fixed (
JdbcMysqlSplitIT.java:929). - Issue 3 (IT depended on
MySqlCatalogundertr-TR): Fixed (JdbcMysqlSplitIT.java:943-952run before the locale switch). - Issue 4: Still open, now confirmed by CI.
Issue 4: the fold assertion includes the literal MIN of the first split and fails on the fixed code
-
Location:
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-7/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlSplitIT.java:990-999, withsplitBounds(...)atJdbcMysqlSplitIT.java:1024-1030. -
Problem: each entry is
splitStart + ".." + splitEnd, and entry 0 starts with the literal stored MIN (FixedChunkSplitter.java:178-179,index == 0 ? partitionStart : decode(...)), which isI1."I1..i3".toLowerCase(Locale.ROOT)is"i1..i3", so the check fails even though the decoded boundi3is correct. CI shows exactly this failure in bothupdated-modules-integration-test-part-8jobs. -
Risk: the IT is deterministically red on correct code, so the PR cannot go green.
-
Best improvement: check only the decoded interior bounds, e.g.
List<JdbcSourceSplit> rootSplits = new ArrayList<>(splitter.generateSplits(jdbcSourceTable)); // ... keep rootBounds = splitBounds(rootSplits) for the equality check for (int i = 0; i < rootSplits.size() - 1; i++) { String bound = rootSplits.get(i).getSplitEnd().toString(); Assertions.assertEquals(bound.toLowerCase(Locale.ROOT), bound, "..."); }
Every split end except the last one is decoded, so this covers all interior bounds without touching the literal MIN/MAX. The list-equality check and the row-identity guard stay as they are.
-
Severity: Medium; blocking.
-
Raised by another reviewer: No.
2. Code Quality
2.1 Coding Standards
Minor, non-blocking: spotless does not require dropping the Javadoc on the IT method; a standard multi-line /** ... */ block (without the {@code ...} that wrapped awkwardly) would keep the method-level documentation style used elsewhere in the class. Fine to leave as is if you prefer.
2.2 Tests and Stability
JdbcTypeConverterLocaleTest,RedshiftCatalogLocaleTest,CharsetBasedSplitterTest#testCaseInsensitiveEncodingUnderTurkishLocale: unchanged, discriminating, Stable (the unit-test jobs that ran this module passed).JdbcMysqlSplitIT#testFixedStringSplitBoundsAreLocaleIndependent(JdbcMysqlSplitIT.java:889-1017): High risk until Issue 4 is fixed; deterministic failure, not a flake. After the fix, data, collation and ordering are deterministic, so I expect it to be stable.
2.3 Documentation
No user-facing option or default changes; no docs needed.
3. Architecture
Fixing each consumer with Locale.ROOT remains the right root-cause fix, and keeping the four sibling PRs disjoint is the right call.
4. Issue Summary
| ID | Issue | Location | Severity | Status |
|---|---|---|---|---|
| 1 | IT compared rows, not split bounds | JdbcMysqlSplitIT.java:986-989 |
Medium | Fixed (CI-proven) |
| 2 | Collation option not pinned | JdbcMysqlSplitIT.java:929 |
Low | Fixed |
| 3 | IT depended on MySqlCatalog under tr-TR |
JdbcMysqlSplitIT.java:943-952 |
Low | Fixed |
| 4 | Fold assertion includes literal MIN I1 and fails on the fixed code |
JdbcMysqlSplitIT.java:990-999 |
Medium | Open, blocking, confirmed by CI |
| 5 | IT method Javadoc turned into line comments | JdbcMysqlSplitIT.java:878-888 |
Low | Optional |
5. Merge Recommendation
Conclusion: Ready to merge after fixes
- Blocking: Issue 4. CI on
24c2d87839is red because of it. - CI notes for the next run: the other failures in run 41675888169 are not caused by this PR:
Redis7Test.failedInitializationClosesConnectionsinunit-test (17, ubuntu-latest)(this PR touches no Redis code; the same job passed on JDK 11 and on Windows) and aMetalakeIT.startUpcontainer timeout in the JDK 11 part-8 job. If they recur after your fix, a rerun of the failed jobs should be enough. - Non-blocking: Issue 5.
The production change is clean and the important IT assertion already passes on real MySQL, so this is one small test change away. The branch is 8 commits behind dev and still mergeable; no sync is needed for the review.
… folded The guard iterated over the start..end strings, but the first split's start is the literal partitionStart (MIN), which keeps its uppercase I. CI failed with expected i1..i3 but was I1..i3. Walk the splits instead: start of every split except the first, end of every split except the last.
|
The fork build finally ran on 24c2d87 and it caught a real bug in the new IT, which is worth quoting because it validates the assertion rather than the fix: That was my guard, not the splitter. The first split's start is the literal partitionStart (MIN = I1), which legitimately keeps its uppercase I, and I had required every element of the bounds list to be folded. Fixed in de80d16: the test now walks the splits and checks only the decoded interior bounds - start of every split except the first, end of every split except the last. Two other failures in the same run are unrelated to this change: \Redis7Test.failedInitializationClosesConnections\ in connector-redis unit tests, and \MetalakeIT.startUp\ timing out in the same IT shard. I have not touched either module. Note on coverage, correcting something I assumed earlier: the \jdbc-connectors-it-part-7\ job is skipped by backend.yml unless api/engine changed, but the changed-module shard \updated-modules-integration-test-part-8\ did run \JdbcMysqlSplitIT, so the new IT is exercised after all. A new run is in progress on de80d16: Build. I will report the result here. |
DanielLeens
left a comment
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks @sharpca, that was quick, and thanks for quoting the failing assertion: it confirms the IT was checking the right thing and only the fixture boundary was wrong. I re-checked the full PR on the new head de80d166dc58bcfc974106645c29e13521945725.
What Problem Does This PR Solve?
Unchanged from my previous review: JDBC dialect type converters and the string split path used locale-sensitive toUpperCase() / toLowerCase(), so under a Turkish default locale (i -> İ) type names and decoded split bounds could differ from the root locale. The production change is unchanged since my last review; the only new commit (de80d166) touches JdbcMysqlSplitIT.java.
1. Code Change Review
1.1 Core Logic Analysis
The new commit changes only the folded-bound assertion in testFixedStringSplitBoundsAreLocaleIndependent:
// before: every entry of rootBounds except the last, which includes split 0's start (the stored MIN value "I1")
for (int i = 0; i < rootBounds.size() - 1; i++) { ... assertEquals(bound.toLowerCase(Locale.ROOT), bound) }
// after: only generated interior bounds
for (int i = 0; i < rootSplits.size(); i++) {
if (i > 0) assertFoldedBound(rootSplits.get(i).getSplitStart());
if (i < rootSplits.size() - 1) assertFoldedBound(rootSplits.get(i).getSplitEnd());
}Key findings:
- My previous Issue 4 is fixed. The first split's start and the last split's end are the real MIN/MAX values read from the table, so they keep their stored case; only the bounds produced by the case-insensitive charset sequence must be folded, and that is exactly what the loop now asserts (
JdbcMysqlSplitIT.java:992-999). - The main locale check (root-locale bounds equal Turkish-locale bounds,
JdbcMysqlSplitIT.java:988-991) is unchanged and still the assertion that turns red if theLocale.ROOTfix is reverted. assertFoldedBoundalso asserts the generated bound is not null (JdbcMysqlSplitIT.java:1032-1039), which is a sensible strengthening rather than a weakening.- The splitter is now called once for the root locale and its splits are reused (
JdbcMysqlSplitIT.java:964-965), so the bound list and the folded check look at the same splits.
1.2 Compatibility Impact
Fully compatible. Test-only change in this round; no API, option, default or serialization change.
1.3 Performance / Side-Effect Analysis
None; test code only.
1.4 Error Handling and Logging
No change. No new issues.
2. Code Quality Assessment
2.1 Coding Standards
Fine. The helper is small and its message explains the expectation.
2.2 Test Coverage and Test Stability
Stability: Stable. The test still saves and restores both default-locale categories in finally, uses a fixed fixture, and has no sleeps or ordering assumptions.
2.3 Documentation Updates
No doc change needed.
3. Architectural Soundness
Precise fix of the fixture assertion; the earlier production design assessment stands.
4. Issue Summary
| # | Issue | Location | Severity |
|---|---|---|---|
| 4 (previous) | Folded-bound check covered the stored MIN value | JdbcMysqlSplitIT.java:992-999 |
Fixed |
No new issues.
5. Merge Recommendation
Conclusion: Ready to merge after fixes
- Blockers - must be fixed: no source blocker remains. The only remaining gate is CI on this head: the fork Build is still in progress, and the apache-side
Buildcheck has not attached yet. - Recommended fixes - non-blocking: the optional nit from my previous review (method Javadoc turned into line comments) still applies, feel free to leave it.
I will update to approve once the fork Build on this head is green. Nice work tightening this up.
|
Update on the fork build for de80d16: the new IT passes now. The shard log shows and the module summary for connector-jdbc-e2e-part-7 is \Tests run: 110, Failures: 0, Errors: 1. The single error is not from this change: That is a container-startup flake in a test this PR does not touch. \doris-connector-it\ also failed in its own module (step I have re-run the failed jobs of run 41730993933 and will report the result here. |
|
Two short updates so you are not waiting on me silently. Issue 5: leaving the CI on de80d16: the failed-job rerun did not start. The fork is currently rejecting workflow dispatches with For the record, the evidence for the fix is already in that run before it wedged: the part-8 log shows and the single error is the MetalakeIT container timeout you identified as unrelated. So the fold assertion is green on real MySQL; the only red left in that run is infrastructure. |
|
Thanks @sharpca for the clear updates, and no worries at all about the wait.
Lowest-cost next step once Actions is enabled on your fork again: rather than rerunning the wedged run, start a fresh Separately, the apache-side As soon as a fork |
|
Fork Actions had been disabled at the repository level; it is enabled again and I dispatched the workflow, so the PR check now reports the fork run for the current head.
One honest gap: |
|
Correction to my correction: the logs say the e2e test did not run, so my first note was right and the retraction above was wrong. Verified from this run's own logs:
Residual uncertainty, stated plainly: I could not scan the other seven updated-modules shards. Their logs are ~12 MB each and the storage endpoint ignores So the e2e case remains unexecuted in CI. The failure analysis is unchanged: the only failing job in this run is |
|
Status on the CI gate, with the pattern across four runs on these two heads. Every failure so far is a container/network failure in a module this PR does not touch, and none of them is the assertion this PR adds.
The positive side is unchanged and is the part that matters for review: on de80d16 the shard that runs this PR's IT reports with the single error being I am not going to keep re-running until a lucky green: each attempt costs 5-6 hours and lands on a different flaky container test. If you are willing to weigh the code-level evidence above, the PR is one merge away; if you would rather have a green run on the head, tell me and I will keep retrying, but I would be spending your CI minutes on infrastructure, not on this change. |
|
Thanks @sharpca for the table, that makes the pattern easy to follow, and sorry these runs keep landing on different containers. I looked at the newest failure myself. In attempt 1 of run 41880443702 on On your question: I agree you should not keep doing full 5-6 hour runs. Rerunning only the failed job is the cheapest step, and attempt 2 of the same run (started 23:27Z, only the part-7 shard is running) is already doing that, so nothing more is needed from you right now. If it passes, that run is green on this exact head and I will update my review to approve straight away. If it hits yet another unrelated container, please just rerun that one job again; there is no need for a new dispatch or an empty commit. About merging on code-level evidence alone: I only have read access here, so that call belongs to the committers. My view stays the same: the source is ready, and the |
|
Green run on It went green by re-running only the previously failed job instead of the whole matrix: attempt 1 failed on The apache-side |
DanielLeens
left a comment
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks @sharpca for getting a green run on de80d166 by rerunning only the failed job, and for the clear failure table along the way. As promised, I re-checked the PR on the current head b3209869b875066bb20e88ff6b54aa789838db1f. The only commit since my last review is b3209869, which merges dev into the branch. Its fork Build is also green. I agree with your summary: every red run on de80d166 (MetalakeIT, the Doris IT step, TiDBCDCIT, JdbcHanaIT container exit code 3, BindException, Hive metastore refused) was infrastructure, and none of them reached this PR's assertions.
What Problem Does This PR Solve?
Unchanged: the JDBC dialect type converters, catalogs, a few dialect keyword checks and the string split path used locale-sensitive toUpperCase() / toLowerCase(). Under a Turkish default locale (i -> İ), type names no longer matched the SQL type constants, and decoded split bounds could differ from the root locale. The PR pins those conversions to Locale.ROOT.
1. Code Change Review
1.1 Core Logic Analysis
What I checked for this re-review:
- I compared the PR's own changes (each of the 39 PR files, diffed against its merge base) at
de80d166and atb3209869. Every added and removed line is identical, so the dev merge changed no production or test code in this PR. - Between the old base (
fcb404cd) and the new base (a3743d7),devchanged nothing underconnector-jdbc/src/main. So the merge brought in no new dialect or converter that would still need the sameLocale.ROOTtreatment. - The core change is still the mechanical
Locale.ROOTpinning, for example the split pathCollationBasedSplitter(String str = isCaseInsensitive ? s.toLowerCase(Locale.ROOT) : s;) and the converterswitchinputs such asDB2TypeConverterandOracleTypeConverter. Normal schema discovery and string-key splitting both hit these sites, so this fixes the main path, not just an edge case.
Status of my earlier issues:
| ID | Issue | Status |
|---|---|---|
| 1 | IT compared rows, not split bounds | Fixed |
| 2 | Collation option not pinned | Fixed |
| 3 | IT depended on MySqlCatalog under tr-TR |
Fixed |
| 4 | Folded-bound check covered the stored MIN value | Fixed in de80d166 (JdbcMysqlSplitIT.java:992-999) |
| 5 | Method Javadoc turned into line comments | Optional; we agreed to leave it |
1.2 Compatibility Impact
Fully compatible. Under the default (root-like) locales the output is identical. No option, default, API, SPI or serialization change.
1.3 Performance / Side-Effect Analysis
None. Passing a Locale argument to the same calls costs nothing extra.
1.4 Error Handling and Logging
No change. No new issues.
2. Code Quality Assessment
2.1 Coding Standards
Consistent, minimal edits; no new dependency.
2.2 Test Coverage and Test Stability
JdbcTypeConverterLocaleTest,RedshiftCatalogLocaleTestandCharsetBasedSplitterTestrun under a Turkish default locale and restore it infinally.JdbcMysqlSplitIT#testFixedStringSplitBoundsAreLocaleIndependentcompares root-locale bounds with Turkish-locale bounds on real MySQL (JdbcMysqlSplitIT.java:988-991). It turns red if the split fix is reverted, and it ran successfully in fork run 41730993933 onde80d166.- Stability: Stable. The tests use fixed fixtures and no sleeps, and they save and restore the locale.
2.3 Documentation Updates
None needed; there is no user-visible option change.
3. Architectural Soundness
A precise fix at the normalization sites. Each converter keeps its own switch, and nothing leaks into shared engine or API code.
4. Issue Summary
| ID | Issue | Location | Severity |
|---|---|---|---|
| - | No open issues | - | - |
5. Merge Recommendation
Conclusion: Ready to merge
CI: the fork Build on the current head passed (https://github.com/sharpca/seatunnel/actions/runs/41956400925), and the previous head passed in https://github.com/sharpca/seatunnel/actions/runs/41880443702 (attempt 2, 78 jobs, 0 failures). The Apache-side PR signal run for this head (https://github.com/apache/seatunnel/actions/runs/41956404239) is action_required; a committer with write access needs to approve that workflow, and nothing is needed from you.
Merge state: MERGEABLE, 1 commit behind dev (an unrelated CLI change). The remaining gate is a write-capable maintainer review.
Blockers: none. Non-blocking: the optional Issue 5 only.
Daniel has READ permission on the repository, so this approval records my technical conclusion; a committer still owns the effective approval and the merge. Thank you for the patience and the careful evidence throughout.
|
Gentle ping for a write-capable review, since this has been green and technically approved for a while and nothing is pending on the author side. State on The only remaining gate is a committer review and a merge through the |
Purpose of this pull request
Fixes #13079.
The JDBC dialect type converters convert the source type name with
String#toUpperCase()/String#toLowerCase()without a locale before matching it against the dialect type constants. Under a Turkish or Azerbaijani default locale a lower-caseintegerupper-cases toINTEGERwith a dotted capital I, and an upper-caseINT4lower-cases toint4with a dotless i, so the type name no longer matches any constant and the converter falls through to its unsupported-type branch.This PR passes
Locale.ROOTto the case conversions thatconnector-jdbcstill performs with the JVM default locale.The ten converters listed in the issue:
DB2TypeConverter,DmdbTypeConverter,IrisTypeConverter,KingbaseTypeConverter,OracleTypeConverterPostgresTypeConverter,RedshiftTypeConverter,SapHanaTypeConverter,XuguTypeConverter,YashanDbTypeConverterOther conversions found while scanning the rest of the connector, which have the same defect:
Gbase8aTypeMapper,HiveTypeMapper,PhoenixTypeConverter,PrestoTypeMapper,SnowflakeTypeConverter,SqliteTypeMapper,TablestoreTypeMapper,TeradataTypeMapper,VerticaTypeMapper, and the column type lookup inPostgresDialect.DamengDataTypeConvertor,DuckDBCatalog,OceanBaseOracleCreateTableSqlBuilder,RedshiftCatalog,RedshiftDataTypeConvertor,XuguCatalog,YashanDbCatalog.WHEREcheck inMysqlDialect,OceanBaseMysqlDialect,OracleDialect,RedshiftDialect,SqlServerDialectandYashanDbDialect(PostgresDialectwas covered with the type lookup), thewhere_conditionprefix validation inJdbcSourceFactory, and the case insensitive fold inCollationBasedSplitter, where the locale changes the computed split boundary rather than only the spelling of a constant.Left to #13063, #13076 and #13077, and therefore not part of this PR:
JdbcDialect,CatalogUtils,MySqlTypeConverter,OceanBaseMySqlTypeConverter,SqlServerTypeConverter,MySqlCatalog,OceanBaseMySqlCatalogandSqlServerURLParser. Keeping those files in the owning PRs is what keeps the four PRs disjoint and avoids theOceanBaseMySqlCatalogconflict, where #13076 removes the secondtoUpperCase()instead of adding a locale to it.Does this PR introduce any user-facing change?
No behaviour change on the default locale. On a Turkish or Azerbaijani JVM the converters now map the same source type to the same SeaTunnel type as on any other locale, instead of failing with an unsupported-type error. String range splitting now computes the same boundaries on every locale for the same data.
How was this patch tested?
Added or extended:
JdbcTypeConverterLocaleTestis a parameterized test with one invocation per converter, so a failure names the dialect rather than an index. It switches the default locale totr-TRin@BeforeEach(base,DISPLAYandFORMATcaptured first) and restores all three in@AfterEach, and it converts a lower-caseinteger/int(and an upper-caseINT4for PostgreSQL) through the ten converters from the issue.RedshiftCatalogLocaleTestdrivesRedshiftCatalog#buildColumn(ResultSet)with a mocked result set undertr-TR, so catalog schema discovery is covered and not only the converter singletons.CharsetBasedSplitterTest#testCaseInsensitiveEncodingUnderTurkishLocaleasserts that an upper-caseIfolds toi(not the dotlessı), encodes to the same range asi, and decodes back toi.All three restore the base,
DISPLAYandFORMATlocale defaults and carry@ResourceLock("java.util.Locale").Result: 68 tests, 0 failures, 1 skipped (the container backed Oracle test).
spotless:checkpasses.Check list
New License Guide
incompatible-changes.mdto describe the incompatibility caused by this PR.PR #13083 scope: every touched file, its call site and why it is locale sensitive
Generated from the PR's own diff: 39 files (35
src/main, 4src/test), 464 added / 38 removed lines.Class column: bug = an
i/Iis folded differently under tr-TR/az-AZ today, hardening = same conversion but the token has noi, so no behaviour change (kept for consistency), mixed = both in one file, test = test only.Counts: 27 bug, 1 mixed, 7 hardening, 4 test-only.
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/dm/DamengDataTypeConvertor.javatoSeaTunnelType,switch (dataType.toUpperCase(Locale.ROOT))decimal,number,int,bigintcontains an i; under tr-TR it upper-cases toİ(U+0130), no case matches, so precision and scale are no longer applied.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/duckdb/DuckDBCatalog.javaisDuckDBDecimal,typeName.toUpperCase(Locale.ROOT).startsWith(DUCKDB_DECIMAL)decimalcontains an i; under tr-TR the prefix check fails and the DuckDBDECIMALis not widened.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/oceanbase/OceanBaseOracleCreateTableSqlBuilder.javabuildColumnSql,switch (column.getSourceType().toUpperCase(Locale.ROOT))INTERVAL DAY(2) TO SECOND(6),BINARY_FLOAT,TIMESTAMP) contain an i or an I, so the unsupported-type rewrite is skipped under tr-TR.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/redshift/RedshiftCatalog.javagetTableName(toLowerCase(Locale.ROOT)) andbuildColumn(getString("DATA_TYPE").toUpperCase(Locale.ROOT))DATA_TYPEvalues such asinteger/bigintcontain an i: tr-TR givesınteger, so the column falls through to the default branch.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/redshift/RedshiftDataTypeConvertor.javatoSeaTunnelType,switch (connectorDataType.toUpperCase(Locale.ROOT))integer/bigintfolding on the catalog type path.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/xugu/XuguCatalog.javadatabaseExists,databaseName.toUpperCase(Locale.ROOT)in the condition SQL and inlistDatabases().contains(...)seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/yashandb/YashanDbCatalog.javabuildColumn,getString("DATA_TYPE").toUpperCase(Locale.ROOT).replaceAll("\\(\\d+\\)", "")INTEGER/TIMESTAMP(6)contain an I; tr-TR yieldsıntegerand the type is unrecognized.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/db2/DB2TypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)integer,bigint,varchar,timestampcontain an i; tr-TR folds it toİand the switch falls to the default branch.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/dm/DmdbTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)startsWith("TIMESTAMP(")probe, which fails under tr-TR.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/gbase8a/Gbase8aTypeMapper.javamapping,metadata.getColumnTypeName(colIndex).toUpperCase(Locale.ROOT)integer,bigint) contain an i.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/hive/HiveTypeMapper.javamapping,metadata.getColumnTypeName(colIndex).toUpperCase(Locale.ROOT)int,bigint,array,timestamp) contain an i.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/iris/IrisTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)integer/bigintfolding on the type path.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/kingbase/KingbaseTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)int,bigint,varchar,timestamp.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/mysql/MysqlDialect.javaapproximateRowCntStatement,table.getQuery().toLowerCase(Locale.ROOT).contains("where")wherehas no i, so the folded text still contains it. Changed for consistency with the other dialects, not to fix a failure.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/oceanbase/OceanBaseMysqlDialect.javaapproximateRowCntStatement,table.getQuery().toLowerCase(Locale.ROOT).contains("where")wherehas no i); consistency only.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/oracle/OracleDialect.javaapproximateRowCntStatement,query.toLowerCase(Locale.ROOT).contains("where")wherehas no i); consistency only.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/oracle/OracleTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)integer,interval,timestampcontain an i; also feeds thestartsWith(ORACLE_INTERVAL)probe.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/phoenix/PhoenixTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)integer/varcharfolding on the type path.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/presto/PrestoTypeMapper.javamapping,metadata.getColumnTypeName(colIndex).toUpperCase(Locale.ROOT)integer,bigint,varchar) contain an i.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/psql/PostgresDialect.javaconvertType/convert,columnDefine.getDataType().toLowerCase(Locale.ROOT); plus thecontains("where")checkINT4,INT8,TIMESTAMPlower-case toınt4under tr-TR), while thewherecheck is consistency only.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/psql/PostgresTypeConverter.javaconvert,typeDefine.getDataType().toLowerCase(Locale.ROOT)INT4/INT8/TIMESTAMPfold toınt4under tr-TR and stop matching the PG constants.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/redshift/RedshiftDialect.javaapproximateRowCntStatement,table.getQuery().toLowerCase(Locale.ROOT).contains("where")wherehas no i); consistency only.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/redshift/RedshiftTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)integer/bigintfolding on the type path.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/saphana/SapHanaTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)integer,tinyint,varcharcontain an i.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/snowflake/SnowflakeTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)integer,timestamp_variantetc. contain an i.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/sqlite/SqliteTypeMapper.javamapping,metadata.getColumnTypeName(colIndex).toUpperCase(Locale.ROOT).trim()integer,boolean) contain an i.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/sqlserver/SqlServerDialect.javaapproximateRowCntStatement,table.getQuery().toLowerCase(Locale.ROOT).contains("where")wherehas no i); consistency only.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/tablestore/TablestoreTypeMapper.javamapping,metadata.getColumnTypeName(colIndex).toUpperCase(Locale.ROOT)seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/teradata/TeradataTypeMapper.javamapping,metadata.getColumnTypeName(colIndex).toUpperCase(Locale.ROOT)integer,bigintcontain an i.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/vertica/VerticaTypeMapper.javamapping,metadata.getColumnTypeName(colIndex).toUpperCase(Locale.ROOT)integer,intervalcontain an i.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/xugu/XuguTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)integer/bigintfolding on the type path.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/yashandb/YashanDbDialect.javaapproximateRowCntStatement,query.toLowerCase(Locale.ROOT).contains("where")wherehas no i); consistency only.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/yashandb/YashanDbTypeConverter.javaconvert,typeDefine.getDataType().toUpperCase(Locale.ROOT)integer,timestamp,binary_floatcontain an i.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/source/CollationBasedSplitter.javastringToAsciiString,isCaseInsensitive ? s.toLowerCase(Locale.ROOT) : sIbecomesıunder tr-TR, which is not in the collation sequence and encodes as index 0 (the padding index), so the generated split bounds change.seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/source/JdbcSourceFactory.javawhere_conditionvalidator,value.toLowerCase(Locale.ROOT).startsWith("where")wherehas no i); consistency only.seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/redshift/RedshiftCatalogLocaleTest.javatestBuildColumnUnderTurkishLocaleRedshiftCatalog#buildColumnwith a mockedResultSetunder tr-TR and assertsINT/LONG; fails on the old code.seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/JdbcTypeConverterLocaleTest.javaSeaTunnelDataType; fails on the old code and names the dialect in the failure message.seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/source/CharsetBasedSplitterTest.javatestCaseInsensitiveEncodingUnderTurkishLocaleIencodes likei(and not like the dotlessı) under tr-TR; fails on the old code.seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-7/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcMysqlSplitIT.javatestFixedStringSplitBoundsAreLocaleIndependentLocale.ROOTand under tr-TR and keeps the row identity check as an extra guard.The 39 rows above are the complete diff of this branch (
git diff --name-onlylists exactly these 39 files); paths are repository-root relative so each row can be matched against the diff directly.