morningman opened a new pull request, #68865:
URL: https://github.com/apache/doris/pull/68865
### What problem does this PR solve?
Issue Number: None
Related PR: #65868
Problem Summary:
**In short.** This ports the time-zone part of branch-4.1 #65868.
`time_zone` accepts Java short IDs such as JST, PST or IST, but the FE forwards
them as written, except for CST. Backends do not know these names: they
silently evaluate time functions in another zone, and Java code running on a
backend fails on them. The FE now sends the canonical zone ID wherever a time
zone leaves the session layer: to backends for queries, loads, BE constant
folding and point queries, and to connectors.
**Background.** `SET time_zone` validates the name against
`TimeUtils.timeZoneAliasMap`, which is `ZoneId.SHORT_IDS` plus the Doris
overrides (CST and PRC map to Asia/Shanghai, UTC and GMT to UTC), and stores
the name as written. When the FE evaluates time itself, it resolves the name
through that map. The places that hand the time zone on behave differently:
| Where the time zone goes | master before this PR |
|---|---|
| Query globals (`CoordinatorContext`), BE constant folding, short-circuit
point queries | raw name, CST special-cased to Asia/Shanghai |
| Broker load and export (`setQueryGlobalsForLoad`), stream and routine load
(`NereidsStreamLoadPlanner`) | raw name |
| Connectors (`ConnectorSessionBuilder`) | raw name |
branch-4.1 #65868 added `TimeUtils.getCanonicalTimeZone` (`ZoneId.of(name,
timeZoneAliasMap).getId()`) and used it for the query, constant-folding and
point-query paths. master does not have it.
**The problem, and what it cost.**
- **Wrong results, no error.** A backend looks the name up in its tz
database, which has only EST, HST and MST of the 28 Java short IDs. For the
others (JST, PST, IST, CTT and 20 more; CST is special-cased), `RuntimeState`
ignores the failed lookup and keeps UTC, and a short-circuit point query falls
back to the backend's default zone. Constant expressions folded on the FE are
still right, so one query can mix two zones.
- **Loads convert in the wrong zone.** A stream load or broker load whose
`timezone` is such a name converts with the same fallback.
- **Java code on the backend fails.** It resolves the name with `ZoneId.of`,
which rejects short IDs. A Paimon INSERT under JST fails with
`ZoneRulesException: Unknown time-zone ID: JST`.
- **Connectors get the raw name.** The Iceberg and Paimon connectors carry
their own copies of the alias map. The MaxCompute predicate converter does not,
so for a session in CST it stops pushing down datetime predicates.
**How this PR fixes it.**
- Adds `TimeUtils.getCanonicalTimeZone`, as in branch-4.1.
- Applies it in `CoordinatorContext.refreshQueryGlobals` (queries, group
commit, jobs with a session), `createQueryGlobalsWithoutSession` (jobs started
by FE daemons), `setQueryGlobalsForLoad` (broker load, export),
`NereidsStreamLoadPlanner` (stream and routine load, transactional INSERT),
`FoldConstantRuleOnBE`, `PointQueryExecutor` and `ConnectorSessionBuilder`. The
CST special cases go away, because the alias map already resolves CST to
Asia/Shanghai.
- Names a backend already understands are unchanged (`Asia/Tokyo`, `UTC`,
`+08:00`, `GMT+08:00`). Short IDs become tz database names or offsets: JST
becomes `Asia/Tokyo`, PST `America/Los_Angeles`, EST `-05:00`.
- The other time zones a backend receives need no change.
`hive.parquet.time-zone` rejects short IDs when the catalog or TVF is created.
The zones a backend hands on, to Java scanners and writers, Arrow Flight
results and partial updates, come from the query globals.
**Results.** Local FE + BE, a row with `ts` = 0, `time_zone` = JST:
| | before | after |
|---|---|---|
| `select from_unixtime(ts) from t` | 1970-01-01 00:00:00 | 1970-01-01
09:00:00 |
| the same query as a short-circuit point query | 1970-01-01 08:00:00 |
1970-01-01 09:00:00 |
| stream load with `timezone: JST` and `dt = from_unixtime(ts)` | 1970-01-01
00:00:00 | 1970-01-01 09:00:00 |
| INSERT into a filesystem Paimon table | fails: `Unknown time-zone ID: JST`
| succeeds |
PST, IST and CTT behaved like JST before the fix. EST and CST were already
right.
Original author: @suxiaogang223
### Release note
Fix wrong results of time functions evaluated on backends and of stream
loads with a `timezone` header, and failed Paimon writes, when `time_zone` is a
Java short ID such as JST, PST or IST.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- New `query_p0/session_variable/test_session_time_zone_short_ids`
covers a scan, a short-circuit point query and a stream load under short IDs.
It passes locally and fails on the unfixed FE (the JST scan returns 1970-01-01
00:00:00).
- `test_paimon_write_append_only` gains an INSERT under JST,
compared with Spark. It needs the Paimon docker fixture and was not run locally.
- [x] Unit Test
- `TimeUtilsTest`, `CoordinatorContextTest` and
`ConnectorSessionImplTest` check the canonical IDs of the session, load and
connector paths. Making the helper, or the load or connector call site, pass
the name through makes them fail. `FoldConstantRuleOnBETest`,
`PointQueryExecutorTest`, `ShortCircuitPointQueryTest`,
`GroupCommitPlannerTest`, `FileQueryScanNodeTest`, `DateTimeAcquireTest` and
`LiteralTest` also pass.
- [x] Manual test (add detailed scripts or steps below)
- The before/after table above, on a local FE + BE with a filesystem
Paimon catalog.
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [x] Yes. Backends and connectors receive canonical zone IDs. When
`time_zone` (or a load's `timezone`) is a Java short ID, time functions
evaluated on backends and load conversions now use that zone instead of a
fallback.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
