[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 5 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. |
|
/review |
There was a problem hiding this comment.
Request changes. Round 1 converged: both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS after their missed-issue rechecks. I am adding no new inline comments because the only remaining substantiated correctness defect is an exact duplicate of the existing session-timezone dependency P1.
That P1 is still incompletely resolved. The FE isDeterministic() == false changes correctly prevent FE/BE-fold collection, SQL-cache reuse, default sync/async MV admission, and reuse of the same prepared short-circuit handle. On BE, however, the new names only affect VectorizedFnCall::is_deterministic(). VectorizedFnCall::is_constant() does not consult that method, and both new functions retain the default use_default_implementation_for_constants() == true. Reusable::init() opens the output expressions and populates _constant_col while its new RuntimeState still uses the default +08:00; PointQueryExecutor::init() applies request.time_zone only afterward, and _do_execute() returns the cached value without reevaluation. Thus, under session America/New_York, a full-key short-circuit query projecting timezone_hour(CAST('2024-01-15 12:00:00' AS TIMESTAMPTZ)) can return 8 on its first BE handle instead of -5. The pre-existing nondeterministic functions in this name set also disable default constant handling; these two do not. Please finish the existing P1 by disabling default constant handling for both functions (or making constant caching require determinism / installing the request timezone before expression open), and add the already-requested literal point-query coverage.
Checkpoint conclusions:
- Goal and data correctness: ordinary session-zone offset extraction, DST, fractional offsets, null propagation, and FE/BE
BIGINTtyping are correct. The goal is not complete because the short-circuit first-open path can return an offset for the default zone rather than the request zone. - Scope, clarity, reuse, and special conditions: registration and visitor changes are focused and symmetric. The explicit session-zone contract and Trino divergence are now documented in FE/BE code and tested. The wrapper-order comment is broader than the legal shapes, but framework normalization makes the implementation safe.
- Concurrency: the functions add no shared mutable state or locks, and no deadlock or lock-order issue was found. The blocking defect is expression-cache lifecycle ordering, not a newly introduced data race.
- Lifecycle and static initialization: no ownership cycle, cleanup leak, or cross-TU static-initialization dependency was found. The reusable point-query expression lifetime is the unresolved issue described above.
- Configuration: no new configuration is added. Ordinary execution observes the existing session
time_zone; the point-query cache is populated before that existing value is installed. - Compatibility and parallel paths: the change is additive, FE/BE signatures agree, and it introduces no storage-format or protocol field. Regular distributed execution is correct; the short-circuit point-query path is not. The function is explicitly session-offset extraction rather than preservation of Trino's input-zone semantics.
- Tests and results: the BE unit test covers fixed, DST, fractional, const, nullable, and session-vs-input-zone cases, and the pytest now keeps each
SETandSELECTon one connection. Prepared/MV and standardregression-testcoverage requested by the existing thread is still absent. Per the review task, I did not run builds or tests; live checks show the build-all job was skipped, so current CI does not independently prove compilation or execution. - Observability: no new log, metric, or tracing requirement was identified for these local scalar functions.
- Transactions, persistence, and writes: no base-table write, transaction, visible-version, EditLog, or delete-bitmap path is modified. Default MV admission now recognizes nondeterminism; its missing regression coverage is already tracked in the existing thread.
- FE/BE variables: no new transmitted variable is introduced. The existing timezone request is forwarded, but the BE reusable lifecycle consumes it too late for cached literal expressions.
- Performance: the per-row named-zone lookup is required for DST-aware offsets; no separate material CPU, allocation, or memory-accounting problem was substantiated.
- Other issues and user focus: no additional user focus was provided. The final changed-file and unresolved-candidate sweep found no further distinct issue, and the prior input-zone scoping and pytest connection concerns are addressed without duplicating their threads.
…hour/timezone_minute on point-query path
|
/review |
There was a problem hiding this comment.
Review status: complete after two convergence rounds. Requesting changes for one correctness issue and one performance issue.
Summary
The FE/BE registration, TIMESTAMPTZ(*) -> BIGINT signatures, nullable propagation, signed fractional offsets, and ordinary DST lookup are consistent. The input-zone versus session-zone behavior is now explicitly documented as a Doris divergence, and the pytest connection handling is fixed. Point-query initialization still permits a constant descendant to be cached before the session timezone is applied (P1), and constant inputs do redundant O(N) lookup and materialization work (P2).
Critical checkpoints
- Goal and proof: direct BE and pytest coverage exercises fixed, DST, fractional, nullable, constant, and input-zone-divergence cases, but the point-query result is not correct for the nested constant path in MAIN-001. A nested DST point-query regression is required. The prepared/MV test gap is already tracked in an existing thread and is not duplicated here.
- Scope and clarity: the nine-file change is focused and the shared helper and registration are clear; the remaining constant-cache policy is incomplete, and the const branch is unnecessarily row-wise.
- Concurrency: no new shared mutable state or lock ordering was introduced.
RuntimeStateis query-local; the point-query reusable lifecycle and initialization ordering is the correctness issue. - Lifecycle and ownership: the function objects are stateless and introduce no ownership cycle. Opening the expression tree before installing the request timezone is MAIN-001.
- Configuration: no new setting is introduced; the feature uses the existing session
time_zonepropagation. - Compatibility: FE/BE names and types align, with no storage or wire-format change. Doris's loss of the input zone is now documented rather than presented as Trino-equivalent behavior.
- Parallel paths: regular execution, FE/BE folding, SQL cache, prepared reuse, synchronous and asynchronous MV handling, and point-query execution were traced. The outer nondeterminism gates work, but a constant descendant escapes them.
- Conditions and error handling: type and nullability invariants use the existing function framework, null maps are preserved, and no
Statusis ignored. - Tests: the added tests cover the direct cases above. Per the review automation instructions, I did not run builds or tests. Add the nested point-query regression from P1 and assert the block-local physical const shape for P2.
- Observability: these pure scalar functions do not require new logs or metrics.
- Persistence, transactions, writes, and FE/BE variables: no new persistence, transactional, or write path exists; the only cross-layer variable is the existing timezone request.
- Performance: MAIN-002 identifies avoidable O(N) cctz lookups and result materialization for a block-constant input.
- User focus: no additional focus area was specified.
| // inputs both reach the plain ColumnTimeStampTz data below. | ||
| col = remove_nullable(col); | ||
| if (is_column_const(*col)) { | ||
| col = assert_cast<const ColumnConst&>(*col).convert_to_full_column(); |
There was a problem hiding this comment.
[P2] Preserve block-local constness without enabling cross-request caching
Because the framework constant path is disabled, this expands a one-value ColumnConst to input_rows_count, allocates a full result column, and performs the identical cctz lookup for every scanned row. A projection such as timezone_hour(CAST('2024-01-15 12:00:00' AS TIMESTAMPTZ)) over a large table therefore does O(N) timezone work for one per-execution value. Keep use_default_implementation_for_constants() false so VectorizedFnCall::is_constant() cannot cache across requests, but detect the const argument here, evaluate its nested value once, and return a block-local ColumnConst; the const-input test can assert that physical shape.
| // columns without re-evaluating them (VectorizedFnCall::is_constant | ||
| // consults this flag). Disable it like other nondeterministic functions | ||
| // (e.g. random, uuid). | ||
| bool use_default_implementation_for_constants() const override { return false; } |
There was a problem hiding this comment.
[P1] Apply the request timezone before constant children open
This override only makes the outer call nonconstant. VectorizedFnCall::open() still opens its children first, and a constant VCastExpr caches itself through VExpr::get_const_col(). In the point-query path, Reusable::init() opens the expression tree while its RuntimeState still has Doris's default +08:00; PointQueryExecutor::init() applies request->time_zone only afterward. For example, with session America/New_York, timezone_hour(CAST(least('2024-03-10 03:30:00','2024-03-11 03:30:00') AS TIMESTAMPTZ)) survives FE folding because least has no FE evaluator. The child cast is cached as 2024-03-09 19:30 UTC under +08:00, so the outer call returns -5; parsing the selected value in New York gives 2024-03-10 07:30 UTC and should return -4. This is distinct from the existing outer-result cache thread: the stale value is a constant descendant and is wrong on the first point-query request. Please install the request timezone before opening or caching the point-query expression tree and add this nested-constant DST regression.
… and const fast path for timezone_hour/minute The point-query short-circuit executor opened the output expressions with the default +08:00 timezone and applied the request timezone only afterwards, so constant descendants (e.g. a const VCastExpr above a session-timezone-dependent function) cached a value evaluated with the wrong zone. Install the request timezone in Reusable::init() before the expressions are prepared/opened. With the framework constant path disabled for timezone_hour/ timezone_minute, a block-local const argument was expanded to input_rows_count rows and re-evaluated per row; compute the single value once and keep the const shape of the result.
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.