[fix](fe) Accept spaced TIMESTAMPTZ offsets in range partition bounds#63281
Open
starocean999 wants to merge 4 commits into
Open
[fix](fe) Accept spaced TIMESTAMPTZ offsets in range partition bounds#63281starocean999 wants to merge 4 commits into
starocean999 wants to merge 4 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
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 correctness regression in the TIMESTAMPTZ partition-boundary conversion path that can route/query data against the wrong UTC partition boundary when the partition DDL omits an explicit offset under a non-UTC session time zone. There is also a small FE test style issue that should be fixed before merge.
Critical checkpoint conclusions:
- Goal/test proof: The PR addresses spaced explicit TIMESTAMPTZ offsets in range partition bounds and adds FE/regression coverage for explicit offsets; however it does not preserve existing no-explicit-offset semantics, so the goal is only partially met.
- Scope/focus: The code change is small and focused in catalog partition key parsing.
- Concurrency/lifecycle/locks/config: No new concurrency, lifecycle, locking, or configuration concerns found.
- Compatibility/persistence: No storage/EditLog format change found; the concern is behavioral compatibility of existing TIMESTAMPTZ partition DDL.
- Parallel paths: Range LESS THAN and CREATE LIKE explicit-offset paths are covered, but the no-offset TIMESTAMPTZ path is a parallel existing behavior that now regresses.
- Test coverage: Added tests cover explicit offsets with spaces, but missing a non-UTC session case without an explicit offset that would catch this regression.
- Observability/performance: No additional observability or performance concerns found.
- User focus: No additional user-provided review focus was supplied.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review completed for PR 63281 at head 739adfc.
No additional blocking issues found beyond the already-known review threads. The latest commit restores TIMESTAMPTZ partition-bound handling for values without explicit offsets and fixes the FE test indentation issue, so I am not duplicating those prior comments.
Critical checkpoint conclusions:
- Goal and tests: The PR aims to accept TIMESTAMPTZ range partition bounds with explicit spaced offsets while preserving no-offset session-time-zone behavior. The current code and added FE/regression tests cover explicit offset create/create-like paths and no-offset session timezone conversion.
- Scope and clarity: The change is focused on partition-bound literal conversion and metadata round-trip formatting. The helper logic is local to
PartitionKey. - Concurrency and locking: No new shared mutable state, metadata locking, or concurrent lifecycle changes were introduced.
- Lifecycle/static initialization: No new static initialization dependency or lifecycle-sensitive object ownership was introduced.
- Configuration: No new configuration items were added.
- Compatibility: The change affects FE parsing/serialization of TIMESTAMPTZ partition bounds but does not introduce storage-format or FE-BE protocol changes.
- Parallel paths: Nereids partition definition conversion and legacy partition key parsing were both considered; the added
toLegacyLiteral()path preserves TIMESTAMPTZ UTC boundary strings for round trips. - Conditional checks: The explicit-time-zone branch is bounded to TIMESTAMPTZ parsing and preserves the old no-offset path.
- Test coverage: Added FE unit tests and a regression suite. Existing review comments covered missing no-offset regression coverage and indentation; both are addressed in the latest diff.
- Observability: No new runtime operational path requiring logs or metrics was introduced.
- Transactions/persistence/data writes: No transaction, edit-log, or data-write path changes were introduced; partition metadata formatting/parsing remains within existing DDL flows.
- FE review checks: No new FE locking, exception-boundary, or visible-version concerns found.
User focus: No additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 30922 ms |
Contributor
TPC-DS: Total hot run time: 169236 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one blocking issue in the current PR. The main goal is clear: keep TIMESTAMPTZ partition boundaries in UTC, preserving explicit offsets and session-time-zone behavior for no-offset values. The added tests cover explicit numeric offsets, no-offset session conversion, create-like, and auto partitioning, but they miss valid named-zone spellings.
Critical checkpoint conclusions: the change is mostly focused and small; no new concurrency, lifecycle, configuration, persistence, or FE-BE protocol paths are involved; data correctness is affected by the inline issue because valid TIMESTAMPTZ partition boundaries using named/lowercase zones can now be rejected; tests prove the numeric-offset and no-offset cases but do not cover named zones; observability is not a concern for this DDL parse path.
Existing review threads: I did not duplicate the earlier no-offset semantic regression or the checkstyle indentation thread.
User focus: no additional user-provided review focus was specified.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: no additional blocking issues found in the current PR head. The earlier inline concerns about no-offset TIMESTAMPTZ session-time-zone semantics, FE checkstyle indentation, and named/lowercase explicit time zones were already present in existing threads and appear addressed by the latest changes.
Critical checkpoints:
- Goal and proof: the PR aims to preserve/accept TIMESTAMPTZ range partition bounds across explicit offsets, generated DDL/create-like, session-time-zone no-offset values, named/lowercase zones, and auto partition boundaries. Added FE unit tests and a regression suite cover these scenarios.
- Scope and clarity: changes are focused on partition bound conversion/serialization paths and targeted tests.
- Concurrency/lifecycle: no new shared mutable runtime state, locks, threads, static initialization dependency, or lifecycle-sensitive object ownership found.
- Config/compatibility/protocol: no new config, storage format, or FE-BE thrift protocol fields found. Existing TIMESTAMPTZ metadata is normalized through current partition-key paths.
- Parallel paths: range partition creation, less-than syntax, create-like DDL round-trip, and auto partition creation were checked. I did not find another distinct TIMESTAMPTZ partition path requiring the same fix in this PR scope.
- Special checks: the explicit-time-zone detection is limited to choosing the existing TIMESTAMPTZ literal parser versus the existing session-time-zone conversion path; invalid inputs still fail during literal parsing.
- Tests: new unit/regression coverage is relevant and uses ordered query output. I did not run the test suite in this review environment.
- Observability: no new long-running or distributed runtime path requiring additional logs/metrics.
- Transactions/persistence/data correctness: no transaction processing or persistence log write/replay changes found; partition boundary correctness remains the key data-routing concern and is covered by the added tests.
- Performance: no hot-path repeated scans or heavy allocations beyond short literal parsing during DDL/partition creation.
- User focus: no additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31532 ms |
Contributor
TPC-DS: Total hot run time: 170545 ms |
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
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.
Problem Summary:
TIMESTAMPTZ range partition bounds with explicit offsets were still inconsistent across FE paths.
CREATE TABLE ... PARTITION BY RANGE ... VALUES LESS THAN ('2024-01-15 13:00:00 +00:00')failed because the raw bound string was reparsed by
PartitionKeyand rejected the space before thetimezone offset. This also overlaps with the existing TIMESTAMPTZ partition-boundary handling that
must preserve explicit UTC offsets during metadata reconstruction.
This PR normalizes spaced numeric timezone offsets in the TIMESTAMPTZ partition-key parsing path and
adds focused FE unit tests plus regression coverage for:
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)