[fix](fe) Fix TIMESTAMPTZ dynamic partition boundaries not aligned to UTC midnight#65219
[fix](fe) Fix TIMESTAMPTZ dynamic partition boundaries not aligned to UTC midnight#65219starocean999 wants to merge 5 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-H: Total hot run time: 28634 ms |
TPC-DS: Total hot run time: 172449 ms |
ClickBench: Total hot run time: 25.03 s |
FE UT Coverage ReportIncrement line coverage |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review result: request changes.
Critical checkpoint conclusions:
- Goal/test proof: The PR targets TIMESTAMPTZ dynamic partition UTC boundaries, but the add path fix is incomplete because cleanup still uses local-zone cutoffs and
nowis sampled twice. The new tests do not reliably prove the fix because the hourly test fails across UTC midnight. - Scope/focus: Changed scope is limited to FE dynamic partition scheduler and FE unit tests; no additional user-provided focus was supplied.
- Concurrency/lifecycle: No new shared mutable state or lock order change found; the relevant lifecycle risk is durable partition names/ranges generated at scheduler time.
- Config/session/compatibility:
dynamic_partition.time_zonenow controls names while TIMESTAMPTZ ranges are UTC; this propagation is missing in dynamic cleanup. No wire/storage format compatibility change seen. - Parallel paths: Add-partition path was updated, dynamic drop path was not.
- Tests/validation: Static review only. Could not run FE tests because this runner lacks
thirdparty/installed/bin/protoc;git diff --checkpassed for the two live PR files.
Subagent conclusions: optimizer-rewrite produced OPT-002 and duplicate OPT-001; tests-session-config produced TSC-001/TSC-002. MRG-001, MRG-002, and MRG-003 are submitted as inline comments; OPT-001 was merged as duplicate of TSC-002. Convergence round 1 ended with both subagents reporting NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set.
| // nowTz: configured-timezone clock, used for naming and storage properties | ||
| ZonedDateTime nowTz = ZonedDateTime.now(dynamicPartitionProperty.getTimeZone().toZoneId()); | ||
| // now: used for border computation. For TIMESTAMPTZ, borders are UTC-based. | ||
| ZonedDateTime now = isTimestampTz ? ZonedDateTime.now(ZoneOffset.UTC) : nowTz; |
There was a problem hiding this comment.
This new UTC boundary handling also needs to be applied to the dynamic cleanup cutoff. getDropPartitionOpForDynamic() still builds its reserved range from ZonedDateTime.now(dynamicPartitionProperty.getTimeZone().toZoneId()) and then converts that local midnight with the configured timezone. With time_zone = Asia/Shanghai and start = -3, the add path creates UTC-day ranges, but the drop path reserves from 16:00:00+00:00, so the previous UTC-day partition still intersects the reserved range and is not dropped. Please derive the drop cutoff with the same TIMESTAMPTZ UTC clock/source-timezone logic as the add path.
|
/review |
There was a problem hiding this comment.
Automated review found two additional issues in the current head after accounting for the existing inline threads.
Critical checkpoint conclusions:
- Goal/test: the PR aims to make TIMESTAMPTZ dynamic partition boundaries UTC-aligned while keeping configured-timezone naming. Add/drop range generation is partially covered, but the current-partition identity used by auto-bucket still uses configured-timezone names while ranges use UTC.
- Scope: the change is focused on FE dynamic partition scheduling and FE unit coverage.
- Concurrency/lifecycle: scheduler op construction still happens under the existing table read-lock flow, followed by existing write-lock mutation paths; I did not find a new lock-order or lifecycle issue.
- Config/compatibility: no new config or persisted format was added. The dynamic_partition.time_zone behavior is compatibility-sensitive, and the accepted scheduler comment is about a remaining semantic mismatch in that behavior.
- Parallel paths: existing threads already cover independent add-path clock sampling and the earlier drop-cutoff mismatch. The new auto-bucket finding is a distinct downstream path using the same local-name/UTC-range split.
- Tests: the new tests cover day/week/hour and drop cutoff cases, but one exact-name assertion pattern can still flake at scheduler/test wall-clock boundaries, and there is no coverage for the auto-bucket current-partition mismatch.
- Validation: static review only. This checkout is missing
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc, so I did not run the FE build or FE unit tests. - User focus: no additional user-provided focus was supplied.
Subagent conclusions: OR-1 was accepted as M-1 and submitted inline; TSC-1 was accepted as M-2 and submitted inline. No subagent candidates were dismissed or merged as duplicates. Convergence round 1 ended with both optimizer-rewrite and tests-session-config returning NO_NEW_VALUABLE_FINDINGS for the same M-1/M-2 proposed comment set.
morrySnow
left a comment
morrySnow
left a comment
There was a problem hiding this comment.
Review Summary
This PR correctly fixes TIMESTAMPTZ dynamic partition boundaries to align to UTC midnight by separating boundary computation (UTC-based) from partition naming (configured timezone). The approach is sound and the test coverage is thorough.
Issue found: getClosedRange() still uses configured timezone for reserved history periods
The getDropPartitionOpForDynamic() method now uses borderTimeZone (UTC for TIMESTAMPTZ) for its main drop boundary computation (lines 579-582). However, the getClosedRange() method (called at line ~602 for reserved_history_periods handling, lines ~516-519 in the file) still passes dynamicPartitionProperty.getTimeZone() to convertToUtcTimestamp(). For TIMESTAMPTZ tables with non-UTC dynamic_partition.time_zone AND reserved_history_periods configured, the reserved history ranges are shifted by the timezone offset relative to UTC-midnight partition boundaries. This can cause partitions near period boundaries to be incorrectly dropped.
Recommendation: Propagate borderTimeZone to getClosedRange() (e.g., pass it as a parameter) so all boundary computations in getDropPartitionOpForDynamic() use the same effective timezone.
|
/review |
|
/review |
There was a problem hiding this comment.
Automated review completed.
I found one blocking compatibility issue: existing TIMESTAMPTZ dynamic partitions created before this change can overlap the new UTC-aligned ranges and block creation of the fixed partitions.
Critical checkpoints:
- Goal/correctness: fresh TIMESTAMPTZ dynamic partitions are moved toward UTC-aligned boundaries, but upgrade compatibility for existing shifted ranges is incomplete.
- Scope: the production change is focused on dynamic partition TIMESTAMPTZ boundary calculation and auto-bucket current-partition exclusion.
- Concurrency/lifecycle: no new thread or locking concern found in the reviewed scheduler paths.
- Config/compatibility: existing non-UTC TIMESTAMPTZ
dynamic_partition.time_zonetables are affected; the inline comment covers the missing migration/gating path. - Parallel paths: add/drop/reserved-history and auto-bucket paths were reviewed. Existing wall-clock test concerns are already covered by prior review threads and were not duplicated.
- Tests: added tests cover fresh UTC-aligned TIMESTAMPTZ behavior, but not the pre-fix shifted-partition upgrade state. I did not run FE tests because
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare absent in this checkout; I performed static review andgit diff --check. - User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite reported OPT-1 as a duplicate of MAIN-1. tests-session-config reported TEST-1 as a duplicate of MAIN-1 and TEST-2 as a duplicate of an existing wall-clock-after-scheduler test thread. Convergence round 1 ended with both subagents returning NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set.
| dynamicPartitionProperty.getTimeZone()); | ||
| nextBorder = convertToUtcTimestamp(partitionColumn, nextBorder, | ||
| dynamicPartitionProperty.getTimeZone()); | ||
| prevBorder = convertToUtcTimestamp(partitionColumn, prevBorder, borderTimeZone); |
There was a problem hiding this comment.
This fixes newly computed TIMESTAMPTZ ranges to UTC, but existing dynamic-partition tables that already have the old configured-timezone ranges will now block the fixed ranges. For example, before this change time_zone = 'Asia/Shanghai' created p20260706 as [2026-07-05 16:00:00+00:00, 2026-07-06 16:00:00+00:00). After upgrade this code wants [2026-07-06 00:00:00+00:00, 2026-07-07 00:00:00+00:00), which overlaps the old partition but is not equal. The overlap check below then sets isPartitionExists, records a create-partition failure, and continues, so the UTC-aligned partition is not created while the shifted partition remains. Please add an upgrade/compatibility path for existing TIMESTAMPTZ dynamic partitions, or gate the UTC-boundary behavior so existing tables do not get stuck with overlapping old ranges.
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Automated review found two blocking TIMESTAMPTZ dynamic partition issues.
Critical checkpoint conclusions:
- Goal/test proof: the PR moves new TIMESTAMPTZ dynamic partition boundaries and names to UTC and adds FE tests for several add/drop/reserved-history paths, but it does not prove upgrade behavior for tables that already have offset-aligned persisted ranges, and it does not cover non-UTC storage lifecycle properties.
- Scope/focus: the change is mostly focused on FE dynamic partition scheduling and auto-bucket history selection, but the remaining issues affect production correctness during upgrade and storage lifecycle timing.
- Concurrency/locking: I did not find a new locking issue; the reviewed scheduler logic still computes ops under the existing table read lock and applies add/drop operations through existing write-locked paths.
- Config/session propagation: session/JVM timezone paths were reviewed. The new add/drop/retention paths normalize TIMESTAMPTZ boundaries to UTC, but hot partition cooldown and storage-policy base time still use the configured-zone clock.
- Compatibility: existing TIMESTAMPTZ dynamic tables created by the old non-UTC logic can partially overlap new UTC candidates and block creation.
- Parallel paths: add, dynamic drop, reserved history, auto-retention, auto-bucket current partition exclusion, and tests were inspected; no other distinct issue remained after duplicate suppression.
- Tests/validation: static review only. This runner is missing thirdparty/installed and thirdparty/installed/bin/protoc, so FE build/tests were not run.
| dynamicPartitionProperty.getTimeZone()); | ||
| nextBorder = convertToUtcTimestamp(partitionColumn, nextBorder, | ||
| dynamicPartitionProperty.getTimeZone()); | ||
| prevBorder = convertToUtcTimestamp(partitionColumn, prevBorder, borderTimeZone); |
There was a problem hiding this comment.
Existing tables that have already been scheduled by the old code can get stuck on the shifted layout. The old TIMESTAMPTZ path with dynamic_partition.time_zone = 'Asia/Shanghai' persisted daily ranges such as [2026-07-05 16:00:00+00:00, 2026-07-06 16:00:00+00:00). After this PR, the same table tries to add [2026-07-06 00:00:00+00:00, 2026-07-07 00:00:00+00:00). This loop checks every existing range with RangeUtils.checkRangeIntersect; because the old range partially overlaps but is not equal, the catch records a create failure, sets isPartitionExists, and skips the new partition. The same happens while old future partitions overlap the new UTC candidates, and when the first non-overlapping UTC partition is finally added there can be a gap between the last old 16:00 upper bound and the next UTC midnight. Please add an explicit upgrade/compatibility path, or a migration test that starts from pre-fix offset-aligned TIMESTAMPTZ ranges and verifies scheduler behavior after the UTC change.
|
/review |
|
run buildall |
There was a problem hiding this comment.
Review summary:
I found one remaining issue in the newly added TIMESTAMPTZ hot-partition cooldown test coverage. The broader TIMESTAMPTZ dynamic-partition UTC-boundary work, AutoBucket range/name handling, drop cutoff, and retained-history handling were reviewed against the current head; the remaining production lifecycle and upgrade concerns I saw are already covered by existing review threads, so I did not duplicate them.
Critical checkpoint conclusions:
- Goal and proof: The PR targets TIMESTAMPTZ dynamic partition boundaries/names and related cleanup/lifecycle paths. The added tests cover several units and cleanup paths, but the new hot-partition cooldown test does not prove the finite expected cooldown boundary, so I left one inline comment.
- Scope: The code changes are focused on FE dynamic partition scheduling, AutoBucket historical partition selection, and FE tests.
- Concurrency and lifecycle: The reviewed scheduler path computes add/drop ops while holding the table read lock and applies mutations later under write locks, matching the surrounding pattern. I did not find a new concurrency hazard in the changed code.
- Config/session/compatibility: No new config was added. Existing concerns about pre-fix TIMESTAMPTZ partitions overlapping new UTC ranges are already covered by existing threads r3528082626 and r3528120739.
- Parallel paths: I checked dynamic add/drop, auto-partition retention cleanup, AutoBucket historical partition selection, and the
PartitionExprUtilAutoBucket caller. I did not find a new non-duplicate parallel-path issue. - Test coverage/results: I could not run FE tests in this checkout because
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare missing. Review validation was static. - User focus: No additional user-provided review focus was present.
Subagent conclusions:
optimizer-rewrite: ReportedOPT-DUP-1, which I merged as duplicate of the existing storage-lifecycle TIMESTAMPTZ thread. Convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.tests-session-config: ReportedTEST-001, accepted asMAIN-001and submitted inline, plusTEST-002, merged as duplicate of existing upgrade-compatibility threads. Convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.
|
|
||
| // Cooldown time must be at UTC midnight (aligned with | ||
| // partition boundaries), not offset by America/Chicago. | ||
| if (dp.getCooldownTimeMs() != DataProperty.MAX_COOLDOWN_TIME_MS) { |
There was a problem hiding this comment.
This new coverage still passes if the generated cooldown is missing or unparseable, because the UTC checks are skipped whenever dp.getCooldownTimeMs() is DataProperty.MAX_COOLDOWN_TIME_MS. That is exactly the fallback PropertyAnalyzer.analyzeDataProperty() uses after a storage_cooldown_time parse failure, so the test would not catch a regression where the TIMESTAMPTZ lifecycle string is rejected. For this setup with hot_partition_num = 1, each SSD partition should have a finite cooldown equal to that partition's upper endpoint (offset + hotPartitionNum). Please assert cooldownTimeMs != MAX_COOLDOWN_TIME_MS for every hot partition and compare the instant to the partition upper bound, rather than only checking that any finite value happens to be at UTC midnight.
3 participants
Related PR: #64795
Problem Summary:
When a table has a
TIMESTAMPTZpartition column anddynamic_partition.time_zoneis set to a non-UTC timezone (e.g.,Asia/Shanghai), the dynamically created partition boundaries are shifted to midnight in the configured timezone rather than UTC midnight.Root cause:
getAddPartitionOp()computed the current date (now) in the configured timezone, thenconvertToUtcTimestamp()interpreted the resulting date string as midnight in that timezone and converted it to UTC — producing a timezone-offset hour (e.g., 16:00) instead of UTC midnight (00:00). Both thenowcomputation and the timezone used inconvertToUtcTimestamp()needed to be fixed.Fix
In
DynamicPartitionScheduler.getAddPartitionOp(), decouple the time references for boundary computation and naming:Boundary computation: For
TIMESTAMPTZ, useZonedDateTime.now(UTC)for the current clock and pass UTC as the source timezone toconvertToUtcTimestamp(). This ensures partition boundaries are at UTC midnight (00:00–24:00) regardless ofdynamic_partition.time_zone.Partition naming: Keep using
ZonedDateTime.now(configured_timezone)for generatingprevBorderForName, so partition names remain identical to the pre-fix behavior. Thedynamic_partition.time_zoneproperty continues to control naming (e.g., week-of-year computation for week partitions).Storage properties:
setStorageMediumProperty()andsetStoragePolicyProperty()also use the configured-timezonenowTzso hot partition determination stays consistent with naming.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)