SEZ9 commented on issue #12975: URL: https://github.com/apache/seatunnel/issues/12975#issuecomment-6406781474
Thanks for the detailed follow-up, and for measuring `Locale.ROOT` vs `Locale.ENGLISH` instead of assuming they agree. The `EEEE` → `Fri` and `MMMM` → `Oct` abbreviation under `Locale.ROOT` is exactly the kind of silent change we want to avoid for English workers, so `Locale.ENGLISH` is the right pick here, and lining it up with what `DAYNAME`/`MONTHNAME` already do via `DateFormatSymbols.getInstance(Locale.ENGLISH)` makes the behaviour of the class internally consistent. Passing the explicit locale at both the `formatdatetime` and `fromUnixTime` call sites covers the scope of what I flagged. The test shape also sounds right: running the real `SQLTransform` under `ENGLISH`, `tr-TR` and `ja-JP`, restoring the default in a `finally` the way `testAtTimeZone` does, and asserting `FORMATDATETIME(dt,'EEEE')` equals `DAYNAME(dt)` and `FORMATDATETIME(dt,'MMMM')` equals `MONTHNAME(dt)` with a numeric control alongside. Two small asks before I sign off on the PR: 1. You mention 7 text patterns but the table shows 2. Could you either add the remaining ones (at least `MMM`, `a`, `G`, `QQQ` from the original report) to the locale-switching test, or paste the rest of that comparison in the PR so the "0 of 7 changed" claim is verifiable? 2. Since non-English workers will now see English month/day names, please make sure the PR description's behaviour-change note is also reflected in the function docs for `FORMATDATETIME`/`FROM_UNIXTIME` so it shows up in release notes. Agreed that an optional locale argument is a separate follow-up; keeping this PR to a deterministic default is the right scope. <!-- streview-comment:2037 --> -- 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]
