[refactor](pb) remove default value from column meta pb - #68323
Merged
Merged
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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
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
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27744 ms |
Contributor
TPC-DS: Total hot run time: 152694 ms |
Contributor
ClickBench: Total hot run time: 23.88 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
/review |
csun5285
previously approved these changes
Sep 22, 2026
gavinchou
reviewed
Sep 22, 2026
|
|
||
| // 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.
reserve this field instead commenting
gavinchou
approved these changes
Sep 22, 2026
gavinchou
approved these changes
Sep 22, 2026
Contributor
|
/review |
Contributor
|
run buildall |
Contributor
|
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. |
Contributor
|
skip buildall |
Contributor
|
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. |
Contributor
TPC-H: Total hot run time: 28055 ms |
Contributor
TPC-DS: Total hot run time: 152636 ms |
Contributor
ClickBench: Total hot run time: 23.88 s |
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
6 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)