[Fix](nereids) Fix nullable date literal binding and date *_diff folding#64127
Merged
Conversation
linrrzqqq
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
run buildall |
Collaborator
Author
|
/review |
![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 blocking correctness issue in FE constant folding for days_diff. The new implementation diverges from BE/runtime semantics for DateTimeV2 cases that cross a date boundary but are less than a full day apart.
Critical checkpoint conclusions:
- Goal/test coverage: The PR addresses nullable date literal binding and datediff folding, and adds regression coverage, but the new tests do not cover partial-day
days_diffboundaries where folding must match runtime. - Scope/clarity: The change is mostly focused, but the
days_difffolding change reusesto_dayssemantics where runtime uses full-day rounded-to-zero semantics. - Concurrency/lifecycle/config/compatibility/persistence: Not applicable; this is FE expression folding and tests only.
- Parallel paths: BE runtime
days_diffremains based on rounded full-day differences, so FE folding must stay aligned with it. - Tests/results: Added results are deterministic, but missing the boundary case that exposes this regression.
- Observability/performance: Not applicable; no new runtime observability or hot-path performance concern found.
- User focus: No additional user-provided focus points were supplied.
Please fix the folding implementation and add a regression case such as days_diff('2021-01-02 00:00:00', '2021-01-01 23:59:59'), which should fold to the same value as runtime evaluation.
| @@ -1177,29 +1207,43 @@ public static Expression hoursDiff(DateV2Literal t1, DateV2Literal t2) { | |||
Contributor
There was a problem hiding this comment.
This changes days_diff folding for DateTimeV2 from whole-day duration semantics to calendar-day semantics, which no longer matches BE runtime. BE evaluates days_diff via datetime_diff<DAY> and date_diff_in_days_round_to_zero_by_time, so days_diff('2021-01-02 00:00:00', '2021-01-01 23:59:59') returns 0 at runtime because the difference is less than one full day. With this new toDays(t1) - toDays(t2) folding it returns 1, so enabling constant folding changes query results. The same issue applies to the overloads below where either argument is DateTimeV2Literal; only the pure DateV2 case is safe to compute from day numbers.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29344 ms |
Contributor
TPC-DS: Total hot run time: 169204 ms |
linrrzqqq
force-pushed
the
fix-ttz-nullable
branch
2 times, most recently
from
d00c6ad to
60668cb
Compare
Collaborator
Author
|
run buildall |
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary:
No additional blocking issue found in this pass. I also considered the existing inline thread about days_diff DateTimeV2 folding and did not submit a duplicate comment.
Critical checkpoint conclusions:
- Goal/test coverage: The PR targets FE constant folding and nullable literal signature binding for date/time diff functions. The added regression cases cover nullable string literals, timestamp-tz literals, time zone changes, and fold-constant checks.
- Scope/clarity: The change is focused on FE Nereids expression folding/signature matching plus the corresponding regression outputs.
- Concurrency/lifecycle/config: No new concurrency, lifecycle-sensitive static initialization, or configuration item is introduced.
- Compatibility/protocol: No FE-BE protocol, storage format, or persisted metadata change is involved.
- Parallel paths: I compared the FE folding paths with BE FunctionTimeDiff behavior for date/datetime/timestamptz. The current helper logic matches BE round-toward-zero behavior for day/week/hour/minute/second/millisecond/microsecond paths, and timestamp-tz overloads use the UTC-normalized literal representation.
- Error handling/data correctness: No unchecked Status-like path applies in this FE Java code. I did not find a distinct constant-folding result mismatch beyond the already-known review context.
- Performance/observability: The added work is constant-time literal evaluation only; no new observability appears necessary.
- Tests: Regression tests are added in nereids_function_p0/scalar_function/D.groovy with output updates in D.out. I did not run the regression suite in this Actions review pass.
Focus points: The focus file says there were no additional user-provided review focus points.
Contributor
TPC-H: Total hot run time: 29271 ms |
Contributor
TPC-DS: Total hot run time: 169382 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
zclllyybb
reviewed
Jun 5, 2026
| } | ||
|
|
||
| private static int dateDiff(LocalDateTime date1, LocalDateTime date2) { | ||
| return ((int) ChronoUnit.DAYS.between(date2.toLocalDate(), date1.toLocalDate())); |
Collaborator
Author
There was a problem hiding this comment.
ChronoUnit 认为0000 年 2 月是闰年，但是我们内部的行为应该是认为0000年是平年, 所以常量折叠如果覆盖到了 0000.2， 就会多一天出来
| if (!argument.isNullLiteral() && argument.isLiteral() && realType.isStringLikeType()) { | ||
| realType = TypeCoercionUtils.characterLiteralTypeCoercion(((Literal) argument).getStringValue(), | ||
| Optional<Literal> literalAfterUnwrapNullable = ExpressionUtils.getLiteralAfterUnwrapNullable(argument); | ||
| if (!argument.isNullLiteral() && literalAfterUnwrapNullable.isPresent() && realType.isStringLikeType()) { |
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29364 ms |
Contributor
TPC-DS: Total hot run time: 170016 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
linrrzqqq
force-pushed
the
fix-ttz-nullable
branch
from
393436b to
0ac87a0
Compare
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29278 ms |
Contributor
TPC-DS: Total hot run time: 169681 ms |
924060929
approved these changes
Jun 9, 2026
924060929
changed the title
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
FE Regression Coverage ReportIncrement line coverage |
zclllyybb
approved these changes
Jun 9, 2026
Contributor
|
skip buildall |
Contributor
|
PR approved by anyone and no changes requested. |
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
4 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.
Problem Summary
Two issues in the FE constant folding of date/time
*_difffunctions(
days_diff,weeks_diff,hours_diff,minutes_diff,seconds_diff,milliseconds_diff,microseconds_diff). Both only affect FE folding — the BEresults were already correct — so
debug_skip_fold_constanton/off coulddisagree for the same query. (
datediff/to_daysalready fold viacalc_daynrand are not changed here; this PR only brings the remaining*_difffolding in line with them.)1. Nullable string/date literals were resolved to TIMESTAMPTZ unexpectedly
During signature search an argument is only type-coerced when
argument.isLiteral()is true. A literal wrapped innullable()/non_nullable()is a function node, not aLiteral, so the check fails: thedatetime coercion and the "no time zone → don't prefer TIMESTAMPTZ" scoring
are both skipped. With the scoring gone every candidate signature ties, and the
first one — the TIMESTAMPTZ overload — wins. So a plain, no-time-zone literal
that happens to be wrapped in
nullable()is cast totimestamptzinstead ofdatetimev2.The change unwraps
nullable()/non_nullable()so the inner literal iscoerced exactly like a bare literal. Genuinely timezone-bearing literals
(e.g.
'2021-12-31 00:30:00+08:00') still bind to TIMESTAMPTZ.before:
now:
2. Wrong day count when the range crosses year
0000The folding used Java
ChronoUnit.*.between(...). Java'sjava.timefollows theISO proleptic Gregorian calendar in which year 0 is a leap year
(
0000-02-29exists). Doris/BE use the MySQL-stylecalc_daynr, in which year 0is not a leap year. So whenever the interval crosses
0000-02-29, FE foldingwas exactly one day larger than the BE result.
The change folds these functions from
calc_daynr(DateLiteral.getTotalDays())for the day count, plus the sub-day remainder for the time-aware units —
matching
datediff/to_daysand the BE.TimestampTzLiteraloverloads areadded so timezone-bearing literals fold on the FE too.
before:
now: FE folding matches BE, both return
365.