[fix](function) Preserve subsecond precision in convert_tz DST gaps - #67493
Open
jacktengg wants to merge 1 commit into
Open
[fix](function) Preserve subsecond precision in convert_tz DST gaps#67493jacktengg wants to merge 1 commit into
jacktengg wants to merge 1 commit into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: CONVERT_TZ mapped nonexistent local times in daylight-saving gaps to the transition instant while explicitly discarding the DateTimeV2 fractional component. This made BE execution inconsistent with timezone-bearing casts, and FE constant folding had the same precision loss. Preserve and round the fractional component consistently in BE and FE. Because preserving fractions introduces resets at civil-second boundaries inside a gap, also disable the monotonic partition-pruning shortcut for affected positive-scale ranges.
### Release note
CONVERT_TZ now preserves DateTimeV2 fractional seconds when a source local time falls in a daylight-saving gap.
### Check List (For Author)
- Test: Unit Test and Regression test
- BE unit test: VTimestampFunctionsTest.convert_tz_test
- FE unit tests: DateTimeExtractAndTransformTest, ConvertTzTest
- Regression test: query_p0/sql_functions/datetime_functions/test_convert_tz
- Behavior changed: Yes. CONVERT_TZ preserves fractional seconds in DST gaps.
- Does this need documentation: No
jacktengg
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
September 3, 2026 10:27
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Requesting changes: the intended same-version convert_tz result is implemented consistently, but two P1 planner/executor compatibility paths can silently prune matching rows.
Critical checkpoints:
- Goal and proof: Both BE paths and FE folding preserve the already-rounded fraction for skipped local times, and the new semantic tests distinguish the old behavior. The goal is met only for same-version execution; the two inline findings block correctness across supported deployments.
- Scope: The production edits are focused on the two BE conversion sites, the shared FE resolver, and monotonicity certification.
- Concurrency and lifecycle: No new threads, locks, shared mutable state, static initialization dependency, or lifecycle ownership is introduced. The timestamp pair is row-local and cached timezone state is unchanged.
- Configuration: No configuration is added or changed.
- Compatibility: Blocking. FE-only timezone rules can disagree with independently loaded BE zoneinfo (MAIN-001), and the unconditional BE result change is unsafe during the documented BE-first rolling-upgrade interval (MAIN-002).
- Parallel paths: Row-varying BE execution, cached constant-zone BE execution, FE constant folding, timezone-bearing casts, and scale-0 behavior were all traced; both BE implementation sites changed together.
- Conditional logic: With identical timezone rules, the source-gap reset scan is boundary-correct for entry, intra-gap second resets, exit, exact endpoints, one-sided bounds, and target fallbacks. Its positive result is unsafe when FE and BE rule sets differ.
- Test coverage: Semantic coverage spans row-varying BE, cached BE, FE folding, scale-6 rounding, gaps, normal times, and overlaps. It does not exercise actual partition/runtime-filter pruning, FE/BE tzdata skew, or old-FE/new-BE execution; those missing end-to-end cases directly expose the accepted findings.
- Test results: The changed expected rows are deterministic and the generated output marker is present. I did not run builds or tests per the automated-review instructions. GitHub CheckStyle, Clang Formatter, license, and secret checks pass; BE UT, FE UT, compile, and performance jobs were still pending when reviewed.
- Observability: No additional logging or metrics are warranted for this deterministic conversion/planning change.
- Persistence, transactions, and data writes: No EditLog, metadata, storage-format, transaction, atomicity, or data-write path is touched.
- FE/BE fields and protocol: No new cross-process variable is added. Existing execution-version propagation has no
convert_tzcapability, alias, or guard for this semantic change, which is part of MAIN-002. - Error, null, range, and memory behavior: Existing invalid-zone/null/output-range handling is unchanged; the new pair is fully initialized and stack-local.
- Performance: No material BE hot-path regression was found. Conservative loss of pruning for some one-sided named-zone ranges is correctness-preserving without a shared timezone-version contract.
- Other issues: No additional substantiated issue remained after three review rounds and a final sweep of all eight changed files and both monotonicity consumers.
Review status: complete and converged after three rounds. Two distinct P1 findings are submitted inline; all other candidates were deduplicated or dismissed with code evidence.
User focus: no additional focus was provided, so the full PR was reviewed.
| if (upperDateTime.isBefore(lowerDateTime)) { | ||
| return false; | ||
| } | ||
| if (mayHaveFractionalSecondSourceGap(fromZone) |
Contributor
There was a problem hiding this comment.
[P1] Avoid FE-only timezone rules when certifying source-gap monotonicity
hasGapResetInRange can return false merely because the FE JVM's tzdb has no transition, but BEs load independently versioned TZDIR/system tzdata, so the resulting true is not safe. A released example is America/Asuncion: tzdb 2025a makes it permanent -03, while tzdb 2024b still has the 2025-10-05 00:00-to-01:00 spring gap. With a 2025a FE and 2024b BE:
Filter(convert_tz(ts,'America/Asuncion','UTC') >= '2025-10-05 04:00:00.500000')
Scan p: ts in [2025-10-05 00:00:00, 2025-10-05 01:00:00)
FE derives an image around [03:00,04:00] and can prune p, but under this PR's retained-fraction behavior the BE maps stored 00:00:00.500000 to 04:00:00.500000, so that row matches. RuntimeFilterPruneClassifier trusts the same certification. Unless FE and every BE share a versioned timezone-rules contract, please keep positive-scale named/non-fixed source zones conservatively non-monotonic and add an end-to-end pruning regression.
|
|
||
| ts_value2.from_unixtime(unix_timestamp_for_convert_tz(ts_value, from_tz), to_tz); | ||
| std::pair<int64_t, int64_t> timestamp; | ||
| ts_value.unix_timestamp(×tamp, from_tz); |
Contributor
There was a problem hiding this comment.
[P1] Preserve old-FE pruning semantics during BE-first upgrades
Doris upgrades BEs before FEs, so this unconditional result change runs while an old FE can still certify every fixed-target convert_tz as monotonic and fold source-gap endpoints with the old zero-fraction policy. With the same Europe/Paris rules on both sides, an old FE can project a DATETIMEV2(6) partition [2021-03-28 02:00:00, 2021-03-28 03:00:00) to the singleton 01:00:00 and prune it for convert_tz(ts,'Europe/Paris','UTC') > '2021-03-28 01:00:00.500000'. This new BE path maps a stored 02:30:00.900000 to 01:00:00.900000, so that pruned row actually matches.
Please stage this semantic change behind an old-FE/new-BE compatibility contract (for example, a query capability or versioned function implementation), and add a mixed-version pruning regression, so the supported rolling-upgrade window cannot drop rows.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 16786 ms |
Contributor
TPC-DS: Total hot run time: 81954 ms |
Contributor
ClickBench: Total hot run time: 14.68 s |
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
2 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.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: CONVERT_TZ mapped nonexistent local times in daylight-saving gaps to the transition instant while explicitly discarding the DateTimeV2 fractional component. This made BE execution inconsistent with timezone-bearing casts, and FE constant folding had the same precision loss. Preserve and round the fractional component consistently in BE and FE. Because preserving fractions introduces resets at civil-second boundaries inside a gap, also disable the monotonic partition-pruning shortcut for affected positive-scale ranges.
Release note
CONVERT_TZ now preserves DateTimeV2 fractional seconds when a source local time falls in a daylight-saving gap.
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)