nzw921rx commented on PR #12996: URL: https://github.com/apache/seatunnel/pull/12996#issuecomment-6476035490
## Review and local validation Reviewed head `b205f49ea59742d0c61e80354316a375c92a04e3`. **LGTM. No blocking issue found within the reviewed and tested scope.** The fix is appropriately placed in `SqlWhereConditionHelper`: both split reads and split-metadata queries use this shared helper. Applying `Locale.ROOT` at the keyword comparisons covers both paths while preserving the original SQL text, literals and identifiers. ### Local verification | Check | Result | | --- | --- | | Clean source build of JDBC and its reactor dependencies, 9 modules | Passed | | Unmodified source-related tests across 7 classes | 163 passed | | Same scope with 5 additional local regression methods | 168 passed | | Enhanced helper tests in separate JVMs started with `en-US`, `tr-TR`, `az-AZ` and `lt-LT` | 124 passed in each JVM | | Spotless and diff whitespace checks | Passed | The additional coverage verifies: - Both the real `ChunkSplitter` read-filter and metadata-filter entry points. - Preservation of Unicode literals and original SQL text. - Quoted keywords and nested clauses remaining outside the top-level guards. - Restoration of independently configured base, FORMAT and DISPLAY locale defaults. - Generated SQL executed through the module’s SQLite JDBC driver against an actual in-memory database, including Unicode values and case-sensitive filtering. ### Negative controls I separately compiled the parent commit’s helper and ran the same tests against it: - Original PR test class: **118 passed, 2 failed**, exactly the two locale regressions added by this PR. - Enhanced `DynamicChunkSplitterTest`: **12 passed, 1 failed**, specifically the additional locale regression covering both read and metadata paths. - With the PR helper, those results are **120/120** and **13/13**, respectively. Production sources remained unchanged during validation. The additional tests were local review extensions and were not pushed. Validation ran on Linux with JDK 17. I did not run the full JDBC test suite or Docker/Engine E2E tests. The Build check for the reviewed head was also successful when checked. -- 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]
