Conversation
yiguolei
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou and
liaoxin01
as code owners
September 21, 2026 08:02
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
csun5285
previously approved these changes
Sep 21, 2026
Contributor
There was a problem hiding this comment.
Review conclusion: request changes for one persisted-format compatibility gap.
Critical checkpoints:
- Goal and correctness: both current
ColumnMetaPB.default_valueproducers are removed, no current reader consumes the field, and schema-evolution defaults still come fromTabletColumn; the runtime removal itself is sound. - Scope and parallel paths: both top-level/vertical and variant subcolumn producers are covered. Static reconstruction of all 148 changed segment fixtures found 146 changed only by recursive tag-14 removal plus derived protobuf lengths/checksum; the other two only reorder unchanged partial-update page blocks along the test's documented normalization path.
- Persistence and rolling compatibility: old-writer/new-reader and new-writer/old-reader are currently readable, but tag 14 is not formally reserved and every old tag-bearing fixture was regenerated. The inline comment requests an enforced reservation and explicit V2-inline/V3-external legacy-read coverage.
- Concurrency, lifecycle, configuration, transactions, memory, errors, observability, and performance: this patch adds no concurrent state, locks, configuration, transaction flow, allocation ownership, or new error/observability path. Its intended performance effect is smaller segment metadata.
- Tests: the new-writer golden corpus spans 73 cases/148 segments, including V2/V3, horizontal/vertical, variant, row store, indexes, partial update, binlog, MoW/MoR, cluster and sequence-key paths. It no longer exercises legacy wire tag 14. No local build or test was run because the review environment forbids it; compile and BE/FE/Cloud CI were still pending at submission time.
- User focus: no additional focus was provided; the complete PR was reviewed.
Two full convergence rounds completed. Round 2's normal runtime, normal tests, and separate risk-focused scans all returned NO_NEW_VALUABLE_FINDINGS beyond the single merged comment.
|
|
||
| // Extra type info to be compatible with tabet schema | ||
| optional bytes default_value = 14; // ColumnMessage.default_value ? | ||
| // optional bytes default_value = 14; // ColumnMessage.default_value, this field is useless, it is added by https://github.com/apache/doris/pull/24170/ |
Contributor
There was a problem hiding this comment.
ColumnMetaPB is persisted in both V2 footers and V3 external metadata, and the pre-change corpus contains tag 14 2,437 times. Turning the declaration into a comment removes it from the descriptor, so a future field can legally reuse 14 and reinterpret old bytes. Please add reserved 14; (and reserve the old name "default_value" as schema hygiene).
Please also add explicit old-writer/new-reader coverage for both V2-inline and V3-external metadata, either with representative pre-change segments or targeted raw-tag injection. Regenerating all 148 goldens removes the only fixtures containing this tag, including non-empty and binary defaults. The runtime removal itself is otherwise compatible because readers source schema-evolution defaults from TabletColumn.
Contributor
TPC-H: Total hot run time: 28063 ms |
Contributor
TPC-DS: Total hot run time: 152870 ms |
Contributor
ClickBench: Total hot run time: 23.87 s |
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 27481 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 152285 ms |
Contributor
ClickBench: Total hot run time: 25.02 s |
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
4 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:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)