SEZ9 commented on issue #13079: URL: https://github.com/apache/seatunnel/issues/13079#issuecomment-6567444007
Thanks @sharpca for opening the PR rather than just confirming the analysis. Passing `Locale.ROOT` to every `toUpperCase()` / `toLowerCase()` in the ten converters from the report is exactly the fix I was after, and `JdbcTypeConverterLocaleTest` running under `tr-TR` with lower-case `integer` / `int` plus upper-case `INT4` for PostgreSQL is the right shape of guard. That it fails against the unpatched `DB2TypeConverter` with `Unsupported type integer` is good evidence the test actually bites. Two small asks on the test: 1. You report it as 1 test covering ten converters. Could you split it per converter (parameterized over the converter instances, or one method each)? If a regression slips into a single dialect, the failure should name that converter directly. 2. Please confirm the previous default locale is captured before it is switched to `tr-TR` and restored even if the converter call throws. You describe it as restored, so this is likely already fine; a leaked `tr-TR` default would make unrelated tests in the same JVM fail in confusing ways. On the scan results: the six `getQuery().toLowerCase()` dialect sites, `CollationBasedSplitter`, and the `JdbcSourceFactory` `where_condition` check are the same class of bug, so bundling them with the converter fix is fine with me, and keeping `JdbcDialect` / `CatalogUtils` and the MySQL / OceanBase / SQL Server converters in their separate PRs as you describe is the right call. Once the PR workflows have run, ping me here and I'll do the full review. <!-- streview-comment:2204 --> -- 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]
