SEPURI-SAI-KRISHNA commented on PR #12703:
URL: https://github.com/apache/seatunnel/pull/12703#issuecomment-6386417504
Hi @chovy-3012, one small thing on `getFormatter`, raised late and non
blocking, but it is a one line change and this PR is its natural home.
`getFormatter` builds the formatter with
`DateTimeFormatter.ofPattern(pattern)`, which captures the JVM default FORMAT
locale at construction. For `FROM_UNIXTIME` that means a text pattern renders
in the worker's language. I filed that for the two existing call sites as
#12975, with the fix in #12976, which passes `Locale.ENGLISH` at
`DateTimeFunction:616` and `:799`. Since this PR replaces `:799` with
`getFormatter(format)`, the natural home for that side becomes `getFormatter`
itself.
**The parse path is the sharper case, and it is new here.** `unixTimestamp`
parses with the same formatter, and a pattern whose letters produce text fails
outright on a non English default. Measured on JDK 17, parsing `23 Oct 2026`
with `dd MMM yyyy`, varying only the FORMAT locale:
```
und OK -> 2026-10-23
en-US OK -> 2026-10-23
tr-TR DateTimeParseException
ja-JP DateTimeParseException
de-DE DateTimeParseException
```
`DateTimeParseException` extends `DateTimeException`, so it lands in
```java
} catch (DateTimeException e) {
// Unparseable input returns NULL rather than throwing
return null;
}
```
and `UNIX_TIMESTAMP('23 Oct 2026', 'dd MMM yyyy')` returns NULL on those
workers instead of an epoch. The job does not fail, the row just loses its
value, and the same config gives different results on different workers.
The suggested change is to construct with an explicit locale:
```java
DateTimeFormatter formatter = DateTimeFormatter.ofPattern(pattern,
Locale.ENGLISH);
```
`Locale.ENGLISH` rather than `Locale.ROOT` because `DAYNAME` and `MONTHNAME`
in this same class already pin English through
`DateFormatSymbols.getInstance(Locale.ENGLISH)`, and because `ROOT`
abbreviates: `EEEE` gives `Sat` under `ROOT` but `Saturday` under `ENGLISH`. It
also makes the cache key honest, since `FORMATTER_CACHE` is keyed on the
pattern alone and the locale is currently implicit in whatever the default was
at first construction.
Your tests would not catch this: no pattern in `SQLDateTimeFunctionsTest` or
`func_unix_timestamp.conf` uses `MMM`, `EEE`, `a`, `G` or `QQQ`, so CI stays
green either way. That is also why I missed it on my first read.
Whichever of this PR and #12976 merges second will need a trivial rebase on
that method. Happy either way, and happy to leave it entirely to you.
--
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]
