[fix](nereids) Fix TIMESTAMPTZ string cast in FE folding#63144
Open
zclllyybb wants to merge 1 commit into
Open
[fix](nereids) Fix TIMESTAMPTZ string cast in FE folding#63144zclllyybb wants to merge 1 commit into
zclllyybb wants to merge 1 commit into
Conversation
Contributor
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found one correctness issue in the new FE TIMESTAMPTZ-to-string folding path. The implementation otherwise stays focused and has targeted unit coverage for offset-based session time zones, but it misses Doris-supported timezone aliases such as CST.
Critical checkpoint conclusions:
- Goal/test: The PR aims to make FE folding of TIMESTAMPTZ string casts match session-time-zone output; tests cover numeric offset zones and scale preservation, but not alias zones.
- Scope: The change is small and localized to TimestampTzLiteral casting plus tests.
- Concurrency/lifecycle/config: No new concurrency, lifecycle, or config concerns.
- Compatibility/parallel paths: FE folding should match BE/runtime cast semantics; the alias handling gap breaks this for valid session time zones.
- Testing: Needs an additional alias-zone case, e.g. CST, after fixing timezone resolution.
- Observability/transactions/data writes: Not applicable.
User focus: no additional user-provided review focus was present.
| @@ -180,6 +195,29 @@ protected Expression uncheckedCastTo(DataType targetType) throws AnalysisExcepti | |||
| throw new AnalysisException(String.format("Cast from %s to %s not supported", this, targetType)); | |||
| } | |||
|
|
|||
Contributor
There was a problem hiding this comment.
This uses DateUtils.getTimeZone(), which calls ZoneId.of() on the raw session variable without Doris' alias map. SET time_zone = 'CST' is valid in FE (TimeUtils.checkTimeZoneValidAndStandardize accepts it and maps it to Asia/Shanghai, and BE query globals also special-case CST), but this new FE folding path will now throw ZoneRulesException: Unknown time-zone ID: CST when folding CAST(<timestamptz literal> AS STRING/VARCHAR/CHAR). Please resolve the session zone through the same alias-aware path as the rest of FE, for example TimeUtils.getTimeZone().toZoneId() or ZoneId.of(sessionTimeZone, TimeUtils.timeZoneAliasMap), and add a test for CST so FE folding remains consistent with runtime casting.
morrySnow
reviewed
May 19, 2026
Comment on lines
+182
to
+183
| } else if (targetType.isVarcharType()) { | ||
| return new VarcharLiteral(getStringValueInSessionTimeZone(), ((VarcharType) targetType).getLen()); |
Contributor
There was a problem hiding this comment.
why do not check length as char ?
Contributor
Author
There was a problem hiding this comment.
uncheckedCastToString#Literal.java also dont check for varchar. only char checks. but I make the code same by directly use uncheckedCastToString#Literal.java and override castValueToString
zclllyybb
force-pushed
the
codex-master-ttz-string-fe
branch
from
36ef310 to
804c370
Compare
FE constant folding casts folded TIMESTAMPTZ values through TimestampTzLiteral.checkedCastTo(). TimestampTzLiteral stores the normalized instant as UTC date/time fields, and the old FE cast path only handled TIMESTAMPTZ and DATETIMEV2 targets. For string-like targets this exposed the internal UTC fields instead of formatting the value in the session time zone like runtime evaluation does. Teach TimestampTzLiteral to provide a TIMESTAMPTZ-aware castValueToString(): convert the internal UTC instant to DateUtils.getTimeZone(), preserve the literal scale, and append the rendered zone offset. The actual CHAR/VARCHAR/STRING folding is delegated to Literal.uncheckedCastToString() so the behavior stays consistent with the rest of Nereids: STRING keeps the full value, VARCHAR uses VarcharLiteral length truncation, and CHAR only folds when the target length can hold the rendered value. The offset suffix is formatted from ZoneOffset.getTotalSeconds() as sign + HH:MM and documented as matching BE TimestampTzValue::to_string() in be/src/core/value/timestamptz_value.cpp. This avoids Java library ids such as Z for UTC, and keeps FE constant folding aligned with BE runtime stringification. Example covered by the test: an internal UTC value of 2023-07-13 19:26:00 in session time zone +12:34 folds to 2023-07-14 08:00:00+12:34, not the raw UTC value. The same test also checks UTC renders +00:00 rather than Z. Add FE unit coverage for FoldConstantRuleOnFE casting TIMESTAMPTZ to STRING and direct VARCHAR/CHAR casts. Verification: mvn test -Dcheckstyle.skip=true -DfailIfNoTests=false -Dmaven.build.cache.enabled=false -Dtest=org.apache.doris.nereids.trees.expressions.literal.TimestampTzLiteralTest -Dfe_ut_parallel=0
zclllyybb
force-pushed
the
codex-master-ttz-string-fe
branch
from
804c370 to
f032d5c
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 31240 ms |
Contributor
TPC-DS: Total hot run time: 169873 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FE constant folding casts folded TIMESTAMPTZ values through TimestampTzLiteral.checkedCastTo(). TimestampTzLiteral stores the normalized instant as UTC date/time fields, and the old FE cast path only handled TIMESTAMPTZ and DATETIMEV2 targets. For string-like targets this exposed the internal UTC fields instead of formatting the value in the session time zone like runtime evaluation does.
Add explicit CHAR, VARCHAR, and STRING cast handling for TimestampTzLiteral. The new path converts the internal UTC instant to DateUtils.getTimeZone(), preserves the literal scale, and appends the rendered zone offset so folded results match TIMESTAMPTZ string semantics.
Example covered by the test: an internal UTC value of 2023-07-13 19:26:00 in session time zone +12:34 folds to 2023-07-14 08:00:00+12:34, not the raw UTC value.
Add FE unit coverage for FoldConstantRuleOnFE casting TIMESTAMPTZ to STRING and direct VARCHAR/CHAR casts.