Uh oh!
There was an error while loading. Please reload this page.
[refactor](storage) drop StorageField wrapper and clean up related dead code - #63233
Merged
airborne12 merged 3 commits intoMay 21, 2026
Merged
Conversation
csun5285
requested review from
airborne12, dataroaring and gavinchou
as code ownersMay 14, 2026 02:07
csun5285
commented
May 14, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
May 14, 2026
Contributor
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
hello-stephen
commented
May 14, 2026
Contributor
TPC-H: Total hot run time: 29579 ms |
hello-stephen
commented
May 14, 2026
Contributor
TPC-DS: Total hot run time: 172234 ms |
hello-stephen
commented
May 14, 2026
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
May 14, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
commented
May 14, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
May 14, 2026
Contributor
TPC-H: Total hot run time: 29501 ms |
hello-stephen
commented
May 14, 2026
Contributor
TPC-DS: Total hot run time: 171293 ms |
hello-stephen
commented
May 14, 2026
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
May 14, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285force-pushed
the
refactor/drop-storagefield-wrapper
branch
from
May 15, 2026 03:14
dba2951 to
2a6bd48Comparecsun5285
commented
May 15, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
May 15, 2026
Contributor
TPC-H: Total hot run time: 29501 ms |
hello-stephen
commented
May 15, 2026
Contributor
TPC-DS: Total hot run time: 170724 ms |
hello-stephen
commented
May 15, 2026
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
May 15, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
May 15, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
PR approved by at least one committer and no changes requested. |
Contributor
PR approved by anyone and no changes requested. |
airborne12
commented
May 17, 2026
Member
/review |
Contributor
There was a problem hiding this comment.
Review completed. I did not find any blocking correctness issues in this refactor.
Critical checkpoint conclusions:
- Goal/test proof: The PR removes StorageField and routes storage/index/schema code through TabletColumn directly. The changed call sites appear to preserve the previous forwarded metadata semantics. Existing BE UT coverage is claimed by the PR; I did not rerun the full suite in this review runner.
- Scope/focus: The change is broad but mechanically focused on replacing the wrapper and deleting now-unused value wrappers/tests.
- Concurrency/lifecycle: No new concurrent state or lock ordering changes were introduced. Ownership changes from raw StorageField pointers to TabletColumnPtr/shared copies appear lifetime-safe for the reviewed writer/schema/iterator paths.
- Config/compatibility/protocol: No new config, persisted format, thrift, or FE-BE protocol changes found. Column metadata written to segment/index footers remains derived from TabletColumn/ColumnMetaPB as before.
- Parallel paths: Segment writing, index building, zone map, inverted index, ANN, row cursor, variant writer, and tests were updated consistently; no remaining live StorageField/CollectionValue references were found outside stale comments.
- Error handling: Existing Status propagation is preserved in the changed paths; no newly ignored Status found.
- Data correctness: Key encoding, index writer creation, and cell-size pointer stepping use the same field_type_size/KeyCoder semantics StorageField previously forwarded. Complex container writers avoid direct container cell-size stepping.
- Performance/memory: Removal of wrapper allocations is neutral-to-positive; no obvious new hot-path redundant work beyond a KeyCoder lookup in RowCursor noted by the PR as non-hot.
- Observability: No new observability need for this internal refactor.
- User focus: No additional user-provided review focus was supplied.
Residual risk: I did not run the full BE build/UT suite here, so compile/test confidence depends on CI and the author-reported BE UT run.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
yiguolei
previously approved these changes
May 20, 2026
StorageField was a thin wrapper over TabletColumn that cached a KeyCoder* pointer and pre-resolved an owned tree of sub-fields. Aside from those two extras, every accessor (type/length/name/is_nullable/unique_id/...) was a direct forward to the underlying TabletColumn it held a copy of, and all 11 subclasses (CharField/VarcharField/.../HllAggField) were empty stubs with zero callers distinguishing them (no dynamic_cast/typeid/static_cast). Replace StorageField with TabletColumn throughout the storage layer: - Schema now stores vector<TabletColumnPtr> instead of vector<StorageField*>, so copy/dtor are handled by shared_ptr ref counting; the deep-copy clone() path is gone. - ColumnWriter family takes TabletColumnPtr (owned by writer) instead of unique_ptr<StorageField>; get_field()/_field renamed to get_column()/_column. - IndexColumnWriter::create and ZoneMapIndexWriter::create take const TabletColumn* directly. - DataTypeFactory drops the StorageField overload (kept the existing TabletColumn one). - Five row_cursor.cpp encode sites switch to free helpers (get_key_coder(type)->...), the only consumers of StorageField's _key_coder cache. Per-call switch overhead is negligible since this is not a hot path; production hot paths (vertical/segment_writer, indexed_column_*) already cache KeyCoder locally without going through StorageField. - _has_char_type, _init_column_mapping, index_builder, variant_*, and segment_iterator switch to TabletColumn::get_sub_column/get_subtype_count in place of StorageField::get_sub_field/get_sub_field_count. - Rename row_cursor's _encode_field to _encode_column_value and column_schema(cid) to column(cid) to match the new semantics; rename local null_field/bigint_field to null_column_ptr/length_column_ptr. Net effect: -434 lines (field.h removed plus dead 11-subclass hierarchy), no behavioral change. BE UT (302 tests across tablet_schema, storage_types, KeyCoder, InvertedIndex, BkdIndex, ColumnWriter, ZoneMap, RowCursor) all pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rs + minor cleanup Follow-up to "drop StorageField wrapper". Three small refactors: 1. ColumnWriter::cell_size() helper Replace 7 occurrences of `field_type_size(get_column()->type())` in ColumnWriter::append_nullable / ScalarColumnWriter::append_* with a single inline cell_size() method on the base class. Pure DRY, no behavior change. 2. VariantWriter ctors: drop redundant raw `column` param VariantDocCompactWriter / VariantSubcolumnWriter / VariantColumnWriter previously took both `(const TabletColumn* column, TabletColumnPtr owned_column)` where the call site always passed the same column in both positions: std::make_unique<VariantDocCompactWriter>( opts, column, std::make_shared<TabletColumn>(*column)); The raw `column` was stored as `_tablet_column` for direct getter access, but the same pointer is available via the base class's `get_column()` (returning `_column.get()`). Collapse to a single `TabletColumnPtr column` param and drop the redundant `_tablet_column` member from both ColumnWriter subclasses; rewrite their 6 method-body usages to call `get_column()`. VariantColumnWriterImpl is the pimpl impl of VariantColumnWriter and is NOT a ColumnWriter subclass, so it keeps its own `_tablet_column` member; the outer VariantColumnWriter ctor now passes `get_column()` to the impl ctor. Semantic change: the raw pointer formerly pointed to the caller's original TabletColumn; now it points to the base class's owned copy (made via `std::make_shared<TabletColumn>(*column)` at the call site). Lifetime-safe and content-equivalent for read-only access. 3. olap_common.h: update stale comments - FieldType enum: drop the dangling "Field" reference (StorageField was removed by the parent commit, doris::Field is unrelated here). - FieldAggregationMethod enum: restore the previous verbose comment style with the class name updated to TabletColumn.
…k / disabled test Follow-up to the previous two commits. Two latent breakages where code that doesn't ride the default build path still referenced classes deleted by this PR series: 1) be/benchmark/benchmark_zone_map_index.hpp `benchmark_test` (built only with `--benchmark`, default OFF) was wiring up its zone-map writer through the now-deleted `StorageFieldFactory::create`. Drop the wrapper and pass the `TabletColumn*` straight into `ZoneMapIndexWriter::create`, which is what the writer wants anyway after the previous commit's API change. Verified by RELEASE build of the `benchmark_test` target: our hpp compiles cleanly (the binary as a whole still fails on `binary_cast_benchmark.hpp`, but that is fallout from PR apache#53135 union-to-`std::bit_cast` migration and is out of scope here). 2) be/test/storage/segment/column_reader_writer_test.cpp This file is in the `# todo: need fix those ut` REMOVE_ITEM list of be/test/CMakeLists.txt and is not built. It was already broken on the deleted row-based `ColumnBlock` / `ColumnVectorBatch` API from a prior BE refactor; this PR series additionally killed `StorageField` and `CollectionValue` it referenced. Strip out the four pieces tied to the classes this PR deleted, leaving the rest of the file as-is for whoever eventually revives the legacy `ColumnBlock` paths: - the `test_array_nullable_data` helper (used CollectionValue + StorageFieldFactory) - TEST_F(test_array_type) (only caller of that helper) - TEST_F(test_single_empty_array) (constructed CollectionValue) - TEST_F(test_mixed_empty_arrays) (same) The file stays in REMOVE_ITEM so the build state is unchanged.
csun5285force-pushed
the
refactor/drop-storagefield-wrapper
branch
from
May 20, 2026 05:49
2a6bd48 to
c379e58Comparecsun5285
commented
May 20, 2026
ContributorAuthor
run buildall |
yiguolei
commented
May 20, 2026
Contributor
/review |
hello-stephen
commented
May 20, 2026
Contributor
TPC-H: Total hot run time: 31562 ms |
hello-stephen
commented
May 20, 2026
Contributor
TPC-DS: Total hot run time: 171520 ms |
hello-stephen
commented
May 20, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
approved these changes
May 20, 2026
gavinchou
approved these changes
May 20, 2026
Contributor
PR approved by at least one committer and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
zhaorongsheng pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Jun 4, 2026
…ad code (apache#63233) Drop the `StorageField` wrapper and related dead code. `StorageField` was a thin layer over `TabletColumn` — every accessor just forwarded, all 11 subclasses were empty stubs with no caller distinguishing them via dynamic_cast/typeid. After removing it, several dead pieces fell out. ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx
csun5285 added a commit
to csun5285/doris
that referenced
this pull request
Jun 16, 2026
…ad code (apache#63233) Drop the `StorageField` wrapper and related dead code. `StorageField` was a thin layer over `TabletColumn` — every accessor just forwarded, all 11 subclasses were empty stubs with no caller distinguishing them via dynamic_cast/typeid. After removing it, several dead pieces fell out. Issue Number: close #xxx Related PR: #xxx (cherry picked from commit 8a8999c)
yiguolei pushed a commit
that referenced
this pull request
Jun 17, 2026
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Drop the
StorageFieldwrapper and related dead code.StorageFieldwas a thin layer overTabletColumn— every accessor just forwarded, all 11 subclasses were empty stubs with no caller distinguishing them viadynamic_cast/typeid. After removing it, several dead pieces fell out.
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)