Repository navigation
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: TIMESTAMPTZ day arithmetic changes UTC fields, so adding a day across a DST transition adds a fixed 24 hours and shifts the local clock. In an America/New_York session, adding one day to 2024-03-09 12:00:00-05:00 produces 13:00 instead of 12:00 the following day.
Apply DAY intervals to the session-local date in BE and FE constant folding, preserving fractional seconds, preferred offsets in overlaps, existing gap resolution, and date-range errors. Restrict DST-sensitive day arithmetic monotonicity and comparison rearrangement: moving a local-day interval across a comparison can otherwise return incorrect rows at a fall-back. Explicit hour arithmetic keeps elapsed-time semantics.
### Release note
TIMESTAMPTZ day addition and subtraction preserve session-local calendar time across daylight-saving transitions, allowing a day to represent 23 or 25 elapsed hours.
### Check List (For Author)
- Test: Regression test / Unit Test / Manual test
- Regression test: test_timestamptz_days_add_dst, test_timestamptz_dst_fold, test_timestamptz_dst_gap, test_timestamptz_utc_functions; all 4 suites passed. Expected results generated by the regression script; 7 FE/BE result groups match.
- Unit Test: 19 FE tests in TimestampTzLiteralTest and SimplifyArithmeticComparisonRuleTest; 10 BE tests in TimeStampTzValueTest and selected VTimestampFunctionsTest cases; all passed.
- Manual test: New York spring transition returns local 12:00 and 82800 seconds; adding 24 hours returns 13:00.
- ASAN BE build, FE build with Checkstyle, clang-format 16, build hygiene, and clang-tidy passed.
- Behavior changed: Yes. TIMESTAMPTZ DAY intervals use session-local calendar dates; unsafe monotonicity and inverse comparison rewrites are disabled for variable-offset zones.
- Does this need documentation: No. Existing date arithmetic bug fix; release note included.
jacktengg
requested review from
924060929,
englefly,
morrySnow,
starocean999 and
yujun777
as code owners
November 2, 2026 17:22
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: None Related PR: apache#70003 Problem Summary: The session-local intermediate of a valid UTC TIMESTAMPTZ can be February 29 in year 0. DateV2 rejects that date before applying a DAY interval, although FE uses a civil calendar and can produce valid UTC output. Use the same civil calendar in BE and validate the final UTC value. For example, UTC 0000-02-28 20:00:00 at +08:00 minus one day now consistently yields UTC 0000-02-27 20:00:00. A zero-day interval must also remain an infallible identity for valid UTC boundary values. Resolve zero before local-year validation in both FE and BE so existing predicate movement cannot introduce errors on rows excluded by a join. Add direct boundary unit tests, year-0 projection regressions, and a zero-day join regression with runtime filters disabled. ### Release note Keep TIMESTAMPTZ day arithmetic consistent between FE and BE at year-0 calendar boundaries and preserve zero-day identity at valid UTC boundaries. ### Check List (For Author) - Test: Regression test / Unit Test - Unit Test: 11 selected BE tests and 20 FE tests passed using run-be-ut.sh and run-fe-ut.sh. - Regression test: test_timestamptz_days_add_dst, test_timestamptz_dst_fold, test_timestamptz_dst_gap, test_timestamptz_utc_functions; all 4 suites passed. Expected output generated by the regression script; 10 FE/BE groups match. - ASAN BE build, FE build with Checkstyle, clang-format 16, build hygiene and clang-tidy passed. - Behavior changed: Yes. Valid UTC boundaries retain zero-day identity; civil calendar intermediate dates no longer cause FE/BE divergence. - Does this need documentation: No. Boundary correctness fixes for existing date arithmetic.
Contributor
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#70003
commit: 4e4a662c1355be835d5a4e6437bb3e9847d48ed8
base: 4bc7f426828e4cedfd2b7ce9b76fe95b14b5b146
reviewed_at: 2026-11-03T02:23+08:00
reviewer: jacktengg
model: gpt-6-astra
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers None. Reviewed locally with the |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28599 ms |
Contributor
TPC-DS: Total hot run time: 150786 ms |
Contributor
ClickBench: Total hot run time: 24.33 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
HappenLee
reviewed
Nov 10, 2026
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: The date comparison comment omits the session timezone, complete dates, and the reason for passing null range bounds to isMonotonic. Describe the ordering requirement and show how the New York autumn transition changes the comparison result after interval rearrangement. Use short descriptive sentences and consistent technical terms.
### Release note
None
### Check List (For Author)
- Test: No need to run functional tests because only comments changed.
- Targeted Maven Checkstyle passed with zero violations.
- Java token comparison confirmed that executable code is unchanged.
- Verified the example's UTC conversions and opposite Boolean results.
- Comment sentences have at most 18 words; all lines fit the 120-character limit.
- Behavior changed: No
- Does this need documentation: No. This change updates the source comment.
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Each nonzero TIMESTAMPTZ DAY operation calls the local-to-UTC
conversion overload with a preferred offset. This overload queried the civil
time and then called cctz::convert, which repeated the same lookup. Initialize
the time point from the existing civil_lookup, using trans for SKIPPED times
and pre otherwise, to reduce these civil-time lookups from two to one per
conversion. Keep the REPEATED preferred-offset selection and fractional seconds
unchanged. This preserves gap and overlap semantics. No end-to-end SQL
performance improvement is claimed.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter='TimeStampTzValueTest.*' -j64 (ASAN; all 9 tests passed)
- build-support/clang-format.sh and build-support/check-format.sh (clang-format 16)
- build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (passed; toolchain resource directory supplied)
- git diff --check
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#70204 Problem Summary: TIMESTAMPTZ day arithmetic clamped a target local time in a DST gap to the transition instant. In America/New_York, adding one day to 2024-03-09 02:30:00.123456-05:00 produced 03:00:00.123456-04:00 instead of 03:30:00.123456-04:00 on March 10. Initialize the preferred-offset local-to-UTC conversion from the existing civil lookup's pre value, aligning its gap policy with apache#70204 while retaining a single civil-time lookup and the overlap offset selection. Align FE DAY constant folding with the same policy through ZonedDateTime.ofLocal. Update unit and regression coverage for adding and subtracting a day into a gap, and disable SQL caching in the FE/BE comparison. Regenerate the regression output with the test runner. ### Release note TIMESTAMPTZ day arithmetic shifts nonexistent local times forward by the DST gap length and preserves fractional seconds in both FE and BE. ### Check List (For Author) - Test: Unit Test / Regression test - ASAN BE and FE build passed via build.sh --be --fe -j64 - run-be-ut.sh: all 9 TimeStampTzValueTest tests passed - run-fe-ut.sh: all 17 TimestampTzLiteralTest tests passed - run-regression-test.sh: test_timestamptz_days_add_dst and test_timestamptz_dst_fold passed - DAY regression passed with SQL caching disabled; its .out file was generated by the runner - clang-format 16, header hygiene, clang-tidy, FE Checkstyle and git diff --check passed - Behavior changed: Yes. Gap targets retain their position and fraction instead of clamping to the transition. - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of all 12 changed files at 633f33a found two P2 correctness issues, reported inline: the shared gap resolver can move existing floor results after their input, and fixed-offset DAY comparison rearrangement can introduce an invalid inverse into an otherwise valid query.
Critical checkpoint conclusions:
- Goal and behavior: session-local DAY arithmetic preserves clock time across DST in BE and FE, including overlap preference, fractional seconds and zero-day identity; the two inline issues remain.
- Scope and reuse: changes are focused and reuse the date-function framework; the shared resolver change also affects existing rounding callers (first finding).
- Concurrency and locks: no new concurrent mutation, thread entry, lock or lock-order change; timezone state follows the existing query-context lifetime.
- Lifecycle and initialization: new calculations use scalar locals and existing timezone objects; no new resource lifetime or cross-translation-unit initializer dependency.
- Configuration: no new configuration; the existing session timezone reaches the BE RuntimeState and FE folding paths.
- Compatibility: no function-name, result-type, precision, wire or storage-format change. DAY result semantics intentionally change, as stated in the release note; FE/BE versions should be aligned when checking results during an upgrade.
- Thrift/protobuf: no changes under gensrc; field-ID review is not applicable.
- Parallel paths: checked days_add/days_sub and DATE_ADD/DATE_SUB/ADDDATE/SUBDATE binding, const/vector/nullable dispatch, FE folding, comparison inversion and both monotonicity consumers. HOUR remains elapsed arithmetic; WEEK and compound intervals retain their existing implementation.
- Conditions and errors: new zero, local-year and DST guards have concrete comments; range failures propagate through existing exception boundaries. The optimizer needs the inverse-validity check described in the second finding.
- Test coverage: inspected BE value/dispatch tests, FE literal/rewrite tests and the new regression suite for both DST directions, gaps, overlaps, NULLs, precision, UTC boundaries and year-zero intermediates. The reported floor and invalid-inverse cases are missing.
- Expected results: manually checked every new golden output against its query and intended DAY policy; FE/BE groups agree. The author reports script generation; generation provenance was not executed or independently reproduced here.
- Observability: existing date-range error reporting remains appropriate; no new distributed operation requires metrics or logs.
- Transactions and persistence: no EditLog, replay, failover, visible-version or delete-bitmap changes.
- Writes and atomicity: no storage-write path, transaction-atomicity or crash-recovery changes.
- FE/BE variables: no new serialized variable or protocol field; the existing session timezone is used.
- Performance and memory: the previously reported redundant civil lookup is removed; no new owned buffer or obvious avoidable allocation in the BE row loop. No performance benchmark was run.
- Additional checks: reviewed preferred-offset helper callers, fixed-offset inversion, special calendar bounds, header/include and test conventions; no other verified issue or remaining static scope gap.
Existing review context: discussion 4524950284 and its reply were read. Its duplicate-lookup concern is addressed on this head and is not reposted; the first finding describes a distinct result-correctness regression. No existing P0/P1 blocker was identified. User focus: no additional focus was supplied.
Validation was static inspection only. No builds, tests, benchmarks, source edits or submodule initialization were performed.
Review converged after 2 round(s).
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: In a +08:00 session, rewriting days_add(ts, 1) <
TIMESTAMPTZ '9999-12-31 23:00:00+00:00' introduces days_sub(cutoff, 1).
The cutoff is valid UTC but has session-local year 10000, so its inverse
fails even though the original comparison can evaluate successfully.
The fixed-offset monotonicity check does not establish that the inverse
is valid, and failed FE folding leaves the invalid call for BE execution.
Fold the candidate inverse for TIMESTAMPTZ DAY addition and subtraction,
and require a TimestampTzLiteral before rearranging the comparison. Keep
the original predicate when folding cannot produce a valid literal, and
reuse the literal when it succeeds. Cover local and UTC year boundaries,
positive and negative intervals, zero-day identities, and valid inverses.
### Release note
Prevent optimizer-introduced out-of-range errors in TIMESTAMPTZ DAY
comparisons near timezone and year boundaries.
### Check List (For Author)
- Test: Unit Test / Regression test / Manual test
- 22 FE unit tests passed: SimplifyArithmeticComparisonRuleTest and TimestampTzLiteralTest.
- Regression suites test_timestamptz_day_predicate_boundary and test_timestamptz_days_add_dst passed.
- Generated the new regression output with run-regression-test.sh -forceGenOut, then compared it in a normal run.
- Reproduced the original out-of-range failure and verified the fixed plan retains the original DAY comparison.
- FE build via build.sh --fe and Checkstyle passed.
- git diff --check passed with the generated .out trailing blank separator allowed.
- Behavior changed: Yes; preserve valid DAY comparisons when their inverse cannot be evaluated safely.
- Does this need documentation: No
This reverts commit 3b6bca3 in preparation for cherry-picking cd42918 from PR apache#70556. ### What problem does this PR solve? Issue Number: N/A Related PR: apache#70556 Problem Summary: Replace the TIMESTAMPTZ DAY-only inverse validation with the shared date arithmetic validation from PR apache#70556. The replacement covers all date arithmetic functions in the rearrangement map, applies type coercion, and validates the inverse exclusively on FE. Keep the existing DST monotonicity guard from earlier commits. ### Release note None; this preparatory revert is followed by the broader fix. ### Check List (For Author) - Test: Manual test; inspected the reverse patch and replacement code. Run unit and regression tests after applying the replacement. - Behavior changed: Yes; this intermediate commit removes the narrow inverse guard before its replacement. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#70556 Problem Summary: Moving date arithmetic across a comparison can introduce an inverse operation outside the date type's range even when the source operation is valid. Fixed-offset TIMESTAMPTZ sessions can also have a valid UTC cutoff whose local year is outside the supported range. Coerce and evaluate each candidate date inverse on FE, accepting only a DateLiteral and reusing it in the rewritten comparison. Preserve the original comparison for invalid, NULL or nonconstant boundaries. The shared guard covers week, day, hour, minute and second addition and subtraction across the supported date types. Preserve this branch's existing DST monotonicity guard. Resolve import conflicts and explicitly use UTC in the two imported boundary unit tests so their DAY assertions exercise inverse validation in a fixed-offset session. No additional production changes were needed for integration. ### Release note Prevent date comparison errors caused by optimizer-generated arithmetic that exceeds the date type's range. ### Check List (For Author) - Test: Unit Test / Regression test / Manual test - 22 FE tests passed via run-fe-ut.sh: SimplifyArithmeticComparisonRuleTest and TimestampTzLiteralTest. - FE build via build.sh --fe and Checkstyle passed. - Six regression suites passed: test_date_comparison_boundary, test_simplify_arithmetic, test_simplify_comparison_predicate, test_date_arithmetic_rejecting_boundary, test_timestamptz_day_predicate_boundary, and test_timestamptz_days_add_dst. - Temporarily restored the reverted boundary suite to verify all 12 original queries, then removed the temporary copies. - Verified the original +08:00 boundary query returns its row and retains days_add in EXPLAIN with BE constant folding enabled. - git diff --check passed with the generated .out trailing separator allowed. - Behavior changed: Yes; reject unsafe date-comparison rearrangements while preserving valid inverse optimizations. - Does this need documentation: No (cherry picked from commit cd42918)
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of all 14 changed files at c158d92 found no additional issues to report inline. The previously reported inverse-range issue is addressed. The existing P2 gap-rounding concern still applies to this exact head and is preserved below without a duplicate comment.
Critical checkpoint conclusions:
- Goal and proof: BE and FE apply TIMESTAMPTZ DAY intervals to the session-local calendar, preserving fractions, overlap preference and zero-day identity. Paired unit and regression cases cover spring/fall and both directions. The existing shared-rounding concern remains.
- Scope and reuse: the change reuses the date-function framework, type coercion and FE constant folder. The inverse-validity guard consistently covers the mapped date units and types. The shared conversion helper also affects existing rounding callers, as already reported.
- Concurrency and locks: no new thread, shared mutable state, lock or lock-order change. FE reads the existing thread-local session; BE reads the existing query RuntimeState timezone.
- Lifecycle and initialization: new civil/time values are local intermediates. No new resource ownership, reference cycle, global initialization dependency or teardown requirement.
- Configuration: no new configuration. Existing session timezone reaches FE folding and every BE dispatch shape.
- Compatibility: no function-name, result-type, precision, storage or wire-format change. DAY results intentionally change across timezone transitions, as documented in the release note; aligned FE/BE versions are needed for uniform new semantics. No new libc API or linker dependency is introduced; final ELF ABI was not audited in this static review.
- Thrift/protobuf: no gensrc definitions changed; field-ID review is inapplicable.
- Parallel paths: checked days_add/days_sub, DATE_ADD/DATE_SUB/ADDDATE/SUBDATE, INTERVAL DAY and timestampadd binding, FE folding, all constant/vector/nullable combinations, partition-bound evaluation and runtime-filter monotonicity. HOUR remains elapsed arithmetic; WEEK and compound intervals retain their consistent existing UTC paths.
- Conditions and errors: zero-day, local-year, DST-monotonicity and inverse-validity guards have concrete explanations. Only inverses that fold on FE to a DateLiteral are accepted; NULL, column boundaries, range failures and unfolded expressions preserve the original comparison. Existing date-range exception propagation remains intact.
- Test coverage: inspected both new BE unit-test blocks, FE literal/rewrite tests and both regression suites. Coverage includes DST gaps/overlaps, negative/zero days, fractions, NULLs, UTC bounds, year-zero local intermediates, nonmonotonic predicates, comparison types/operators/signs, column boundaries, recursive inverses and BE-fold mode. The existing rounding gap scenario remains absent from the added tests; non-hour transitions and installed timezone-database parity were assessed statically rather than executed.
- Expected results: every new golden-output group agrees with its query and the inspected arithmetic/NULL semantics, including 82800/90000-second transitions, overlap offsets, shifted gaps, lexical order_qt ordering and boundary comparison truth values. The author reports script generation; generation provenance was not independently reproduced.
- Observability: existing date-range errors provide input and interval context. Pure scalar arithmetic adds no distributed operation needing new metrics or logging.
- Transactions and persistence: no EditLog, replay, failover, visible-version or delete-bitmap path changes.
- Writes and atomicity: no production storage-write or crash-recovery changes. Source columns are read only and results use newly owned columns.
- FE/BE variables: no new serialized field. The existing RuntimeState timezone is forwarded through the generic date-operation wrapper, including constant handling.
- Performance and memory: the previously reported duplicate civil lookup is removed. New BE arithmetic uses fixed-size local intermediates and required timezone lookups; no new owned buffer, per-row column copy or untracked container was introduced. No benchmark was run.
- Additional sweep: checked all preferred-offset helper callers, source and target calendar limits, final UTC validity, historical second offsets, subtraction overloads, unrelated plusDays callers, alias registration, monotonicity consumers, include/header conventions and all changed paths. No additional verified issue or unresolved static coverage gap remains.
Existing review context and user focus:
- P2 gap-rounding thread: independently confirmed still applicable. The current HOUR/MINUTE floor code computes a local civil grid and calls the changed preferred-offset converter. In New York, the reported hour_floor example still resolves skipped 02:30 to 03:30, later than the 03:15 input. The author's reference to follow-up PR #70204 does not change this head. This concern is already covered and is not reposted.
- P2 inverse-range thread: addressed by the new FE-only inverse-fold and DateLiteral gate, including the +08:00 source-local-year overflow scenario. The original predicate is retained when the inverse is invalid.
- Duplicate-lookup thread: addressed by reuse of the existing civil lookup.
- No existing P0/P1 inline finding was identified. No additional user-provided focus was supplied.
Validation was static inspection only. No builds, tests, benchmarks, source edits or submodule initialization were performed. Runtime behavior, installed timezone-database agreement, golden-file generation and final artifacts were not independently executed or validated. Three reviewers returned NO_NEW_VALUABLE_FINDINGS in round 1; all candidates and risk items have evidence-backed dispositions.
Review converged after 1 round(s).
Contributor
Author
|
run buildall |
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
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.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
TIMESTAMPTZ day arithmetic currently changes UTC fields, so adding one day across a DST transition adds exactly 24 hours and shifts the local clock. In an America/New_York session,
days_add(CAST('2024-03-09 12:00:00-05:00' AS TIMESTAMPTZ(6)), 1)produces 13:00 the following day instead of 12:00.Apply DAY intervals to the session-local date in BE and FE constant folding, then resolve the result back to UTC. Use a civil calendar intermediate consistent with FE, including local February 29 in year 0 when both UTC endpoints are valid. Preserve fractional seconds, the input offset for repeated local times, existing cctz gap resolution, and date-range errors. Zero-day intervals remain an identity at valid UTC boundaries so predicate movement remains safe. This covers days_add/days_sub and their SQL aliases; hours_add remains elapsed-time arithmetic.
Local-day arithmetic can reverse UTC ordering at a fall-back and is not always invertible. Restrict its monotonicity declaration in variable-offset zones and prevent comparison rearrangement from replacing a DAY predicate with an invalid inverse operation. For example, moving the interval in
days_add(ts, 1) < '2024-11-04 01:30:00-05:00'can incorrectly include the first 01:50 from the preceding fall-back day.Release note
TIMESTAMPTZ day addition and subtraction now preserve session-local calendar time across daylight-saving transitions. One day may represent 23 or 25 elapsed hours; explicitly adding 24 hours retains elapsed-time semantics.
Check List (For Author)
Check List (For Reviewer who merge this PR)