Uh oh!
There was an error while loading. Please reload this page.
[refactor](storage) replace TypeInfo polymorphic hierarchy with FieldType - #63153
Conversation
hello-stephen
commented
May 12, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
csun5285
commented
May 12, 2026
run buildall |
hello-stephen
commented
May 12, 2026
TPC-H: Total hot run time: 29787 ms |
hello-stephen
commented
May 12, 2026
TPC-DS: Total hot run time: 170297 ms |
zclllyybb
commented
May 12, 2026
/review |
There was a problem hiding this comment.
This refactor is not ready to merge because it removes the storage TypeInfo helper API while leaving multiple BE test files that still call the removed symbols, so those test targets will fail to compile.
Critical checkpoint conclusions:
- Goal/test proof: The intended storage refactor is only partially applied; existing tests are not fully migrated, and the PR marks no tests as run.
- Scope/focus: The production changes are focused, but the migration is incomplete across test code.
- Concurrency/lifecycle/config/compatibility: No new concurrency, special lifecycle, config, persistence, or wire/storage format compatibility concerns were identified in the reviewed changes.
- Parallel paths: Parallel BE storage test paths still use the removed API and must be updated.
- Test coverage/results: Existing tests were modified in a few files, but many affected tests remain unmigrated; no generated results are involved.
- Observability/performance: No additional observability or performance issue found for this refactor.
- User focus: No additional user-provided review focus was specified.
Uh oh!
There was an error while loading. Please reload this page.
…Type The TypeInfo class hierarchy (TypeInfo / ScalarTypeInfo / ArrayTypeInfo / MapTypeInfo / StructTypeInfo) only exposed two operations -- type() and size() -- both fully derivable from FieldType. Storing a TypeInfoPtr alongside the TabletColumn / Field that already carries the same information was redundant. Replace TypeInfo* fields and parameters with FieldType throughout the storage layer, and add a static field_type_size(FieldType) helper that maps each type to sizeof(CppTypeTraits<FT>::CppType) so size lookups stay in sync with the existing traits machinery. Index writer's ARRAY peel-off now reads the inner element type from TabletColumn::get_sub_column(0).type() instead of dynamic_cast'ing to ArrayTypeInfo, which removes the only consumer of nested TypeInfo accessors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f81e5db to
b2ebab5Comparecsun5285
commented
May 12, 2026
run buildall |
PR approved by at least one committer and no changes requested. |
hello-stephen
commented
May 12, 2026
TPC-H: Total hot run time: 29627 ms |
hello-stephen
commented
May 12, 2026
TPC-DS: Total hot run time: 171000 ms |
hello-stephen
commented
May 12, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
May 13, 2026
/review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Automated review completed for b2ebab5.
I did not find additional distinct issues beyond the inline threads that are already open, so I am not duplicating those comments.
Critical checkpoint conclusions:
- Goal/test evidence: the PR consistently replaces BE storage
TypeInfoplumbing withFieldType/field_type_sizein the modified storage/index paths. Existing CI shows Doris compile and BE UT TeamCity checks passing; the GitHub macOS BE UT failure is an environment JDK-25-vs-JDK-17 setup failure. - Scope: the change is broad but mechanically focused on removing
TypeInfofrom storage/index readers/writers and associated tests. - Concurrency/lifecycle: no new shared mutable state, locks, thread entries, or cross-TU static dependencies were introduced in the reviewed diff.
- Config/compatibility: no new config items or persisted format changes were introduced; existing serialized field type and encoding metadata remain unchanged.
- Parallel paths: the reviewed primary key, bloom filter, zone map, inverted index, column reader/writer, and default-value paths were updated consistently in the touched code.
- Error handling: status propagation is preserved in the reviewed paths; unsupported type handling remains present where readers/writers initialize. Existing open comments already cover questions around
NONE/UNKNOWNhandling and variable-length type sizing. - Data correctness/version/delete bitmap: the change does not modify visible-version selection, rowset lifecycle, transaction persistence, or MoW delete-bitmap semantics.
- Memory/performance: no new large allocations or memory-tracking-sensitive containers were introduced; the change removes TypeInfo allocation/indirection.
- Test coverage: touched unit tests were migrated in part, and existing review context already discusses remaining TypeInfo test references, so I did not repeat that thread.
User focus: no additional user-provided focus points were present.
PR approved by anyone and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
…Type (apache#63153) Replace TypeInfo* fields and parameters with FieldType throughout the storage layer, and add a static field_type_size(FieldType) helper that maps each type to sizeof(CppTypeTraits<FT>::CppType) so size lookups stay in sync with the existing traits machinery.
…Type (apache#63153) Replace TypeInfo* fields and parameters with FieldType throughout the storage layer, and add a static field_type_size(FieldType) helper that maps each type to sizeof(CppTypeTraits<FT>::CppType) so size lookups stay in sync with the existing traits machinery. (cherry picked from commit 7e5c941)
Replace TypeInfo* fields and parameters with FieldType throughout the storage layer, and add a static field_type_size(FieldType) helper that maps each type to sizeof(CppTypeTraits::CppType) so size lookups stay in sync with the existing traits machinery.
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)