[feature](function) Add Trino-compatible timezone_hour and timezone_m… - #66860
[feature](function) Add Trino-compatible timezone_hour and timezone_m…#66860POSSIBLEwbwb wants to merge 3 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes. The review converged after three full rounds with three blocking correctness/test findings, annotated inline.
Checkpoint conclusions:
- Goal and data correctness: not achieved. The implementation is advertised as Trino-compatible but extracts the session zone after Doris has discarded the input value's zone.
- Scope, clarity, conditions, and reuse: the FE/BE registration changes are otherwise focused and symmetric, and the const/nullable wrappers are safe; resolving the semantic contract may require a broader representation change or explicit rescoping.
- Concurrency and lifecycle: the functions add no shared mutable state, locks, ownership cycle, or static-initialization dependency. The unsafe reusable-plan and persisted-MV lifecycles are covered by the second finding.
- Configuration: no new option is added. Existing
time_zonechanges reach ordinary distributed and non-folded point-query execution, and SQL result caching keys it correctly; supported BE folding exposes the stale prepared-plan path. - Compatibility and parallel paths: FE binding,
BIGINT/nullable typing, visitor translation, BE lookup, and ordinary timezone forwarding agree. No storage format or protocol field is added, and no distinct rolling-upgrade defect beyond normal new-builtin availability was found; prepared short-circuit and both MV families remain unsafe. - Tests and results: the BE unit test covers fixed, DST, fractional, and nullable session-offset arithmetic, but not the advertised input-zone semantics or the prescribed all-const combinations. The added Python test discards each
SETbefore its SELECT, omits the claimed fractional/nullable end-to-end cases, and there is no standardregression-testresult. No matching documentation PR was linked or found. Per the review-task contract, I did not run builds or tests. - Observability, transactions, persistence, and writes: no new logging/metrics or base-table transaction/EditLog path is needed; the persisted materialized-view consequence is covered by the second finding.
- FE/BE variables and performance: no new transmitted variable is introduced, and all ordinary send paths for the existing timezone were checked. Per-row named-zone lookup is needed for DST; no separate CPU or memory issue was substantiated.
- Other issues and user focus: no additional focus point was provided, and the final missed-issue sweep found no further distinct defect.
| auto& result_data = result_column->get_data(); | ||
| result_data.resize(input_rows_count); | ||
|
|
||
| const cctz::time_zone& timezone = context->state()->timezone_obj(); |
There was a problem hiding this comment.
[P1] Extract the input value's zone, not the session zone
Trino's timestamp with time zone retains a zone key, and timezone_hour/timezone_minute extract that value's offset. Doris converts an explicit input zone to UTC and discards it, then this line substitutes the session zone. For example, with session +08:00, CAST('2024-01-15 12:00:00-04:30' AS TIMESTAMPTZ) returns 8/0 here instead of Trino's -4/-30. That silently breaks the advertised migration compatibility. Please resolve the contract by retaining/extracting the input zone (including serialization compatibility), or explicitly scope/rename the feature as session-offset extraction, and add an end-to-end case where the input and session zones differ.
| /** | ||
| * ScalarFunction 'timezone_hour'. | ||
| */ | ||
| public class TimezoneHour extends ScalarFunction |
There was a problem hiding this comment.
[P1] Model the session-timezone dependency
Both new functions read session time_zone but inherit isDeterministic() == true. With BE folding enabled, a constant result can be serialized into a reusable prepared point-query plan and remain stale after SET time_zone. The same classification also admits these expressions into synchronous and async materialized views, whose persisted result/rewrite identity does not retain this execution-only variable. Please represent the session dependency so folding, prepared-plan reuse, and MV admission/rewrite all account for it for both classes, and add prepared-query and MV tests across timezone changes.
| """ | ||
| # UTC+08:00 has no DST, the offset of the session timezone is the same | ||
| # for every instant, so timezone_hour always returns 8 here. | ||
| runner.init("set time_zone = '+08:00'") |
There was a problem hiding this comment.
[P2] Apply SET time_zone on the SELECT's connection
runner.init() sends the Doris SET through PaloQE.do_sql(), which opens and closes a connection for that call. get_sql_result() then opens a fresh session, so neither SELECT sees the timezone set above it; with the default +08:00, the New York block returns 8/0, not -5/-4. Use the existing do_set_properties_sql(select_sql, ["set time_zone = ..."]) pattern (and avoid modifying the unrelated MySQL session), then add the claimed fractional/nullable and input-zone-vs-session-zone cases in the standard regression suite.
There was a problem hiding this comment.
Thanks for the thorough review! All three blocking findings are addressed. The branch now contains:
Commit aee93c9b — the three fixes:
[P1] Input-zone semantics — I resolved the contract by making the session-zone semantics explicit (your "explicitly scope" option): a TIMESTAMPTZ value in Doris stores only the UTC instant — the input zone is discarded at parse time (TimestampTzValue is 8 bytes of UTC microseconds, be/src/core/value/timestamptz_value.h), so recovering the input zone requires redesigning the storage format, well beyond this function pair. The scoping is now explicit in:
- FE javadoc on
TimezoneHour/TimezoneMinute(session-zone extraction, divergence from Trino noted). - BE comment above the offset extraction in
function_timezone_hour_minute.cpp. - Documentation (en/zh) with a divergence example: input
-04:30, session+08:00→ returns 8/0; Trino would return -4/-30. - End-to-end pytest case with differing input/session zones, and BE unit tests
const_input+session_zone_wins_over_input_zone.
[P1] Session-timezone dependency (determinism) — modeled on both engines:
- FE:
isDeterministic()overridden tofalseinTimezoneHour/TimezoneMinute, which setsStatementContext.hasNondeterministicduring analysis (same mechanism asnow()/current_date()): the statement is excluded from SQL cache, and the expression is not folded into reusable prepared plans or admitted into MV expressions. - BE:
"timezone_hour"/"timezone_minute"added toNON_DETERMINISTIC_FUNCTIONSinvectorized_fn_call.cpp, so BE-side constant folding skips them too.
[P2] pytest connection — test_query_timezone_hour_minute rewritten to use do_set_properties_sql, so each SET time_zone and its SELECT run on one connection (this is the established pattern, e.g. test_query_union_join.py). Cases: fixed offset (+08:00), America/New_York winter/summer (DST), fractional (Asia/Kolkata 5/30), input-zone-vs-session-zone divergence (-04:30 input, +08:00 session), and NULL input.
Commit 77bbf781 — robustness: the BE executor now unwraps nullable before const so the column reaches the plain ColumnTimeStampTz data regardless of wrapper order. (Const columns are only legal at the top level of a column tree — Doris enforces this — so the reachable combinations are plain / top-level const / top-level nullable, all covered by the unit tests.)
Verification (local):
- BE unit tests: 6
FunctionTimezoneHourMinuteTestcases pass against the ASAN UT build (fixed, DST, fractional, const input, session-vs-input zone, nullable). - FE compiles cleanly (fe-core, JDK 17 / Maven 3.9).
- The pytest case was run against a local single-node cluster earlier in this PR's development.
Honest caveats:
- I did not add prepared-query / MV regression tests: the palo2 pytest suite has no prepared/MV infrastructure, and I have no local regression-framework environment to validate a new
.groovysuite (I prefer not to submit unvalidated tests). The determinism fix uses the same standard mechanism as existing non-deterministic builtins; if you'd like, I can follow up with aprepared_stmt_p0/mv_p0groovy case once I can run the regression framework locally. - The matching documentation update is in [docs](function) add timezone_hour and timezone_minute function docs doris-website#4070 (pushed; not linked in the PR body since I have no token to comment from here).
…sm and pytest connection for timezone_hour/timezone_minute
…e_minute for robustness
|
@linrrzqqq Thanks for triggering the first review! I've pushed aee93c9 and |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-20T03:35:00Z. Please trigger /review again after that time. |
3 participants
feature Add Trino-compatible timezone_hour and timezone_minute functions
Issue: #48203
Purpose
Add two new scalar functions
timezone_hour(timestamp_tz)andtimezone_minute(timestamp_tz)for Trino compatibility. They return the hour / minute component of the timezone offset
of the session timezone at the given instant (DST-aware), consistent with Trino semantics:
timezone_hour(timestamp '2024-01-01 00:00:00+08:00')->8timezone_hour(timestamp '2024-07-01 00:00:00+00:00')with session tzAmerica/New_York->-4timezone_minute(timestamp '2024-01-01 00:00:00-04:30')->-30Changes
BE
be/src/exprs/function/function_timezone_hour_minute.cpp(new)FunctionTimezoneHour/FunctionTimezoneMinute: readcontext->state()->timezone_obj(),compute offset via
TimestampTzValue::utc_offset()(DST-aware), returnoffset / 3600/(offset % 3600) / 60as Int64.be/src/exprs/function/simple_function_factory.hFE
fe/fe-core/.../nereids/trees/expressions/functions/scalar/TimezoneHour.java(new)fe/fe-core/.../nereids/trees/expressions/functions/scalar/TimezoneMinute.java(new)ExplicitlyCastableSignature, signatureBIGINT <- TIMESTAMP_TZ(WILDCARD),PropagateNullable(same pattern as TimeToSec).fe/fe-core/.../nereids/trees/expressions/visitor/ScalarFunctionVisitor.javafe/fe-core/.../catalog/BuiltinScalarFunctions.javatimezone_hour/timezone_minute.Regression
pytest/qe/palo2/src/test_query_datetime_function.pytest_query_timezone_hour_minute: fixed-offset (Asia/Shanghai),DST (America/New_York), fractional offsets (-04:30, +05:45), nullable input.
Test plan
mvn package -DskipTests -Dskip.doc=true -T 4 -pl fe-core -am— BUILD SUCCESS, checkstyle cleanfunction_timezone_hour_minute_test(fixed offset / DST / fractional / nullable)test_query_datetime_function.pyLicense
This contribution is licensed under the Apache License 2.0.