SEPURI-SAI-KRISHNA opened a new pull request, #12495:
URL: https://github.com/apache/seatunnel/pull/12495
### Purpose of this pull request
The `Sql` transform resolves function names, `CAST` target types, datetime
field names, `VECTOR_REDUCE` methods and the `engine` option value by
uppercasing the user's text with `String.toUpperCase()`, which uses the JVM
default locale. On a worker whose default locale is Turkish or Azeri, lowercase
`i` uppercases to `İ` (U+0130) rather than `I`, so a keyword spelled in lower
or mixed case no longer matches the constant it is compared against and the job
fails.
I reproduced this against `dev` at c3f06f9a8 with the production code
untouched, by pinning only the default locale:
| default locale | `select sign(age), cast(age as int), current_timestamp` |
| --- | --- |
| `en` | passes |
| `tr-TR` | `COMMON-05 Unsupported operation: Unsupported function: sign` |
and for the `engine` option, `engine = "internal"` under `tr-TR`:
```
IllegalArgumentException: No enum constant
org.apache.seatunnel.transform.sql.SQLEngineFactory.EngineType.İNTERNAL
```
The query, the config and the schema are identical in both rows. Only the
worker's locale differs.
To be precise about the blast radius, rather than assert it: an **all
uppercase** spelling is safe. I checked every keyword these switches compare
against (156 distinct values, extracted from the source rather than typed by
hand) against every locale the JDK exposes. No locale changes an already
uppercase keyword. What breaks is the lower or mixed case spelling of the 64
keywords that contain an `i`, and it breaks only on `tr` and `az` variants:
| JDK | locales exposed | locales affected | keywords broken | uppercase
breakages |
| --- | --- | --- | --- | --- |
| 11 | 748 | 8, all `tr`/`az` variants | 64 of 156 | 0 |
| 17 | 1017 | 10, all `tr`/`az` variants | 64 of 156 | 0 |
| 21 | 1069 | 10, all `tr`/`az` variants | 64 of 156 | 0 |
Affected keywords include `SIN`, `SINH`, `ASIN`, `SIGN`, `SUBSTRING`,
`INSTR`, `INSERT`, `POSITION`, `SPLIT`, `RIGHT`, `BIT_LENGTH`, `CEIL`,
`CEILING`, `TRIM`, `LTRIM`, `RTRIM`, `RADIANS`, `PI`, `IFNULL`, `NULLIF`,
`MULTI_IF`, `UUID`, `TIMESTAMPADD`, `DATEDIFF`, `FROM_UNIXTIME`,
`PARSEDATETIME`, `FORMATDATETIME` and `REGEXP_LIKE` in the function dispatch;
`INT`, `INTEGER`, `BIGINT`, `SMALLINT`, `TINYINT`, `STRING`, `BINARY`,
`DECIMAL`, `TIME`, `TIMESTAMP`, `TIMESTAMP_TZ` and `DATETIME` in `CAST`;
`MINUTE`, `MILLISECOND`, `MICROSECONDS`, `ISODOW`, `ISOYEAR` and `MILLENNIUM`
as datetime fields; `CURRENT_TIME` and `CURRENT_TIMESTAMP`; `RANDOM_PROJECTION`
and `SPARSE_RANDOM_PROJECTION`; and `INTERNAL` for `engine`.
This is the same defect class as #11949. That issue covered the
`LOWER`/`UPPER` SQL functions, which convert user *data*, and it was fixed in
#11951; #11994 did the same for the rename transform. The sites here are the
lookups that resolve SQL *keywords*, which is different code and was not
covered. I also already applied `Locale.ROOT` to `NumericFunction` in this same
package in #11937, with a
`NumericFunctionTest.testRoundingDispatchIsLocaleIndependent` guard. This PR is
the remainder of that subtree.
### Does this PR introduce any user-facing change?
No new or changed options. No behaviour change on any locale outside `tr`
and `az`, and none for uppercase SQL on any locale, so nothing CI exercises
today changes. Lower and mixed case queries that fail on a Turkish or Azeri
worker start working.
### How was this patch tested?
13 call sites in 7 files now pass `Locale.ROOT`. A grep of the subtree
confirms that is every remaining default-locale case conversion under
`transform/sql/`:
| file | site |
| --- | --- |
| `SQLTransform` | `engine` option value |
| `ZetaSQLFunction` | function dispatch, time-key expression, `CAST`
argument |
| `ZetaSQLType` | function return type, time-key expression type |
| `DateTimeFunction` | `DATEADD`, `DATEDIFF`, `DATE_TRUNC`, `EXTRACT` fields
|
| `VectorFunction` | `VECTOR_REDUCE` method |
| `CastFunction` | `CAST` target type |
| `SystemFunction` | `TRUE`/`FALSE` string compare |
Four tests were added to existing test classes, each pinning the default
locale in a `try`/`finally` and restoring it. That is the shape already merged
in this module in #11937, #11951 and #11994. Surefire here is 2.22.2 with the
defaults (`forkCount=1`, `reuseForks=true`), there is no `<parallel>`
configuration and no `junit-platform.properties` under
`seatunnel-transforms-v2`, so classes run sequentially in one JVM and the
pinning cannot leak into another test.
* `ZetaSQLEngineTest.testLowerCaseSqlResolvesUnderAnyDefaultLocale`
* `DateTimeFunctionsTest.testDatetimeFieldIsLocaleIndependent`
* `VectorFunctionTest.testVectorReduceMethodIsLocaleIndependent`
* `SQLTransformTest.testEngineOptionValueIsLocaleIndependent`
**Before and after.** With the four tests in place and the seven production
files restored byte for byte to their `dev` blobs, all four fail. With the fix
applied, all four pass. I also ran the identical inputs under `Locale.ENGLISH`
against those same untouched `dev` files and all four pass there, which is what
isolates the locale as the cause rather than the query.
**Every changed line is load bearing.** I reverted each of the 13 sites one
at a time and reran the four test classes:
| site | test that fails when reverted |
| --- | --- |
| `SQLTransform` engine | `testEngineOptionValueIsLocaleIndependent` |
| `ZetaSQLFunction` dispatch |
`testLowerCaseSqlResolvesUnderAnyDefaultLocale` |
| `ZetaSQLFunction` time key |
`testLowerCaseSqlResolvesUnderAnyDefaultLocale` |
| `ZetaSQLFunction` cast arg |
`testLowerCaseSqlResolvesUnderAnyDefaultLocale` |
| `ZetaSQLType` function type |
`testLowerCaseSqlResolvesUnderAnyDefaultLocale` |
| `ZetaSQLType` time key type |
`testLowerCaseSqlResolvesUnderAnyDefaultLocale` |
| `DateTimeFunction` DATEADD | `testDatetimeFieldIsLocaleIndependent` |
| `DateTimeFunction` DATEDIFF | `testDatetimeFieldIsLocaleIndependent` |
| `DateTimeFunction` DATE_TRUNC | `testDatetimeFieldIsLocaleIndependent` |
| `DateTimeFunction` EXTRACT | `testDatetimeFieldIsLocaleIndependent` |
| `VectorFunction` reduce | `testVectorReduceMethodIsLocaleIndependent` |
| `CastFunction` cast type | `testLowerCaseSqlResolvesUnderAnyDefaultLocale`
|
| `SystemFunction` TRUE/FALSE | none |
12 of 13 are caught. The 13th is not, and I would rather flag it than let it
look like an oversight: `TRUE` and `FALSE` contain no `i`, so that site is
behaviour neutral on every locale in the table above. I changed it anyway so
the subtree has no remaining default-locale conversion for the next reader to
have to re-derive. Say the word and I will drop that one line.
Local run: `mvn -pl seatunnel-transforms-v2 test` is 1162 tests, 0 failures,
0 errors. `mvn -pl seatunnel-transforms-v2 spotless:check` passes.
**Scope.** `seatunnel-transforms-v2` has 11 more default-locale conversions
outside `transform/sql/`, in three unrelated plugins: `validator`
(`DataValidatorTransformConfig`), `Calcite` (`BuiltinFunctions`,
`VectorReduceFunction`) and `nlpmodel` (`ModalityType`,
`ModelInvocationCacheKey`, `AbstractModel`, `PayloadFormat`, `DoubaoModel`).
`Calcite` is a separate transform plugin and is not reachable through this
transform's `engine` option, which only maps `ZETA` and `INTERNAL` to
`ZetaSQLEngine`. I left all of those out to keep this PR to one plugin and one
problem, and I am happy to follow up on them separately if you would like.
### Check list
* [x] If any new Jar binary package adding in your PR, please add License
Notice according [New License
Guide](https://github.com/apache/seatunnel/blob/dev/docs/en/developer/new-license.md)
(no new dependency)
* [x] If necessary, please update the documentation to describe the new
feature. (no option or documented behaviour changes, so no docs update)
* [x] If necessary, please update `incompatible-changes.md` to describe the
incompatibility caused by this PR. (none: the only behaviour that changes is
behaviour that was broken)
* [x] If you are contributing the connector code, please check that the
following files are updated. (not a connector change)
--
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]
