[fix](TIMESTAMPTZ) serialize TIMESTAMPTZ schema change defaults with offset#63290
Open
Mryange wants to merge 3 commits into
Open
[fix](TIMESTAMPTZ) serialize TIMESTAMPTZ schema change defaults with offset#63290Mryange wants to merge 3 commits into
Mryange wants to merge 3 commits into
Conversation
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.
Review completed. The FE serialization change is focused and appears to address the schema-change TIMESTAMPTZ CURRENT_TIMESTAMP backfill path by materializing a type-aware realDefaultValue with timezone information. I did not find duplicate existing review context or any additional user focus points.
Critical checkpoint conclusions:
- Goal/test proof: The change targets schema change backfill for TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP and adds both FE unit coverage and a regression test, but the new regression test needs to comply with Doris regression-test standards before merge.
- Scope: The production code change is small and localized to default-value materialization/formatting.
- Concurrency/lifecycle: No new concurrency, locks, static initialization dependency, or lifecycle-sensitive state was introduced.
- Config/compatibility/protocol: No new config or FE-BE protocol/storage format compatibility change was introduced; the existing column default string sent to BE remains the mechanism.
- Parallel paths: Both Nereids schema-change and legacy ColumnDef materialization paths were considered; no additional functional path requiring the same fix was found in this PR.
- Tests/results: Coverage was added, but the regression test currently violates required test-case conventions.
- Observability: No new observability is required for this localized serialization fix.
- Transaction/persistence/data correctness: The change affects persisted Column.realDefaultValue for schema change; the materialized value is deterministic for existing-row backfill and does not introduce transaction or visible-version concerns.
- Performance: No hot-path or meaningful performance concern found.
Focus points: No additional user-provided review focus was present.
| // under the License. | ||
|
|
||
| suite("test_timestamptz_default_schema_change", "p0") { | ||
| def tableName = "test_timestamptz_default_schema_change" |
Contributor
There was a problem hiding this comment.
Doris regression-test standards in AGENTS.md require hardcoding table names for ordinary single-table tests instead of using def tableName. Please inline test_timestamptz_default_schema_change in the SQL strings, or use a literal only where dynamic naming is genuinely needed.
| } | ||
|
|
||
| sql "DROP TABLE IF EXISTS ${tableName} FORCE" | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
Doris regression-test standards require dropping tables before use, not after, so the failed-test environment is preserved for debugging. This suite already drops the table at the beginning; please remove the final DROP TABLE here.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary: no additional blocking code issues found in this pass. I did not duplicate the two existing inline threads about regression-test table-name hardcoding and removing the final drop; those remain already-known review context.
Critical checkpoint conclusions:
- Goal and proof: the PR targets TIMESTAMPTZ
CURRENT_TIMESTAMPreal-default formatting during schema change; the code passes the target type into schema-change default materialization and adds unit/regression coverage for the scenario. - Scope: the implementation is small and focused on default-value materialization plus formatting reuse.
- Concurrency/lifecycle: no new shared mutable state, locks, threads, or non-intuitive lifecycle/static initialization dependency were introduced.
- Configuration/compatibility: no new config items, protocol fields, storage formats, or rolling-upgrade compatibility paths are involved.
- Parallel paths: both legacy
ColumnDefschema-change conversion and NereidsColumnDefinition.translateToCatalogStyleForSchemaChange()paths were updated for TIMESTAMPTZ typed formatting. - Conditional checks: the new type checks are limited to TIMESTAMPTZ formatting selection and match the surrounding default-value logic.
- Tests: a FE unit test and a schema-change regression test cover the main positive case; existing inline comments already cover the remaining regression-test standards issues.
- Observability: no new runtime operation requiring logs or metrics is introduced.
- Transactions/persistence/data writes: no direct transaction or EditLog behavior changes were found; schema-change default materialization still flows through existing column metadata construction.
- Performance: no hot-path regression found; formatting happens during DDL/default materialization only.
User focus: no additional user-provided review focus was present.
Mryange
changed the title
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 31374 ms |
Contributor
TPC-DS: Total hot run time: 169649 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 30587 ms |
Contributor
TPC-DS: Total hot run time: 169226 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one blocking issue in the new TIMESTAMPTZ default serialization path. The implementation can emit a UTC offset as Z, which makes the added unit test fail under UTC/default-UTC test environments and also diverges from Doris TIMESTAMPTZ string output, which uses numeric offsets such as +00:00.
Critical checkpoint conclusions:
- Goal/test proof: the PR targets schema-change backfill for
TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP; coverage was added, but the FE unit test is currently timezone-dependent. - Scope/clarity: the code change is small and focused on default-value materialization.
- Concurrency/lifecycle: no new concurrency, locks, static initialization, or lifecycle-sensitive state was introduced.
- Compatibility/persistence: no storage format or edit-log schema changes were introduced; existing
Column.realDefaultValuesemantics are reused. - Parallel paths: both Nereids schema-change and legacy
ColumnDef.toColumn()paths were updated. - Tests: regression coverage was added; existing duplicate regression-style comments about table-name variables/final drop were already raised and are not repeated here. The UTC formatting issue below still needs a fix.
- Observability/performance: no new observability need; performance impact is negligible.
User focus: no additional user-provided review focus was specified.
| @@ -454,6 +455,14 @@ public static Expression fromJavaDateType(LocalDateTime dateTime, int precision) | |||
Contributor
There was a problem hiding this comment.
ZoneOffset.toString() returns Z for UTC, so when the FE default/session timezone is UTC this produces values like 2026-05-16 12:00:00.000000Z. The new unit test expects [+-]HH:MM, and BE-side TIMESTAMPTZ formatting also emits numeric offsets (+00:00), so this is timezone-dependent and will fail in UTC test environments. Please normalize UTC to +00:00 here, for example by formatting the offset with a formatter or replacing Z with +00:00.
Contributor
TPC-H: Total hot run time: 31331 ms |
Contributor
TPC-DS: Total hot run time: 168755 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
2 similar comments
Contributor
FE Regression Coverage ReportIncrement line coverage |
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.
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
Schema change could fail when adding a TIMESTAMPTZ column with DEFAULT CURRENT_TIMESTAMP, such as ALTER TABLE ... ADD COLUMN ts TIMESTAMPTZ(6) DEFAULT CURRENT_TIMESTAMP(6). Existing rows are backfilled from Column.realDefaultValue during schema change, but FE previously materialized CURRENT_TIMESTAMP as a plain datetime string without a timezone offset. Root cause: the schema-change default serialization path was not type-aware for TIMESTAMPTZ, while BE expects a TIMESTAMPTZ literal with an offset when parsing the backfill value. This change makes both the Nereids and legacy ColumnDef paths materialize TIMESTAMPTZ CURRENT_TIMESTAMP defaults with the session offset, reuses TimestampTzLiteral formatting for the datetime body, and adds FE and regression coverage for the schema-change path.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)