[fix](fe) Preserve TIMESTAMPTZ user variable type - #66833
Conversation
### What problem does this PR solve? Issue Number: N/A Problem Summary: TIMESTAMPTZ user variables lost their original type when converted from legacy literals to Nereids literals. Preserve DateLiteral types through the existing legacy literal conversion path so timezone rendering and comparison semantics remain correct. ### Release note Fix TIMESTAMPTZ user variables to preserve timezone-aware rendering and comparison semantics. ### Check List (For Author) - Test: FE Unit Test, Regression test, and Manual test - Behavior changed: Yes. TIMESTAMPTZ user variables retain their type and timezone semantics. - Does this need documentation: No
|
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.
The local DateLiteral conversion and DST-fold regression are directionally correct, but the typed user-variable path still has five blocking correctness gaps.
Review status: capped/incomplete. The final allowed subagent round produced one new accepted finding, so the mandated convergence workflow did not converge within three rounds. All candidates discovered in those rounds were independently validated, resolved, and deduplicated before this submission.
Critical checkpoint conclusions
- Goal and proof: The new branch does preserve the primitive and scale of a correctly stored in-process legacy TIMESTAMPTZ value. The unit test proves that narrow bridge and the regression has correct local DST offset/row expectations. It does not prove or achieve end-to-end correctness because forwarding, literal comparison, chained SET folding, and both legacy coercion helpers remain broken.
- Scope and clarity: The production diff is small and reuses
Literal.fromLegacyLiteral, but changing every legacy date user variable from a string to a typed literal activates downstream behavior that the patch and tests do not cover. - Concurrency/thread safety: No new shared state, lock, atomic, or concurrent mutation is introduced; user variables remain connection-owned. The distributed forwarding lifecycle is relevant: the master reconstructs forwarded variables on the RPC thread before installing the forwarded
ConnectContext, which contributes to finding 1. No new lock-order or deadlock risk was found. - Error handling: No new
Statusor catch boundary is added. Findings 3 and 4 show valid SQL now surfacing analysis errors rather than values; those are correctness regressions, not acceptable error handling. - Memory safety/ownership: This is Java scalar conversion with O(1) objects; no native allocation, reservation, buffer ownership, COW, or BE memory-accounting concern applies.
- Lifecycle/static initialization: No new static object or persistence lifecycle is introduced. Session reset clears the map normally. The parallel follower/master restoration lifecycle is incomplete as described in finding 1.
- Configuration: No configuration item is added. The existing mutable global
enable_new_type_coercion_behavior=falseis supported and exposes findings 4 and 5; the patch does not preserve behavior under that mode. - Compatibility: There is no new function symbol, storage format, EditLog, or wire-field change. The existing FE-to-FE
TExprNode.typealready carries TIMESTAMPTZ and scale, but the receiver ignores it; its current generic decoder also drops TIMESTAMPTZ scale. Rolling/local behavior is therefore inconsistent. - Parallel paths and conditions: Local rebinding and follower-to-master rebinding are functionally parallel but only the local path is fixed. The broad
instanceof DateLiteralcondition correctly maps DATE/DATEV2/DATETIME/DATETIMEV2 locally; the concrete surviving defects are TIMESTAMPTZ-specific downstream paths. - Data correctness: Five distinct failures survive: forwarding can lose type/scale and return wrong DST rows; same-second fractions compare equal; chained string assignment fails; legacy unequal-scale comparison fails analysis; and legacy common-result typing can collapse distinct DST instants. No additional generic date-family defect survived validation.
- Tests and expected results: The added
.outvalues are correct for 05:05Z inAmerica/New_York(01:05 at -04:00, matching ids 1 and 3), queries are ordered, and the table setup follows suite conventions. Missing high-value coverage is forced FE forwarding, same-second microsecond comparison, chained SET-to-string, legacy unequal-scale comparison, and legacyNULLIF/CASE across the fold. The unit test directly injects a legacy literal and bypasses SQL assignment/forwarding. - Executed tests: I ran no builds or tests because the authoritative review prompt forbids them. Current GitHub checks show CheckStyle, BE UT (macOS), formatting, license, secrets, dependency review, and other lightweight checks passing; no completed FE unit or regression check is visible.
- Observability: No new logging or metrics are needed for this O(1) expression bridge; the failures require correctness fixes and targeted tests rather than added observability.
- Transactions, persistence, and writes: No EditLog, transaction, durable metadata, or production data-write path changes. Test inserts are isolated regression setup. Master failover persistence semantics are not involved, while ordinary statement forwarding is covered by finding 1.
- FE/BE variables: No new FE-to-BE variable is introduced. An existing FE-to-FE user-variable payload is a required parallel path and is mishandled. BE null/nullable column rules do not apply to this FE-only change.
- Performance: The new branch is constant time and introduces no meaningful CPU, memory, or asymptotic regression.
- Other issues: Literal equality/hash, cache keys, session cleanup, arithmetic, non-TIMESTAMPTZ date conversions, and regression determinism were checked; no further valuable finding survived. There was no additional user-specified focus.
The five inline comments identify the blocking fixes and missing coverage.
| return Literal.of(((FloatLiteral) literalExpr).getValue()); | ||
| } else if (literalExpr instanceof DecimalLiteral) { | ||
| return Literal.of(((DecimalLiteral) literalExpr).getValue()); | ||
| } else if (literalExpr instanceof DateLiteral) { |
There was a problem hiding this comment.
[P1] Preserve the type across forwarded user variables
This only fixes the in-process path. A user can SET @v = CAST(... AS TIMESTAMPTZ(6)) on a follower and then have the query forwarded when that FE cannot read or is configured to forward. FEOpExecutor sends a TExprNode whose type is still TIMESTAMPTZ(6), but ConnectProcessor.getLiteralExprFromThrift ignores that field and reparses node.date_literal.value with a null type before this branch runs. The value therefore arrives here as DATE/DATETIME(V2), so rendering loses the offset and a DST-fold predicate is coerced to wall-clock DATETIMEV2 semantics and can return the wrong rows. Please decode node.type including its TIMESTAMPTZ scale (Type.fromThrift currently drops that scale), use it for reconstruction, and add a forced-forward round-trip covering rendering and equality.
| } else if (literalExpr instanceof DecimalLiteral) { | ||
| return Literal.of(((DecimalLiteral) literalExpr).getValue()); | ||
| } else if (literalExpr instanceof DateLiteral) { | ||
| return Literal.fromLegacyLiteral(literalExpr, literalExpr.getType()); |
There was a problem hiding this comment.
[P1] Include TIMESTAMPTZ microseconds in literal comparison
This branch makes user variables reach FE folding as TimestampTzLiteral, but that class inherits DateLiteral.compareTo, where the fractional tie-breaker is applied only to DateTimeV2Literal. Because TimestampTzLiteral is a sibling subclass and DateTimeLiteral.getValue() stops at whole seconds, SET @a = ...00.100000Z; SET @b = ...00.200000Z; SELECT @a = @b, @a < @b folds to 1, 0 instead of 0, 1. Please include TIMESTAMPTZ microseconds in the comparison and add equality/ordering coverage for two user variables within the same second.
| } else if (literalExpr instanceof DecimalLiteral) { | ||
| return Literal.of(((DecimalLiteral) literalExpr).getValue()); | ||
| } else if (literalExpr instanceof DateLiteral) { | ||
| return Literal.fromLegacyLiteral(literalExpr, literalExpr.getType()); |
There was a problem hiding this comment.
[P1] Keep chained user-variable casts foldable
After this returns a TimestampTzLiteral, SET @rendered = CAST(@ts AS VARCHAR(64)) is no longer accepted. SetUserDefinedVarOp requires the expression to FE-fold to a literal, but TimestampTzLiteral.uncheckedCastTo handles only TIMESTAMPTZ/DATETIMEV2 and throws for string-like targets, so folding leaves a Cast and reports must be constant value. The added SELECT still passes because it can defer the cast to BE. Please add a session-timezone-aware string fold (not the internal UTC getStringValue()) and cover a chained SET after changing time_zone.
| } else if (literalExpr instanceof DecimalLiteral) { | ||
| return Literal.of(((DecimalLiteral) literalExpr).getValue()); | ||
| } else if (literalExpr instanceof DateLiteral) { | ||
| return Literal.fromLegacyLiteral(literalExpr, literalExpr.getType()); |
There was a problem hiding this comment.
[P1] Preserve TIMESTAMPTZ in legacy comparison coercion
With the supported global enable_new_type_coercion_behavior=false, user variables declared as TIMESTAMPTZ(3) and TIMESTAMPTZ(6) no longer share an exact type, so comparison takes the legacy common-type path. That path falls through to DATETIME, but CheckCast does not allow TIMESTAMPTZ-to-DATETIME, so the valid comparison now fails analysis instead of comparing the instants. Please keep TIMESTAMPTZ as the common type at the maximum scale in this compatibility branch and add differently-scaled coverage under the legacy setting.
| } else if (literalExpr instanceof DecimalLiteral) { | ||
| return Literal.of(((DecimalLiteral) literalExpr).getValue()); | ||
| } else if (literalExpr instanceof DateLiteral) { | ||
| return Literal.fromLegacyLiteral(literalExpr, literalExpr.getType()); |
There was a problem hiding this comment.
[P1] Preserve TIMESTAMPTZ in legacy result coercion
The legacy common-result path is separate from comparison coercion and is also exposed by returning typed values here. With enable_new_type_coercion_behavior=false, TIMESTAMPTZ(3) and TIMESTAMPTZ(6) arguments to NULLIF, IF, or CASE reach findCommonPrimitiveTypeForCaseWhen, which selects DATETIMEV2 instead of a wider TIMESTAMPTZ. The allowed casts convert each instant to the current session wall clock; across the New York fall-back, 05:05Z and 06:05Z both become 01:05, so NULLIF(@a,@b) can return NULL for distinct instants. Please preserve TIMESTAMPTZ at the maximum scale in this helper and add unequal-scale legacy-mode NULLIF/CASE coverage across the fold.
|
/review 重新review一下，之前的review的内容都不是当前pr引入的，过去的行为都是错误的。 |
There was a problem hiding this comment.
Fresh review result: no additional PR-introduced findings.
Review status: converged after Round 1. Two normal full-review tracks and a separate risk-focused track all returned NO_NEW_VALUABLE_FINDINGS; the main-agent adjudication and final changed-file sweep left zero unresolved candidates and zero inline comments.
Opinion: the local type-preservation fix is correct, focused, and non-blocking.
User focus
I re-reviewed causality against the authoritative four-file diff. I did not reuse the five prior TIMESTAMPTZ threads as new findings from this PR; they were treated as existing review context and duplicate-fenced. No distinct additional issue introduced by this change was found.
Critical checkpoint conclusions
- Goal and proof:
ConnectContext.getLiteralForUserVarnow reusesLiteral.fromLegacyLiteralfor stored date-family values, so a local TIMESTAMPTZ user variable keeps its type, declared scale, and UTC instant when the session timezone later changes. The unit test checks the reconstructed class, scale 6, and nonzero microseconds. The regression performs SET at UTC, use atAmerica/New_York, and verifies both rendering and instant equality through the DST fold;05:05Zcorrectly renders as01:05 -04:00and matches ids 1 and 3. - Scope and clarity: The production change is a small existing-conversion reuse. Although the branch covers every legacy
DateLiteral, the conversion switch has exact DATE, DATEV2, DATETIME, DATETIMEV2, and TIMESTAMPTZ cases and preserves every applicable field and scale. No concrete non-TIMESTAMPTZ regression was found. - Concurrency and thread safety: User variables remain connection-owned and are resolved synchronously. No shared state, new thread, lock, atomic, lock ordering, or deadlock surface is introduced.
- Lifecycle and static initialization: Session initialization/reset behavior is unchanged, and the patch adds no static/global initialization, ownership cycle, asynchronous lifetime, or persistence lifecycle.
- Error handling and memory safety: The branch delegates to the established exhaustive conversion instead of silently continuing. It adds no ignored status, exception boundary, native allocation, buffer ownership, COW, or memory-tracking concern; work is a constant-size Java literal conversion.
- Configuration: No configuration item is added or changed. The regression changes only the session
time_zoneand restores session settings at the end. - Compatibility and parallel paths: No function symbol, storage format, EditLog record, RPC field, or FE-BE contract changes. Local substitution, forwarded reconstruction, chained SET, cache/replacement behavior, and legacy coercion paths were inspected. The five already-live thread topics were not duplicated, and no distinct changed-location instance remained.
- Conditions and data correctness: The
instanceof DateLiteralcondition is exhaustive for reachable stored date-family literals. TIMESTAMPTZ retrieval copies the already-stored UTC fields and catalog scale directly; it does not reinterpret them in the later session timezone. - Tests and expected results: The JUnit 5 assertions cover type/scale/value preservation. Regression queries are deterministic (
order_qt/ORDER BY), tables are dropped before creation and retained afterward, and the new.outrows and New York offsets agree with the inserted instants. I ran no builds or tests because the review prompt forbids them. Visible CI shows CheckStyle and lightweight repository checks passing, but no completed FE unit or regression job was available as functional execution evidence. - Observability: No logging or metrics are warranted for this small deterministic expression bridge; correctness is adequately targeted by unit and regression coverage.
- Transactions, persistence, and writes: No production transaction, data-write, visible-version, EditLog, failover, or durable metadata path is modified. The test inserts are isolated regression setup.
- FE-BE propagation and nullable handling: No new variable is transmitted between FE and BE, and no BE column/nullability path is involved.
- Performance: Resolution remains O(1) with a few scalar-object constructions; no meaningful CPU, memory, or asymptotic regression was found.
- Other issues: Test determinism, date-family special cases, session cleanup, compatibility, and documentation impact were checked; no further valuable finding survived the final sweep.
3 participants
TIMESTAMPTZ user variables could lose their original type when converted from a legacy literal to a Nereids literal. The value was restored as a string, which changed timezone rendering and comparison semantics after the session timezone changed.
Root cause:
ConnectContext.getLiteralForUserVardid not handleDateLiteraland fell back to string conversion. This change uses the existing legacy literal conversion path to preserve the TIMESTAMPTZ type, scale, and instant value.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)