Repository navigation
[refine](storage) Strengthen enum type safety in storage code - #69287
Merged
yiguolei merged 1 commit intoOct 22, 2026
Merged
Conversation
Mryange
requested review from
airborne12,
csun5285,
deardeng,
eldenmoon,
gavinchou,
liaoxin01,
luwei16 and
yiguolei
as code owners
October 19, 2026 12:13
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.
Reviewed all 102 changed paths at f8f18a6 using the supplied aggregate diff, the repository review skill, and the applicable AGENTS.md guides. No actionable introduced defect was found in this static review. There were no existing inline threads to carry forward and no additional user focus points.
The review followed the complete binlog path (historical operation derivation, Int64 persistence, DETAIL/MIN_DELTA output), tokenizer dictionary construction and consumption, RocksDB column-family access, and local/Cloud tablet and compaction consumers.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and evidence | Scoped enums and typed fields replace implicit integer mixing. Existing unit cases were adapted for binlog codes, invalid operations, historical retrieval, metadata access, scan ranges and Kuromoji categories; runtime proof was not obtained in this static-only invocation. |
| Scope and clarity | The broad caller updates are necessary for the enum changes. Small shared headers isolate binlog and trigger types; no unrelated functional change identified. |
| Concurrency and locks | No threads, lock acquisitions, critical sections or atomic ordering changed. Tablet/rowset and schema-change state checks retain their original conditions and lock scope. |
| Lifecycle and initialization | Rowset acquire/release and delayed close transitions, dictionary ownership and tokenizer resets retain their behavior. New compatibility constants are constexpr; no new cross-TU initialization dependency found. |
| Configuration and dynamic updates | No configuration items added or changed. |
| Symbols, storage and upgrade compatibility | Persisted binlog values and int64 width, uint8 Kuromoji category ordinals, explicit protobuf tablet-state mappings and file-header versions remain stable. Compaction/tablet compatibility aliases preserve unchanged Cloud consumers; compaction ostream output is retained. No externally exchanged structure changed. |
| Thrift/protobuf definitions | No gensrc changes. |
| Parallel and special-case paths | Reviewed local/Cloud callers, manual/automatic/load-triggered compaction, plain/MoW/fixed/flexible binlog derivation, DETAIL/MIN_DELTA and IK/Kuromoji/pinyin/ngram/standard tokenizers. No missed migration found. |
| Conditions and error propagation | Changed comparisons preserve their predicates. Invalid/null binlog fallback, dictionary category validation and page-size checks remain equivalent; no introduced silent failure found. |
| Test coverage | Inspected all changed tests and relevant unchanged consumers. Positive and negative operation/category checks remain intact; no new runtime behavior requiring a separate feature test identified. |
| Expected test results | Assertions retain their expected values with enum qualifiers or explicit casts. No regression output files changed. |
| Observability | Numeric logging and compaction trigger labels are preserved; this refactor adds no execution path needing new metrics. Enum formatting is supported by the pinned fmt 7.1.3 argument mapper. |
| Transactions and persistence | Tablet state serialization/recovery and column-family ordinals remain equivalent; transaction prepare/commit/publish behavior is unchanged. No FE EditLog change. |
| Writes, atomicity and crash recovery | Binlog operation selection and emitted numeric values remain unchanged; no added write ordering, durability or cleanup step. No new crash window identified. |
| FE/BE variable transport | No transported variables or protocol fields added. |
| Performance and memory safety | Enum storage widths in persisted/category paths remain fixed; no added allocations, copies or scans. Page header constants remain 4/16 bytes and positive type-size arithmetic preserves normal page bounds. |
| Other/module obligations | Header layering, include budgets, PCH whitelist, extern-template pairing and unity-skip checks passed using the read-only build-hygiene script. No dependency/linker changes or new libc API use; release ELF compatibility was not audited. |
Validation and limits: static source/call-chain inspection plus bash build-support/check-build-hygiene.sh; no build, unit test, regression test or runtime validation was performed, as required by the review contract. The PR author's build claim was not independently reproduced. These execution limits are distinct from convergence of the static review.
Convergence: all three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS in round 1 on the unchanged empty finding set. All changed paths and initial risk items have evidence-backed dispositions; no unresolved static question or uncovered changed path remains.
Review converged after 1 round(s).
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28495 ms |
Contributor
TPC-DS: Total hot run time: 151014 ms |
Contributor
ClickBench: Total hot run time: 24.27 s |
Contributor
Author
|
run buildall |
HappenLee
previously approved these changes
Oct 20, 2026
HappenLee
left a comment
HappenLee
left a comment
Contributor
There was a problem hiding this comment.
LGTM. The scoped-enum / typed-field refactor is mechanical and well-scoped; persisted values and Cloud compatibility are preserved. Approved.
Contributor
TPC-H: Total hot run time: 28663 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 150089 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
ClickBench: Total hot run time: 24.31 s |
yiguolei
previously approved these changes
Oct 21, 2026
csun5285
previously approved these changes
Oct 21, 2026
Issue Number: N/A Related PR: apache#69174 Problem Summary: Use scoped enums and typed fields for storage states, character categories, compaction triggers, and binlog operations to prevent implicit integer conversions and mixing unrelated values. Preserve persisted values and Cloud source compatibility; adapt callers and existing tests. None - Test: ASAN BE build, header hygiene, and diff whitespace checks passed. Existing unit tests were adapted but not run. - Behavior changed: No - Does this need documentation: No
Mryange
force-pushed
the
refine-be-enum-types-other-modules
branch
from
October 21, 2026 08:09
f8f18a6 to
4052eba
Compare
Contributor
|
run buildall |
yiguolei
approved these changes
Oct 21, 2026
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR #69287 at 4052eba: no new actionable findings and no existing inline findings to carry forward. All 102 changed paths were covered (71 storage/execution/test paths and 31 tokenizer/dictionary/test paths), with a separate review of the three initial risk areas. All three reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty inline set. The review converged in 1 round; no unresolved code candidates or source-coverage gaps remain.
Critical checkpoint conclusions:
- Goal and evidence: the scoped-enum migration preserves the examined storage and tokenization behavior. Existing unit tests were adapted; the author reports an ASAN BE build, while unit tests were not run. This review performed static analysis only.
- Scope and clarity: the broad consumer updates are required by the type migration; the two new headers contain only lightweight enum definitions. No unrelated functional change found.
- Concurrency: existing scanner, compaction, tablet, transaction and schema-change synchronization is unchanged; no new threads, lock ordering, blocking operations or heavy critical-section work found.
- Lifecycle and initialization: rowset load/close/release, tablet shutdown and dictionary ownership remain equivalent. New compatibility constants use constant initialization; no new cross-translation-unit dynamic initialization dependency.
- Configuration: none added or changed; dynamic reconfiguration is unaffected.
- Compatibility: changelog BIGINT operation values, file-header versions, RocksDB column-family slots, tablet protobuf mappings and Kuromoji bytes remain stable. Cloud compatibility aliases and numeric stream output preserve unchanged callers. Direct enum formatting is supported by the repository-pinned fmt 7.1.3 enum mapper. No dependency or linker changes introduce a new libc requirement; release artifacts were not built or audited.
- Thrift/protobuf definitions: no gensrc changes, so cross-branch field-ID review is not applicable.
- Parallel paths: checked local/Cloud compaction and tablet callers, plain/fixed/flexible changelog writes, DETAIL/MIN_DELTA reads, all changed tokenizer families, and dictionary producer/reader paths. No missed corresponding update found.
- Conditions and errors: changed conditions retain the same enum meanings; invalid/null operation fallbacks and page-header guards are preserved. No new swallowed error or speculative recovery branch found.
- Test coverage: existing tests retain the operation transition matrix, invalid/extreme operations, pending batches, cross-rowset folding, partial updates, historical deletes, dictionary round trips and invalid-category rejection. Builds and tests were prohibited by this review contract and were not executed.
- Expected results: no regression .out files changed; modified unit assertions preserve their numeric/category expectations.
- Observability: enum logs and compaction trigger labels retain existing values and identifiers; no new operational path requires additional instrumentation.
- Transactions, persistence and failover: column-family descriptor order, write batches, persisted state mappings, publish/delete-bitmap gates and visibility checks remain unchanged; no new replay/failover obligation identified.
- Writes, atomicity and crashes: operation columns still receive the same int64 values; transaction, rowset publication, cleanup and crash boundaries are unchanged.
- FE/BE transport: no new transmitted variable or protocol sending point.
- Performance and memory: fixed-width operation/category vectors preserve their element widths; no new scans, allocations or ownership edges found. Page-size arithmetic is equivalent for valid pages; widened plain-page products avoid prior 32-bit overflow on malformed counts.
- Other issues: IK's bool replaces only reachable binary trie states, Pinyin's removed status field had no readers, and standard-tokenizer EOF/token values are preserved. No additional substantiated issue found.
User focus: no additional focus points were supplied. Initial changelog-boundary, unchanged-consumer/persistence, and unsigned-page-arithmetic risks were independently investigated and dismissed with concrete evidence. Validation limit: static review only; no build, unit/regression execution, or release-artifact ABI verification is claimed.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28667 ms |
Contributor
TPC-DS: Total hot run time: 151353 ms |
Contributor
ClickBench: Total hot run time: 24.28 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
approved these changes
Oct 22, 2026
gavinchou
approved these changes
Oct 22, 2026
luwei16
approved these changes
Oct 22, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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
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
Related PR: #69174
Problem Summary:
Unscoped enums and integer fields used for storage states, character categories, compaction triggers, and binlog operations allow unintended implicit conversions and mixing unrelated values. Use scoped enums throughout these paths, store enum values in typed fields and containers, and convert explicitly at serialization, indexing, and logging boundaries. Replace anonymous size enums with constexpr constants and the IK dictionary's binary node state with a bool.
Persisted numeric values and storage formats remain unchanged. Keep compatibility constants and compaction-type stream output so Cloud source and tests do not need changes.
Release note
None
Check List (For Author)
BUILD_TYPE=ASAN bash ./build.sh --be -j48passed. Header hygiene and diff whitespace checks passed. Existing unit tests were adapted but not run.Check List (For Reviewer who merge this PR)