Repository navigation
[refactor](be) Read stored values through StorageLayout - #69609
Merged
Merged
Conversation
… the storage types from core ### What problem does this PR solve? Issue Number: None Related PR: apache#68608, apache#68664, apache#69070 Problem Summary: Since apache#69070 the write path converts every value through StorageLayout<FieldType>, but the read direction still lives elsewhere, in two copies. The plain and bitshuffle page decoders push the stored bytes through IColumn::insert_many_fix_len_data, and ColumnVector and ColumnDecimal unpack the V1 DATE, DATETIME and DECIMAL layouts inside that method. The zone map turns the stored bounds of a page back into Fields through Field::create_field_from_olap_value, which reads the storage type of every PrimitiveType from PrimitiveTypeTraits::StorageFieldType. So core still knows how the storage engine lays out three of its types, and the layout table is not the one place that says how a type is stored. This change gives the fixed-width layouts the read direction and removes the other two copies: - `StorageLayout<FT>::to_primitive` is the inverse of `to_storage` for one value, and `storage_to_column(values, n, dst)` appends n stored values, which need not be aligned, to the column of the PrimitiveType, or to a Nullable over it whose new rows are marked not NULL. The bit-cast and floating-point layouts copy once; the V1 layouts repack value by value. The plain and bitshuffle decoders read through it. - `IColumn::insert_many_fix_len_data` and its overrides, with `ColumnVector::insert_date_column` / `insert_datetime_column`, are deleted; core no longer includes uint24.h or decimal12.h through primitive_type.h. - The zone map writer keeps its running bounds as the stored values of the column's FieldType (`CppTypeTraits<FT>::CppType`) and materialises them with `to_primitive`. `Field::create_field_from_olap_value`, `PrimitiveTypeTraits::StorageFieldType` and the unused `WindowFunnelState::NativeType` go away. Pages decode to the same values as before. Two details of the zone map change in form only: - Its DATE bound used to be rebuilt with the range-checked `from_olap_date`, which leaves the zero date when the stored bits do not form a valid date; the page decoders always used the unchecked `set_olap_date`, which `to_primitive` now uses for both. Every date the writer stores comes from `to_olap_date` of a checked value, so no stored bound changes; only a corrupted cell would now read back as its own fields instead of the zero date. - Its DATETIME bounds compare as int64 instead of uint64; the packed digits never set the sign bit. ### Release note None ### Check List (For Author) - Test: Unit Test - New: StorageLayoutTest round trips `to_primitive` for every layout kind and reads unaligned V1 cells, bit-cast cells and cells into a Nullable column through `storage_to_column`; PlainPageTest and BitShufflePageTest decode a V1 DATE page by batch and by row id into ColumnDate and a Nullable over it. - Adapted: the tests that built Fields from stored bytes use `field_of_cell` (be/test/storage/storage_layout_test_util.h); the tests of the deleted insert helpers are removed. - Run on this revision (ASAN build): 518 tests from 23 suites, all pass: StorageLayoutTest (22), PlainPageTest (8), BitShufflePageTest (14), ColumnZoneMapTest (25), PageFormatTest (2), SegmentFlusherFormatTest (7) and SegmentFlusherTransformFormatTest (4) against their unchanged goldens, ColumnReaderTest (67), EncodingInfoTest (8), ColumnVectorTest (42), ColumnDecimalTest (38), ColumnArrayTest (55), ColumnNullableTest (18), DateBloomFilterTest (2), OlapTypeTest (43), TestRowCursor (19), InvertedIndexReaderTest (53), KeyCoderDateTimeTest (1), KeyCoderTest (8), RowKeyEncoderTest (9), IndexBuilderTest (42), PartitionTransformersTest (27), StorageFieldTypeTest (4). - clang-format 16, build hygiene, clang-tidy on the changed lines. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#68608 Problem Summary: Review follow-up on the read direction of StorageLayout. values_column had one caller, repack_to_column, and its name did not say what it did: mark the rows a page decodes into a Nullable destination not NULL and hand back the nested column. repack_to_column now does that itself before it repacks into the nested column, next to the only code that needs it (the bit-cast and floating-point layouts reach ColumnNullable through insert_many_raw_data, which marks the rows itself). The comment above TypedZoneMapIndexWriter::ValType, which master never had, is dropped. ### Release note None ### Check List (For Author) - Test: Unit Test - Run on this revision (ASAN build): the 518 tests from the 23 suites run for the read direction, all pass, StorageLayoutTest (22) with StorageToColumnIntoNullableMarksTheRowsNotNull, PlainPageTest (8) and BitShufflePageTest (14) with the V1 DATE pages read by row id into a Nullable column, and SegmentFlusherFormatTest (7) / SegmentFlusherTransformFormatTest (4) against their unchanged goldens. - clang-format 16, build hygiene, clang-tidy on storage_layout.cpp. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#68608 Problem Summary: CppTypeTraits<FieldType>::CppType is the C++ type a segment stores for a FieldType, and PrimitiveTypeTraits<PrimitiveType>::CppType is the compute-layer type of a PrimitiveType. The same member name on both makes the code that converts between the two layers (StorageLayout, the zone map, the key coder) hard to read: every CppType has to be traced back to the traits it came from. The storage-layer traits become StorageTypeTraits<FieldType>, with the members StorageType and UnsignedStorageType (the unsigned integer of the same width, which the key coder and the BKD encodings use to reorder the bits). The static_assert of the primary template and the comments that name the traits follow. Aliases that only restate a member keep their own names: BaseFieldTypeTraits / NumericFieldTypeTraits / TypeTraits::CppType, the local CppType / UnsignedCppType of the key coders, and StorageLayout::StorageValue. No type changes. ### Release note None ### Check List (For Author) - Test: Unit Test - Run on this revision (ASAN build; the full UT build compiles every user of the traits): 673 tests from 31 suites, all pass: the 23 suites run for the read direction (518 tests) and the suites of the test files the rename touches, BloomFilterIndexReaderWriterTest (31), PageBuilderColumnTest (16), KeyCoderTraitsTest (8), BkdDifferentialTest (21), BkdNativeOrderTest (32), BkdNativeOrderControl (1), BkdPropertyTest (24) and SniiBkdAdapterTypesTest (22). - clang-format 16, build hygiene. clang-tidy on the 11 changed .cpp files reports nothing on a changed line except the existing cognitive complexity warning of test_ordering in key_coder_test.cpp, whose signature line the rename touches; its body is unchanged. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…changes ### What problem does this PR solve? Issue Number: None Related PR: apache#68608 Problem Summary: Review follow-up. clang-tidy reports errors on lines this branch changed, in test files the earlier tidy pass did not cover: - The decimal12_t brace initializers in storage_layout_test and row_cursor_test now name their fields (modernize-use-designated-initializers). - The V1 DATE page tests of plain_page_test and bitshuffle_page_test build their page in a fixture helper, and decoding by batch and reading by row id into a Nullable column are now separate tests. Each test checks one path, and each body stays under the cognitive-complexity threshold. - key_coder_test's test_ordering, whose body is unchanged from master and whose signature line the rename touched, gets the NOLINTNEXTLINE that other long test bodies in be/test use. repack_to_column now tests for a Nullable destination with is_column_nullable, as the column reader does. IColumn::is_nullable() is also true for a ColumnConst over a Nullable, which the cast that follows would reject. A page decoder is only ever handed a column the reader created, so nothing changes at run time. ### Release note None ### Check List (For Author) - Test: Unit Test - Run on this revision (ASAN build): 675 tests from 31 suites, the set run for the rename, all pass; PlainPageTest (9) and BitShufflePageTest (15) now hold the split V1 DATE tests. - clang-format 16, build hygiene, clang-tidy on the six files this commit touches: no finding on a line the branch changes. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
csun5285
requested review from
airborne12,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
October 26, 2026 04:54
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 42 changed paths at head 5287505, including the removed column test source, related reader/writer call chains, and parallel encoding paths. No substantiated new finding or existing blocking inline finding remains. No additional user focus was supplied.
The refactor routes fixed-width page decoding and zone-map bound conversion through StorageLayout while removing storage-specific column/Field APIs and duplicate type aliases. The concrete flow reviewed was: writer storage cells -> plain/bitshuffle page decoder -> canonical mutable primitive or nullable column; raw zone-map bounds -> StorageLayout::to_primitive -> Field -> serialized bounds.
Critical checkpoint conclusions:
- Goal and evidence: conversion ownership is centralized, with tests for inverse values, unaligned V1 bytes, append offsets, nullable flags, stored NaN payloads, and DATE batch/row-ID decoding. Tests were inspected, not executed.
- Scope and clarity: changes stay within conversion/trait migration and replacement tests; no unrelated production change identified.
- Concurrency and locks: no new threads, shared mutable state, lock acquisitions or lock-order changes. Destination columns retain existing mutable ownership.
- Lifecycle and initialization: conversions synchronously copy into owned column buffers. Test page owners outlive decoder views. No new global initializer or reference cycle.
- Configuration: no added or changed configuration.
- Compatibility: storage trait underlying types, cell widths, encodings and persisted formats remain unchanged. All removed in-tree API consumers were migrated; explicit layout template declarations and definitions match.
- Thrift/protobuf: no changes; cross-release field-ID checks are not applicable.
- Parallel paths: plain/bitshuffle sequential and row-ID reads and dictionary INT decoding are covered. FOR, RLE and binary decoding retain their separate paths; registered fixed-width types have layouts.
- Special conditions: the nullable branch documents the page invariant. FileColumnIterator separates NULL runs, and canonical file-type columns establish the new concrete V1 casts; no reachable ColumnConst or nullable-shape regression found.
- Test coverage: focused unit coverage includes negative decimals, unaligned input, nullable appends and DATE decoder integration. No end-to-end, build or test execution was performed under the static-only contract.
- Expected results: no regression output files changed; inspected C++ expectations match their stored values and row IDs.
- Observability: existing Status/exception paths remain sufficient for this refactor; no new distributed operation or independent operational state requires metrics/logs.
- Transactions and persistence: no EditLog, transaction visibility, rowset-version, delete-bitmap or failover protocol changes.
- Data writes and atomicity: write conversions retain existing representations; read appends and allocation/type failures remain local to the established mutable-column flow. No new durable state or crash-recovery dependency.
- FE/BE variables: no transmitted variables or send paths added.
- Performance and memory: raw layouts retain bulk copying; V1 repacking reserves once and appends per value using existing tracked POD arrays. No new owning buffer, per-row COW detachment or redundant scan identified.
- Additional risks: checked legacy DATE/DATETIME/DECIMAL conversion, signed DATETIME bounds, numeric/timestamp wrappers, zone-map serialization, removed-symbol references, transitive includes and CMake discovery. No supported-input regression was established.
The three initial risk areas have evidence-based dispositions: page conversion/null-map and destination invariants are preserved; trait/API/template migration is complete by static inspection; supported zone-map bounds retain their values. Arbitrary corrupt nonzero DATE cells can produce different intermediate Fields because the shared inverse uses unchecked date field setting; no supported introduced trigger was found, and equivalence for arbitrary corrupt cells is not claimed. DATE serde's existing invalid-parse behavior substitutes FIRST_DAY.
Coverage limits: static review only, with no compilation, runtime tests or release ELF audit. No unresolved concrete candidate or uncovered changed-file scope remains. Review converged after two rounds, including an independent check of the frozen payload.
Review converged after 2 round(s).
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28399 ms |
Contributor
TPC-DS: Total hot run time: 150935 ms |
Contributor
ClickBench: Total hot run time: 24.53 s |
…writer does ### What problem does this PR solve? Issue Number: None Related PR: apache#68608 Problem Summary: Review follow-up. The read direction of StorageLayout handled a Nullable destination: repack_to_column, which writes the typed column of DATE, DATETIME and DECIMALV2 directly, unwrapped a Nullable itself, while the other layouts left it to ColumnNullable's insert_many_raw_data. The write direction never sees a Nullable: ScalarColumnWriter::append peels it off with peel_nullable, writes the NULL runs itself and hands column_to_storage the nested column. FileColumnIterator now does the same when it reads. next_batch and read_by_rowids peel a Nullable destination once: the iterator appends the NULL runs to its null map and nested column, and the page decoders append the values to the nested column. A dictionary page decoder may replace the column it appends to (a ColumnDictI32 becomes a ColumnString when a page falls back to plain encoding), so the nested column goes back into the Nullable when the read ends, on an early return too. Neither the page decoders nor StorageLayout see a Nullable any more, and repack_to_column loses its Nullable branch. ### Release note None ### Check List (For Author) - Test: Unit Test - PageFormatTest reads every column back by batch and now also by row id, two rows of every three so that the reads start and stop inside NULL runs; the DICT cases are read into a Nullable over ColumnDictI32 as well, which the plain pages at the end of each column make the decoder replace. - The V1 DATE decoder tests read by row id into a ColumnDate; the StorageLayout test of a Nullable destination is removed. - Run on this revision (ASAN build): the 31 suites of the earlier runs, 674 tests, all pass; and 76 more suites that read through a column reader (SegmentIterator, compaction, rowset, variant, inverted index, page decoders): 1015 tests pass, 3 skip themselves, none fail. TestDeltaWriterClusterKey and SegmentCacheTest stop in their first test once its read-back checks have passed: drop_tablet reaches Rowset::clear_cache, which uses a FileCacheFactory that no earlier test of a filtered run installs (enable_file_cache defaults to true). TestDeltaWriter, which drops tablets the same way, was not run. - clang-format 16, build hygiene, clang-tidy on the changed files: no finding on a line the branch changes. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… column writer does" ### What problem does this PR solve? Issue Number: None Related PR: apache#68608 Problem Summary: Review follow-up. This reverts commit 184f186, which peeled a Nullable destination off once per read through the PeeledNullable helper class in column_reader.cpp. The next commit redoes the change at the page decoder call sites instead, without the class. ### Release note None ### Check List (For Author) - Test: No need to test (the tree returns to 5287505, which was tested; the next commit carries the replacement and its tests) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lable ### What problem does this PR solve? Issue Number: None Related PR: apache#68608 Problem Summary: Review follow-up, redoing the reverted 184f186 without its PeeledNullable class. The read direction of StorageLayout handled a Nullable destination: repack_to_column, which writes the typed column of DATE, DATETIME and DECIMALV2 directly, unwrapped a Nullable itself, while the other layouts left it to ColumnNullable's insert_many_raw_data. The write direction never sees a Nullable: ScalarColumnWriter::append peels it off with peel_nullable and hands column_to_storage the nested column. FileColumnIterator::next_batch and read_by_rowids now pass each page decoder call through decode_values (column_reader.cpp). For a Nullable dst the decoder appends to the nested column, and the null map grows by the rows the nested column gained, as ColumnNullable's insert overrides did. A dictionary page decoder may replace the column it appends to (a ColumnDictI32 becomes a ColumnString when a page falls back to plain encoding); the replacement goes back into the Nullable right after the call. Between the calls dst is a whole Nullable again, so the NULL runs are appended as before and the loops change only at the four decoder calls. Neither the page decoders nor StorageLayout see a Nullable any more: repack_to_column loses its Nullable branch, and the dictionary page decoder's comment no longer says that a ColumnNullable converts its nested column. Lending the nested column costs, per decoder call into a Nullable, an is_column_nullable call and one reference count increment and decrement; 184f186 paid that once per read, at the price of the class and of appending the NULL runs to the lent column. ### Release note None ### Check List (For Author) - Test: Unit Test - PageFormatTest reads every column back by batch and now also by row id, two rows of every three so that the reads start and stop inside NULL runs; the DICT cases are read into a Nullable over ColumnDictI32 as well, which the plain pages at the end of each column make the decoder replace. Under gdb, ColumnNullable::change_nested_column is reached from all four decoder calls, 16 times, once for each ColumnDictI32 the decoder converts, and the test passes. - The V1 DATE decoder tests read by row id into a ColumnDate; the StorageLayout test of a Nullable destination is removed. - Run on this revision (ASAN build): 674 tests from the 31 suites of the earlier runs, all pass; and the reader suites of the earlier run (SegmentIterator, compaction, rowset, variant, inverted index, page decoders): the same 1015 tests in 75 suites pass, 3 skip themselves. VectorSearchTest.InnerProductRangeSearchBasic failed once (249 results against 251, tolerance 1; it compares an approximate HNSW range search over vectors seeded from std::random_device with an exact one and reads no column) and passed in 5 reruns. SegmentCacheTest and TestDeltaWriterClusterKey stop in their first test once its read-back checks have passed: Rowset::clear_cache uses a FileCacheFactory that no earlier test of a filtered run installs (enable_file_cache defaults to true). TestDeltaWriter was not run. - clang-format 16, build hygiene, clang-tidy on the changed files: no finding on a line this change touches; the cognitive complexity of next_batch and read_by_rowids stays at the base's 97 and 148. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Reviewed all 45 changed paths at a6682b0. No new actionable findings and no existing inline blockers to retain. No additional user focus was supplied.
The reviewed flow is: execution column -> ScalarColumnWriter nullable peeling -> stored cells/pages -> FileColumnIterator NULL/value runs -> page decoder -> StorageLayout execution values -> nullable map/replacement restoration -> scanner predicates/output. Index writers consume the same cells; zone maps convert their bounds back to Fields through the same layout.
Critical checkpoint conclusions:
- Goal: storage-specific read conversion is centralized in StorageLayout and removed from compute column APIs. Direct conversion and integrated page-read tests support the intended refactor; they were inspected, not run.
- Scope and clarity: the production changes and corresponding test migrations stay focused on this representation boundary.
- Concurrency and locks: no new threads, shared mutable conversion state, locks, lock order, or lock-duration change. Existing iterator and destination ownership remains in use.
- Lifecycle and initialization: no new static initialization dependency. Nullable nested borrowing ends within decode_values; dictionary fallback retains existing rows and installs the replacement column before returning.
- Configuration: no items added or changed.
- Compatibility: all 34 renamed storage traits retain their concrete storage/unsigned types. Page writers and encodings retain their format; DATE, DATETIME and DECIMAL reads use the former conversion routines. No exchange serialization or mixed-version protocol change was found. Internal removed APIs have no remaining live callers in repository searches.
- Thrift/protobuf: no definitions changed.
- Parallel paths: traced batch and rowid reads, plain/bitshuffle, bool RLE, binary plain V1/V2/V3, dictionary fallback, prefix, scalar complex-type children, offset peeking and IndexedColumnIterator. FOR value reads remain pre-existing NotSupported behavior.
- Conditions and errors: nullable detection is structural; replacement detection handles a real dictionary-to-string transition. NULL runs stay outside value decoding, value rows receive matching false null flags, and decoder Status errors propagate.
- Test coverage: new checks cover unaligned V1 cells, append to a nonempty column, negative decimal values, NaN payloads, DATE batch/rowid decoding, and selected rowids/NULL maps/dictionary fallback across pages. Offset-only and null-map-only paths were code-traced. Compilation, runtime error injection and end-to-end SQL behavior remain unexecuted validation limits.
- Expected results: no .out or golden bytes changed. Inspected expected dates, decimal signs, NaN bits and selected row comparisons agree with the representations. Existing page-format golden comparisons remain in place.
- Observability: this refactor adds no operational path requiring new logs or metrics; existing error propagation remains available.
- Transactions and persistence: no transaction, rowset-version, delete-bitmap, EditLog, publish or failover protocol changes. Persisted cell representations are retained.
- Writes and crash behavior: writers consume the same cell types and use existing encoders; no new commit ordering, persistent state or crash cleanup mechanism is introduced.
- FE/BE variables: none added or passed differently.
- Performance and memory: compatible layouts retain bulk copies; legacy layouts reserve once and convert linearly into tracked column storage. Nullable borrowing does not invoke per-run COW cloning. No new owning conversion buffer is added.
- Other checks: layout extern declarations match explicit instantiations; migrated test discovery and direct scalar-type includes were checked. No new source .cpp test inclusion or libc/toolchain dependency was found. Binary ABI compatibility was not measured.
The review converged after 2 rounds: both complete-review agents and the independent risk agent reported NO_NEW_VALUABLE_FINDINGS in the final round; every candidate and R1/R2/R3 risk was dismissed with evidence, and no static coverage question remains. This is static inspection only: no builds, tests, benchmarks, formatting/static-analysis gates or ELF audits were run, and no source files were modified.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28910 ms |
Contributor
TPC-DS: Total hot run time: 150851 ms |
Contributor
ClickBench: Total hot run time: 24.36 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
approved these changes
Oct 27, 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: 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)