[fix](be) Preserve dynamic defaults after schema change - #67334
Open
jacktengg wants to merge 2 commits into
Open
[fix](be) Preserve dynamic defaults after schema change#67334jacktengg wants to merge 2 commits into
jacktengg wants to merge 2 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Schema change evaluates dynamic column defaults once to obtain a fixed literal for historical-row backfill. FE previously sent only that literal to BE, so a partial update inserting a new key reused the schema-change timestamp instead of evaluating CURRENT_TIMESTAMP at statement time. Preserve the original default expression alongside the fixed backfill literal, persist both in tablet schema metadata, and use the expression only when partial update fills missing columns for a new row.
### Release note
Fix partial updates to evaluate dynamic defaults at statement time after columns are added through schema change.
### Check List (For Author)
- Test:
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.catalog.ColumnDefaultValueSerializationTest
- Manual test: Compiled the modified BE production and unit-test sources with the ASAN_UT compilation database
- Build: ./build.sh --fe -j 8
- Behavior changed: Yes. New rows inserted by partial update evaluate dynamic defaults at statement time while historical rows retain the fixed schema-change backfill value.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Add regression coverage for a partial update that inserts a new key after a schema change adds a DATETIMEV2(6) column with CURRENT_TIMESTAMP(6) as its default. The case preserves a historical row, waits two seconds after schema change, performs the partial insert, and verifies that the historical backfill and new-row default produce two distinct timestamps.
### Release note
None
### Check List (For Author)
- Test:
- Regression test: ./run-regression-test.sh --run -f regression-test/suites/unique_with_mow_p0/partial_update/test_partial_update_schema_change_current_timestamp_default.groovy
- Behavior changed: No
- Does this need documentation: No
jacktengg
requested review from
gavinchou,
liaoxin01 and
yiguolei
as code owners
August 31, 2026 07:42
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16898 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 83331 ms |
Contributor
ClickBench: Total hot run time: 14.77 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
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.
Automated review — changes requested
Review status: complete after two convergence rounds on head 9da15ebc2ba7ecd9a11069795859c49a1b6654a5. Two distinct P1 correctness issues are attached inline.
Critical checkpoint conclusions:
- Goal and proof: the local DATETIMEV2/CURRENT_TIMESTAMP path is implemented and covered by unit plus regression tests, but the goal is not complete because cloud shadow schemas lose the fixed backfill value and legacy DATETIME partial updates now consume an unevaluated expression.
- Scope and clarity: the patch is focused on dual default-value transport and persistence; no unrelated changes were found.
- Concurrency and lifecycle: no new threads, shared mutable state, lock ordering, static initialization, or ownership lifecycle is introduced.
- Configuration: no configuration item is added or changed.
- Compatibility and persistence: the appended optional Thrift/protobuf fields are wire-compatible; TabletColumn protobuf round-trip, equality, current-schema overlay, and schema-cache identity were traced without another issue. The cloud protobuf value mismatch is the remaining parallel-path defect.
- FE/BE variable passing and parallel paths: the local Thrift path carries the literal/expression pair, but the cloud protobuf path writes the expression into both fields.
- Conditions, error handling, and nullability: the new evaluator branches cover DATETIMEV2/TIMESTAMPTZ but omit reachable V1 DATETIME; NOT NULL writes fail and nullable writes can silently receive NULL.
- Tests and results: the new BE/FE tests are build-discovered and the regression result matches its query, but there is no cloud heavy-schema-change assertion or legacy-DATETIME fixed/flexible coverage. Builds and tests were not run because this review environment explicitly prohibits them.
- Observability, transactions, and data writes: existing schema-change and load diagnostics are sufficient; no new metric is required. The two inline issues nevertheless affect write correctness and historical backfill data.
- Performance and memory: no material CPU, allocation, or memory-safety concern was found beyond small per-column strings.
- Additional focus: no user-specific focus was supplied. A final sweep of all 13 changed paths and all candidates found no further distinct issue.
| for (unsigned int cur_cid : missing_cids) { | ||
| const auto& column = tablet_schema.column(cur_cid); | ||
| if (column.has_default_value()) { | ||
| const auto& default_value_expr = column.default_value_for_partial_update(); |
Contributor
There was a problem hiding this comment.
[P1] Evaluate legacy DATETIME before preferring the expression
FE still permits CURRENT_TIMESTAMP on DATETIME, and those V1 schemas remain reachable. Before this change the frozen timestamp literal parsed successfully; now default_value_for_partial_update() selects CURRENT_TIMESTAMP, but the branches below handle only DATETIMEV2/TIMESTAMPTZ. The V1 literal SerDe rejects the expression, so a NOT NULL partial update fails and a nullable column can silently become NULL. Please evaluate OLAP_FIELD_TYPE_DATETIME too and add legacy fixed/flexible coverage.
| if (column.getDefaultValue() != null) { | ||
| builder.setDefaultValue(ByteString.copyFrom(column.getDefaultValue().getBytes())); | ||
| } | ||
| if (column.getDefaultValueExprDef() != null) { |
Contributor
There was a problem hiding this comment.
[P1] Keep the frozen cloud backfill literal in default_value
For a schema-change Column, getDefaultValue() is CURRENT_TIMESTAMP(6) while getRealDefaultValue() is the DDL-time literal. createTabletMetaBuilder uses this protobuf for cloud shadow tablets, and SchemaChangeJob later parses new_column.default_value() as a literal. This stores the expression in both fields; DATETIMEV2 conversion swallows the invalid literal and substitutes MIN_DATETIME_V2, so historical rows get corrupted instead of receiving the DDL-time value. Please mirror ColumnToThrift for default_value and assert the frozen value in the protobuf test, with cloud heavy-schema-change coverage.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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: close #xxx
Related PR: #xxx
Problem Summary:
Schema change evaluates dynamic column defaults once to obtain a fixed literal for historical-row backfill. FE previously sent only that literal to BE, so a partial update inserting a new key reused the schema-change timestamp instead of evaluating CURRENT_TIMESTAMP at statement time. Preserve the original default expression alongside the fixed backfill literal, persist both in tablet schema metadata, and use the expression only when partial update fills missing columns for a new row.
Release note
Fix partial updates to evaluate dynamic defaults at statement time after columns are added through schema change.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)