Repository navigation
[test](be) Pin the on-disk pages of every page encoding and drop dead write-path code - #68609
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
ff00d41 to
0eed29e
Compare
|
/review |
There was a problem hiding this comment.
Complete static review of PR #68609 at head 0eed29ee2d87a9105f3d8224d7b172571040f97b. One new P2 issue is inline; no existing P0/P1 inline comments apply. No additional user focus was supplied.
Critical checkpoints:
- Goal and proof: The new BE test writes, reads back, and pins page bytes across registered storage types and encodings. The golden comparator can miss a binary-length change, so its exact-byte proof is incomplete (inline finding). Builds and tests were prohibited by the review prompt; conclusions are from static inspection.
- Scope and clarity: Four production files remove a CHAR guard, two complex-writer null-fill implementations, an unused template, and an unused member; the new test and eight fixtures support the page-format goal. No unrelated source change was found.
- Concurrency: The changed production paths add no thread, lock, or shared-state behavior. The unit test changes an existing page-cache flag in SetUp and restores it in TearDown; the prescribed harness runs these Google Test cases sequentially.
- Lifecycle and static initialization: Writer finish/write/close and reader setup are paired in the test; no new cross-translation-unit initializer or production lifecycle transition was found.
- Configuration: No production configuration is added. The test temporarily changes the existing page-cache flag and uses an opt-in environment variable for golden output.
- Compatibility and storage format: No encoding implementation or persisted format changes. The removed header template and private member have no repository callers. The CHAR access-path removal was traced through FE CHAR exclusion and BE CHAR predecoders; no reachable incompatible path was substantiated. Runtime compatibility was not tested.
- Parallel paths: Normal nullable Struct/Map append uses its existing
append_nullableoverrides; sparse Variant gap filling supports scalar/array paths and cannot reach the removed Struct/Map overrides on supported types. Plain, dictionary, and prefix page paths were checked against the test and CHAR path. - Conditional checks and error handling: The new NotSupported overrides fail if called; no supported caller was found. The golden comparator can silently return OK for extra trailing fixture bytes, violating the test's exact-byte contract (inline finding).
- Test coverage: The static type/encoding mapping yields 86 fixture cases and 502 recorded page entries; seven binary fixture sizes match layout totals. The comparator needs negative coverage for shorter and longer golden files.
- Test results: No build, unit test, regression test, or runtime validation was performed because the review prompt prohibits them. Fixture layout and diff consistency were checked statically.
- Observability: No new production operation needs logs or metrics. The test reports changed page names for in-page differences, but the inline issue covers unreported length differences.
- Transactions and persistence: No transaction, EditLog, publish, or replay path changes.
- Data writes and atomicity: The changed complex-writer methods are not on supported write routes; existing append/finish behavior remains. No new crash or atomicity path was established.
- FE/BE variables: No new cross-module variable or Thrift field is introduced.
- Performance: No new production hot-path work was added. The test-only fixture scan is bounded; no material production regression was identified.
- Other issues: All changed files, relevant upstream/downstream paths, existing review context, and candidate findings were rechecked. No unresolved or duplicate candidate remains. Review status: COMPLETE.
0eed29e to
1dcbc03
Compare
|
/review |
There was a problem hiding this comment.
Static review of PR #68609 at head 1dcbc03. Three new findings are inline: two P2 test-coverage gaps and one P3 comparison error. The prior P2 thread about binary golden lengths is fixed at lines 975-979 on this head; no existing P0/P1 inline finding remains. No additional user focus was supplied.
Review status: INCOMPLETE convergence. The source and focused agents found no new issue in round 3, but the test agent first found the CHAR read-back issue in that final permitted round. The three-round cap prevented another confirmation round. Every candidate found in these rounds was independently verified, deduplicated, accepted, or dismissed, and every accepted candidate is included below.
Critical checkpoints:
- Goal and proof: The suite exercises registered scalar page encodings and pins their data-page bytes, but it does not pin the production ARRAY/MAP offset-page footer, and its CHAR read-back comparison can accept padded logical values (inline findings). The checked-in layout has 502 page entries across 86 cases; seven binary fixture sizes match their listed page lengths.
- Scope and clarity: Four production files remove unused or unsupported paths; the new test and eight fixture files serve the page-format goal. No unrelated source change was found.
- Concurrency and locks: The changed production paths add no thread, shared state, or lock operation. The test changes the existing page-cache flag only within its fixture setup and teardown; no concurrent test execution was established.
- Lifecycle and static initialization: Writer append/finish/write/close and reader initialization were traced. No new cross-translation-unit static dependency or production lifetime transition was found.
- Configuration: No production configuration was added. The golden-output environment variable is test-only; no dynamic configuration contract changes.
- Compatibility and storage format: No production encoder or persisted format was changed. The tests target compatibility, but M2 shows a live footer format outside their golden set. No rolling-upgrade behavior was validated at runtime.
- Parallel paths: Struct/Map nullable writes use their pointer-form append_nullable overrides; sparse variant paths do not establish a supported call to the new NotSupported append_nulls methods. FE excludes resolved CHAR (including nested CHAR) from length-based OFFSET requests. Production offset pages and scalar pages were compared separately.
- Conditions and error handling: Removed CHAR rejection is guarded upstream by FE; unsupported Struct/Map direct null-fill returns an error. The layout comparator can invoke undefined behavior when the generated layout extends a shorter golden layout (inline finding). Status paths checked in the changed code were not silently discarded.
- Test coverage and results: The scalar page matrix covers normal, null, dictionary overflow, and multiple-page inputs, but M2 and M3 are concrete gaps. The prior binary-size issue is repaired. No build, unit test, regression test, or runtime check was run because the review prompt prohibits them; this review is static.
- Observability: No new production operation or distributed state requires a metric or log. The layout mismatch diagnostic needs the bounded comparison described inline.
- Transactions, persistence, and data writes: No EditLog, transaction, publish, replay, or committed-data visibility code changed. The writer deletions were checked against normal and nested write routes; no new production crash or atomicity failure was substantiated.
- FE/BE variables and performance: No new transmitted variable or Thrift field exists. No new production hot-path work or material performance regression was identified.
- Other issues: All 13 changed paths, relevant writer/reader and FE call chains, existing review context, and the final candidate list were swept. No candidate remains without a disposition. The review execution is complete under the three-round limit, with convergence explicitly incomplete as stated above.
1dcbc03 to
fc55d1b
Compare
|
/review |
There was a problem hiding this comment.
Review complete at fc55d1b after two full-review and focused-risk rounds. This was a static review only: the review instructions prohibit builds, tests, and source edits. I found no new distinct inline issue, and no existing P0/P1 issue remains to list as blocking.
Existing thread disposition: the [P2] CHAR read-back comparison (comment 4129841174) still masks the PREFIX logical-value mismatch, and the [P3] three-iterator layout comparison (4129841179) is still unbounded. The earlier [P2] golden byte-length issue (4129585058) is fixed by the explicit size check at lines 1016-1019. The [P2] offset-footer thread (4129841167) is partly addressed: the UNSIGNED_BIGINT case now uses OffsetColumnWriter and pins its page footer, while its requested composite ARRAY/MAP iterator coverage remains on record. I did not repost these concerns.
Critical checkpoints:
- Goal and proof: the added test generates 86 registered storage-type/encoding cases, compares seven checked-in page-byte files and a 467-page layout, rewrites for determinism, and reads values back. The current fixtures have consistent lengths, footer bounds, row counts, ordinals, offset footer ordinals, and CRC32C checksums. These static checks support fixture integrity, not a claim that the BE test ran; the two known read-back/layout limitations remain.
- Focus and size: no additional user focus was supplied. The production edits are confined to an unreferenced zone-map helper, an unused field, and STRUCT/MAP direct-null methods; the test and fixtures implement the stated page-encoding coverage.
- Concurrency: no new runtime thread, lock, shared state, or lock order is introduced. The fixture saves and restores the bitmap/page-cache configuration it changes.
- Lifecycle and static initialization: writer, file, and reader ownership follow local construction/destruction order; no new cross-translation-unit initializer or long-lived reference is introduced.
- Configuration: no new Doris configuration item or dynamic-change requirement is introduced. The golden-output environment variable is test-only.
- Compatibility and rolling upgrades: the PR adds an oracle but no new serialized format, protocol symbol, or FE/BE field. Production primary-key PREFIX single-page bytes are also covered by existing strict Segment goldens; multi-page B-tree index pages are outside this PR's expressly scoped Scalar/Offset data-page encoding goldens and have a dedicated functional test.
- Parallel paths and conditions: normal STRUCT/MAP nullable writes use their overrides; sparse VARIANT calls direct append_nulls only for supported scalar/ARRAY physical paths. The FOR exclusion follows the lack of a current production data-column default. Existing ARRAY/MAP composite read coverage remains the prior-thread concern.
- Tests and negative coverage: generated rows include NULL runs, extrema, over-page strings/JSON, dictionary overflow, and repeated writes across pages. There is no new error behavior needing a negative test. This review ran no BE unit, regression, style, or build command.
- Test results: the seven binary goldens match layout lengths, footer fields, and checksums; no runtime test result was independently produced. The prior CHAR and layout findings limit read-back and diagnostic coverage as noted above.
- Observability: no new runtime path needs metrics or logging; existing Status propagation is retained.
- Transactions and persistence: no EditLog or transaction/version behavior changes. Existing persisted page bytes are the compatibility target of the new fixtures.
- Data writes, crashes, and atomicity: no reachable normal write or publish path changed. The new NotSupported returns apply only to direct STRUCT/MAP null insertion; caller and typed-path tracing found no supported production input reaching them.
- FE/BE variables: none added or transmitted.
- Performance: no new runtime hot-path work; fixture-only repeated writes are bounded. No distinct CPU or memory regression was substantiated.
- Other issues: a possible IndexedColumnWriter multi-page index golden gap was investigated separately and dismissed as outside the stated data-page encoding scope; its existing multi-page unit test checks reader behavior. A NaN reconversion concern lacked a changed decoder path or demonstrated user-visible failure.
Review completion: every changed path and candidate was swept, the live PR base/head and inline threads were refreshed, and all round-two reviewers returned NO_NEW_VALUABLE_FINDINGS. The two still-applicable issues above are already in inline threads.
|
run buildall |
TPC-H: Total hot run time: 27948 ms |
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Remove code the storage write path no longer reaches: - get_zone_map_value() in column_predicate.h has no caller. - ColumnWriter::_null_bitmap is never read or written (the null bitmap lives in _null_bitmap_builder). - StructColumnWriter / MapColumnWriter::append_nulls() are unreachable: both override the vectorized append_nullable(), whose bodies never call append_nulls(); the bitmap append_nullable() that does is only entered through append(cell) / append(bool, void*), which only scalar writers get; and the variant writers pad only scalar and array writers with NULLs. They now return NotSupported, as the variant writers already do. ### Release note None ### Check List (For Author) - Test: No need to test (removes code without callers; covered by the BE build and the existing unit tests) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
TPC-DS: Total hot run time: 152572 ms |
ClickBench: Total hot run time: 24.04 s |
…iles
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Nothing pins the bytes of the pages a column writer writes.
The bitshuffle, plain, RLE, frame-of-reference, binary plain and binary prefix
page tests are excluded from the UT build, and the page tests that do run
round-trip values without comparing bytes. SegmentFlusherFormatTest pins whole
Segments, but its columns hold a handful of rows, so no page ever fills up,
and only the encodings the writers pick are written (never PLAIN_V2, for
example).
Add PageFormatTest, in the style of SegmentFlusherFormatTest. For every
registered (storage type, page encoding) pair - DICT once for V1/V2 Segments
(PLAIN dictionary page) and once for V3 (PLAIN_V3 dictionary page), AGG_STATE
once per serialized shape - deterministic rows are converted by
OlapBlockDataConvertor and appended block by block to a ScalarColumnWriter,
the way a Segment writer appends them, with small page budgets so that every
case spans several pages. Array and map offsets go to an OffsetColumnWriter,
as they do in a Segment, so their page footers also pin the next array item
ordinal. A file writer keeps each page PageIO writes. The goldens under
be/test/storage/test_data/page_format are one file per page encoding
(bit_shuffle.dat, dict.dat, plain.dat, plain_v2.dat, plain_v3.dat, prefix.dat,
rle.dat), in which the pages of each column follow one another as they are on
disk: the encoded values, the null map, the page footer and the checksum.
layout.txt lists every page with its column, rows and size, and a failure
names the page that changed. Pages are written with NO_COMPRESSION, so they
hold what the page builder encoded; for bitshuffle pages, dictionary codes
included, that is LZ4 output, as the bitshuffle page builder compresses its
values itself. Columns hold NULLs, scattered and in a run, unless they are
never nullable (array and map offsets, and the AGG_STATE, HLL, BITMAP and
QUANTILE_STATE columns FE only creates NOT NULL), so pages both with and
without a null map are pinned, the plain pages an overflowing dictionary falls
back to included.
Each column is also read back through ColumnReader and the iterator a Segment
reads it with, and must hold the rows written. CHAR is compared as it reads
back: without the padding it was written with, which loading a page strips,
except from a PREFIX page, which no writer writes for CHAR. layout.txt covers
all 88 cases, so a new or removed one changes it, and a second test fails when
a storage type gains an encoding without a spec. Two registered types have no
spec: no writer is given UNSIGNED_INT, as array and map offsets are
UNSIGNED_BIGINT, and the root of a VARIANT is JSONB, which the JSONB pages
cover.
FOR is registered for the integer and date types but left out. Its only
writer was the dictionary of a bitmap index, and nothing has written FOR pages
since bitmap indexes were removed; FrameOfReferencePageDecoder cannot read the
values back; and a frame that keeps its original values is written with bytes
ForEncoder never initializes, so its pages are not even the same from run to
run.
The rows cover narrow runs, full-width noise and extremes of every integer
width, array offsets, NaN with a sign or a payload (written as the canonical
NaN), signed zeros, infinities and denormals, dates at the edges of their
range, decimals at their precision bounds, empty, UTF-8, trailing-space and
oversize strings, a dictionary that overflows into plain pages, empty, single
and sparse HLLs, empty, single and Roaring bitmaps, and quantile states. The
test pins bitmap_serialize_version and enable_set_in_bitmap_value to the
defaults the goldens were written with, as other tests leave them changed.
Regenerate the goldens with DORIS_PAGE_FORMAT_GOLDEN_OUTPUT_DIR set to a
directory outside the checked-in one, and only for an intended format change.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- PageFormatTest passes against the goldens generated from this tree, also
when run after function_bitmap_test and BitmapValueTest, which leave
bitmap configs changed
- Changing a page byte, a layout.txt line or the length of a golden file,
or adding a stray golden file, each fails the test and names the page or
file
- Behavior changed: No
- Does this need documentation: No
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
fc55d1b to
8b390de
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Review complete for head 8b390def0047f51d4bdb218c65b7dec73d7b55f2 after two review rounds, each with two full-review passes and a risk-focused pass. I found no new, nonduplicate issue to post inline. This was a static review only; the review instructions prohibited builds and tests.
Goal and coverage. The change removes unused storage-writer code and adds byte goldens for encoded scalar data and dictionary pages. The new BE test is picked up by the test CMake glob. Its 33 type specs produce 88 cases and 483 recorded pages across seven binary goldens; static inspection found that the layout lengths account for all seven files, with valid page footer bounds and checksums. These checks support the fixture structure but do not establish that the test passes at runtime. FOR is explicitly excluded because its vectorized reader does not support this read-back path. The implementation is focused on this goal, though the large fixture is necessarily more extensive than the dead-code removal.
Correctness, write paths, and parallel paths. Ordinary Segment writes call ColumnWriter::append, which dispatches nullable STRUCT and MAP values to their specialized append_nullable implementations; ARRAY null runs do not call a child writer's append_nulls. Sparse VARIANT paths cannot create a valid MAP/STRUCT subcolumn writer before their direct null-append call. Thus the new NotSupported overrides have no substantiated production caller on this head. The removed zone-map helper and private bitmap member have no remaining in-tree references. No transaction visibility, delete-bitmap, FE/BE variable-passing, or other persisted write protocol is changed.
Concurrency, lifecycle, configuration, and performance. No production thread, lock, static-initialization dependency, or config item is added. The test owns and closes its file handles and restores the existing bitmap/cache settings it changes. No new production hot-path work was found; the repeated writes add cost only to this unit test. No new runtime behavior calls for logs or metrics.
Storage compatibility and tests. The production hunks do not change the on-disk encoding or FE/BE protocol. The new goldens pin encoded data/dictionary pages, including the current OffsetColumnWriter footer bytes. Ordinal INDEX_PAGEs are deliberately outside this fixture; an existing whole-Segment golden contains multi-page ordinal index pages and compares their full bytes. No new end-to-end test was added for composite ARRAY/MAP read-back, and I did not run the new or existing tests. Nulls, value extremes, dictionary fallback, and multi-page cases are represented in the new input matrix; its remaining CHAR and comparison gaps are already reported.
Existing review context. The existing P2 CHAR logical-value comparison (r4129841174) and P3 layout std::mismatch range (r4129841179) still apply. The earlier P2 .dat length issue (r4129585058) is fixed by the current size check. The earlier P2 offset-footer concern (r4129841167) is partly addressed by use of OffsetColumnWriter; the requested composite ARRAY/MAP read-back case is still absent from this fixture. I did not duplicate any of these threads. No existing P0/P1 inline comment remains to list. The one new candidate raised during the first risk pass, ordinal index byte coverage, was dismissed after the second pass established the fixture's data-page scope and the existing whole-Segment byte guard. No suspicious point remains unresolved.
User focus. No additional review focus was supplied.
TPC-H: Total hot run time: 27887 ms |
TPC-DS: Total hot run time: 152425 ms |
ClickBench: Total hot run time: 23.92 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…d by FieldType ### What problem does this PR solve? Issue Number: None Related PR: apache#68608, apache#68609 Problem Summary: How a compute-layer value becomes the value a segment stores is written down in several places. PrimitiveTypeConvertor<PT> gives the V1 DATE, DATETIME and DECIMAL layouts to the key encodings of RowCursor and the BKD index; KeyCoderTraitsForFloat canonicalises NaN on its own; and the comparison and IN-list predicates repeat the V1 conversions by hand to probe a bloom filter. The key-encodable types are listed as (FieldType, PrimitiveType) pairs so that the key helpers can be keyed by PrimitiveType. Add storage/storage_layout.h. For every fixed-width FieldType, StorageLayout<FT> gives the StorageValue a segment stores, the PrimitiveValue the compute layer holds, and to_storage from one to the other: a bit cast, the quiet NaN for every NaN of FLOAT/DOUBLE, and the V1 packing of DATE, DATETIME and DECIMAL. FLOAT and DOUBLE also give that NaN rule as canonicalize_nan, for code that already holds a stored value. Make the PrimitiveType <-> FieldType mapping in storage_field_type.h constexpr, so that a template picks its layout with StorageLayout<primitive_type_to_storage_field_type(PT)>; data_type.h now declares FieldType itself instead of including that header. Move the users onto it: full_encode_field_as_key and so the RowCursor and BKD query key encodings, the BKD +/- infinity sentinels, KeyCoderTraitsForFloat, which canonicalises the NaN of the value it encodes through canonicalize_nan, and the bloom filter probes of the comparison and IN-list predicates. The key-encodable types are listed by FieldType alone. Delete PrimitiveTypeConvertor, whose last user was a test helper. Every conversion is the one it replaces, so no stored or encoded byte changes. The one difference is that a bloom filter probe now canonicalises a NaN, and no bloom filter index is built on FLOAT or DOUBLE. This is the second piece split out of apache#68608; the page encoders and decoders move onto StorageLayout in later pieces. ### Release note None ### Check List (For Author) - Test: Unit Test - StorageLayoutTest (new): the StorageValue size of every fixed-width FieldType, the V1 DATE, DATETIME and DECIMAL packing, NaN canonicalisation and bit casts - KeyCoderTest (key order of full_encode_field_as_key for every key type), TestRowCursor, StorageFieldTypeTest, DateBloomFilterTest, BloomFilterIndexReaderWriterTest, BlockColumnPredicateTest, PredicateCreatorTest, InvertedIndexReaderTest (BKD range queries of every key type), SniiBkd*, and SegmentFlusherFormatTest and SegmentFlusherTransformFormatTest with strict golden comparison - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…d by FieldType ### What problem does this PR solve? Issue Number: None Related PR: apache#68608, apache#68609 Problem Summary: How a compute-layer value becomes the value a segment stores is written down in several places. PrimitiveTypeConvertor<PT> gives the V1 DATE, DATETIME and DECIMAL layouts to the key encodings of RowCursor and the BKD index; KeyCoderTraitsForFloat canonicalises NaN on its own; and the comparison and IN-list predicates repeat the V1 conversions by hand to probe a bloom filter. The key-encodable types are listed as (FieldType, PrimitiveType) pairs so that the key helpers can be keyed by PrimitiveType. Add storage/storage_layout.h. For every fixed-width FieldType, StorageLayout<FT> gives the StorageValue a segment stores, the PrimitiveValue the compute layer holds, and to_storage from one to the other: a bit cast, the quiet NaN for every NaN of FLOAT/DOUBLE, and the V1 packing of DATE, DATETIME and DECIMAL. FLOAT and DOUBLE also give that NaN rule as canonicalize_nan, for code that already holds a stored value. StorageValue and PrimitiveValue are taken from the two layers' own tables, CppTypeTraits and PrimitiveTypeTraits, and defined nowhere else. Make the PrimitiveType <-> FieldType mapping in storage_field_type.h constexpr, so that a template picks its layout with StorageLayout<primitive_type_to_storage_field_type(PT)>; data_type.h now declares FieldType itself instead of including that header. Move the users onto it: full_encode_field_as_key and so the RowCursor and BKD query key encodings, the BKD +/- infinity sentinels, KeyCoderTraitsForFloat, which canonicalises the NaN of the value it encodes through canonicalize_nan, and the bloom filter probes of the comparison and IN-list predicates. The key-encodable types are listed by FieldType alone. Delete PrimitiveTypeConvertor, whose last user was a test helper. Every conversion is the one it replaces, so no stored or encoded byte changes. The one difference is that a bloom filter probe now canonicalises a NaN, and no bloom filter index is built on FLOAT or DOUBLE. This is the second piece split out of apache#68608; the page encoders and decoders move onto StorageLayout in later pieces. ### Release note None ### Check List (For Author) - Test: Unit Test - StorageLayoutTest (new): the StorageValue size of every fixed-width FieldType, the V1 DATE, DATETIME and DECIMAL packing, NaN canonicalisation and bit casts - KeyCoderTest (key order of full_encode_field_as_key for every key type), TestRowCursor, StorageFieldTypeTest, DateBloomFilterTest, BloomFilterIndexReaderWriterTest, BlockColumnPredicateTest, PredicateCreatorTest, InvertedIndexReaderTest (BKD range queries of every key type), SniiBkd*, and SegmentFlusherFormatTest and SegmentFlusherTransformFormatTest with strict golden comparison - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…d by FieldType ### What problem does this PR solve? Issue Number: None Related PR: apache#68608, apache#68609 Problem Summary: How a compute-layer value becomes the value a segment stores is written down in several places. PrimitiveTypeConvertor<PT> gives the V1 DATE, DATETIME and DECIMAL layouts to the key encodings of RowCursor and the BKD index; KeyCoderTraitsForFloat canonicalises NaN on its own; and the comparison and IN-list predicates repeat the V1 conversions by hand to probe a bloom filter. The key-encodable types are listed as (FieldType, PrimitiveType) pairs so that the key helpers can be keyed by PrimitiveType. Add storage/storage_layout.h. For every fixed-width FieldType, StorageLayout<FT> gives the StorageValue a segment stores, the PrimitiveValue the compute layer holds, and to_storage from one to the other: a bit cast, the quiet NaN for every NaN of FLOAT/DOUBLE, and the V1 packing of DATE, DATETIME and DECIMAL. FLOAT and DOUBLE also give that NaN rule as canonicalize_nan, for code that already holds a stored value. The types are the two layers' own, PrimitiveTypeTraits<PT>::CppType and CppTypeTraits<FT>::CppType, and callers name them that way; the StorageValue and PrimitiveValue aliases inside StorageLayout are its private shorthand. Make the PrimitiveType <-> FieldType mapping in storage_field_type.h constexpr, so that a template picks its layout with StorageLayout<primitive_type_to_storage_field_type(PT)>; data_type.h now declares FieldType itself instead of including that header. Move the users onto it: full_encode_field_as_key and so the RowCursor and BKD query key encodings, the BKD +/- infinity sentinels, KeyCoderTraitsForFloat, which canonicalises the NaN of the value it encodes through canonicalize_nan, and the bloom filter probes of the comparison and IN-list predicates. The key-encodable types are listed by FieldType alone. Delete PrimitiveTypeConvertor, whose last user was a test helper. Every conversion is the one it replaces, so no stored or encoded byte changes. The one difference is that a bloom filter probe now canonicalises a NaN, and no bloom filter index is built on FLOAT or DOUBLE. This is the second piece split out of apache#68608; the page encoders and decoders move onto StorageLayout in later pieces. ### Release note None ### Check List (For Author) - Test: Unit Test - StorageLayoutTest (new): the StorageValue size of every fixed-width FieldType, the V1 DATE, DATETIME and DECIMAL packing, NaN canonicalisation and bit casts - KeyCoderTest (key order of full_encode_field_as_key for every key type), TestRowCursor, StorageFieldTypeTest, DateBloomFilterTest, BloomFilterIndexReaderWriterTest, BlockColumnPredicateTest, PredicateCreatorTest, InvertedIndexReaderTest (BKD range queries of every key type), SniiBkd*, and SegmentFlusherFormatTest and SegmentFlusherTransformFormatTest with strict golden comparison - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…d by FieldType ### What problem does this PR solve? Issue Number: None Related PR: apache#68608, apache#68609 Problem Summary: How a compute-layer value becomes the value a segment stores is written down in several places. PrimitiveTypeConvertor<PT> gives the V1 DATE, DATETIME and DECIMAL layouts to the key encodings of RowCursor and the BKD index; KeyCoderTraitsForFloat canonicalises NaN on its own; and the comparison and IN-list predicates repeat the V1 conversions by hand to probe a bloom filter. The key-encodable types are listed as (FieldType, PrimitiveType) pairs so that the key helpers can be keyed by PrimitiveType. Add storage/storage_layout.h. For every fixed-width FieldType, StorageLayout<FT> gives the StorageValue a segment stores, the PrimitiveValue the compute layer holds, and to_storage from one to the other: a bit cast, the quiet NaN for every NaN of FLOAT/DOUBLE, and the V1 packing of DATE, DATETIME and DECIMAL. FLOAT and DOUBLE also give that NaN rule as canonicalize_nan, for code that already holds a stored value. The types are the two layers' own, PrimitiveTypeTraits<PT>::CppType and CppTypeTraits<FT>::CppType, and callers name them that way; the StorageValue and PrimitiveValue aliases inside StorageLayout are its private shorthand. Make the PrimitiveType <-> FieldType mapping in storage_field_type.h constexpr, so that a template picks its layout with StorageLayout<primitive_type_to_storage_field_type(PT)>; data_type.h now declares FieldType itself instead of including that header. Move the users onto it: full_encode_field_as_key and so the RowCursor and BKD query key encodings, the BKD +/- infinity sentinels, KeyCoderTraitsForFloat, which canonicalises the NaN of the value it encodes through canonicalize_nan, and the bloom filter probes of the comparison and IN-list predicates. The key-encodable types are listed by FieldType alone. Delete PrimitiveTypeConvertor, whose last user was a test helper. Every conversion is the one it replaces, so no stored or encoded byte changes. The one difference is that a bloom filter probe now canonicalises a NaN, and no bloom filter index is built on FLOAT or DOUBLE. This is the second piece split out of apache#68608; the page encoders and decoders move onto StorageLayout in later pieces. ### Release note None ### Check List (For Author) - Test: Unit Test - StorageLayoutTest (new): the StorageValue size of every fixed-width FieldType, the V1 DATE, DATETIME and DECIMAL packing, NaN canonicalisation and bit casts - KeyCoderTest (key order of full_encode_field_as_key for every key type), TestRowCursor, StorageFieldTypeTest, DateBloomFilterTest, BloomFilterIndexReaderWriterTest, BlockColumnPredicateTest, PredicateCreatorTest, InvertedIndexReaderTest (BKD range queries of every key type), SniiBkd*, and SegmentFlusherFormatTest and SegmentFlusherTransformFormatTest with strict golden comparison - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…d by FieldType (#68664) ### What problem does this PR solve? Issue Number: None Related PR: #68608, #68609 Problem Summary: How a compute-layer value becomes the value a segment stores is written down in several places. PrimitiveTypeConvertor<PT> gives the V1 DATE, DATETIME and DECIMAL layouts to the key encodings of RowCursor and the BKD index; KeyCoderTraitsForFloat canonicalises NaN on its own; and the comparison and IN-list predicates repeat the V1 conversions by hand to probe a bloom filter. The key-encodable types are listed as (FieldType, PrimitiveType) pairs so that the key helpers can be keyed by PrimitiveType. Add storage/storage_layout.h. For every fixed-width FieldType, StorageLayout<FT> gives the StorageValue a segment stores, the PrimitiveValue the compute layer holds, and to_storage from one to the other: a bit cast, the quiet NaN for every NaN of FLOAT/DOUBLE, and the V1 packing of DATE, DATETIME and DECIMAL. Make the PrimitiveType <-> FieldType mapping in storage_field_type.h constexpr, so that a template picks its layout with StorageLayout<primitive_type_to_storage_field_type(PT)>; data_type.h now declares FieldType itself instead of including that header. Move the users onto it: full_encode_field_as_key and so the RowCursor and BKD query key encodings, the BKD +/- infinity sentinels, the NaN handling of KeyCoderTraitsForFloat, and the bloom filter probes of the comparison and IN-list predicates. The key-encodable types are listed by FieldType alone. Delete PrimitiveTypeConvertor, whose last user was a test helper. ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
…padding ### What problem does this PR solve? Issue Number: None Related PR: apache#68608, apache#68609, apache#68664 Problem Summary: The short key, primary key and cluster key encodings, and the MoW probes of partial update, row binlog derivation and historical row lookup, read their key columns through OlapBlockDataConvertor, the copy of each block the segment writer makes in storage layout. That copy is what pads CHAR values to their declared length, so the key encodings depended on it for the padding a short key needs, and every data page, dictionary, zone map, bloom filter and inverted index of a CHAR column stored the padding too. RowKeyEncoder now encodes from the block's own columns at absolute row numbers: a fixed-width key value goes through StorageLayout<FT>::storage_at, and a CHAR key is padded into the key by StorageLayout<CHAR>::append_padded, which the RowCursor seek keys use as well. VerticalSegmentWriter collects its key, sequence and cluster key columns from the block, and the MoW probe paths pass block columns instead of converting them. The keys are byte for byte the ones the converter path produced. With no one left reading padded CHAR values from the converter, it stops padding them: a CHAR column is converted like VARCHAR, and its data pages, dictionary pages, zone maps, bloom filters and inverted indexes store the value's own bytes. Segments written before still read the same: the CHAR page pre-decoders strip the padding of data and dictionary pages, a zone map bound of a CHAR column is cut at its first '\0', and CLucene cuts an indexed value at its first '\0'. A bloom filter cannot tell padded hashes from unpadded ones, which is why CHAR predicates already skip bloom filters, and they keep doing so. The goldens that change are the 6 CHAR cases of PageFormatTest and the 32 SegmentFlusherFormatTest cases with a CHAR column; the other 82 and 41 cases keep their bytes. ### Release note CHAR columns are stored without their zero padding outside the key encodings, so their data pages and zone maps get smaller. Query results do not change. ### Check List (For Author) - Test: Unit Test - StorageLayoutTest (storage_at, append_padded), RowKeyEncoderTest, KeyCoderTest, KeyProbeTest, RowCacheProbeTest, FixedPartialUpdateTest, FlexiblePartialUpdateTest, HistoricalRowRetrieverTest, VerticalSegmentWriterWritePathsTest (two new MoW cases: appends past row 0, and a sequence column grouped away from its schema position), VerticalSegmentWriterMowCheckTest, PageFormatTest, SegmentFlusherFormatTest and SegmentFlusherTransformFormatTest with strict golden comparison, ColumnZoneMapTest, BinaryPlainPageV2Test, BinaryPlainPageV3Test, EncodingInfoTest, ColumnReaderTest, TestRowCursor, OlapDataConvertorTest - Behavior changed: Yes. New segments store CHAR unpadded outside the key encodings; reads of old and new segments return the same values. - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]>
…padding Issue Number: None Related PR: apache#68608, apache#68609, apache#68664 Problem Summary: The short key, primary key and cluster key encodings, and the MoW probes of partial update, row binlog derivation and historical row lookup, read their key columns through OlapBlockDataConvertor, the copy of each block the segment writer makes in storage layout. That copy is what pads CHAR values to their declared length, so the key encodings depended on it for the padding a short key needs, and every data page, dictionary, zone map, bloom filter and inverted index of a CHAR column stored the padding too. RowKeyEncoder now encodes from the block's own columns at absolute row numbers: a fixed-width key value goes through StorageLayout<FT>::storage_at, and a CHAR key is padded into the key by StorageLayout<CHAR>::append_padded, which the RowCursor seek keys use as well. VerticalSegmentWriter collects its key, sequence and cluster key columns from the block, and the MoW probe paths pass block columns instead of converting them. The keys are byte for byte the ones the converter path produced. With no one left reading padded CHAR values from the converter, it stops padding them: a CHAR column is converted like VARCHAR, and its data pages, dictionary pages, zone maps, bloom filters and inverted indexes store the value's own bytes. Segments written before still read the same: the CHAR page pre-decoders strip the padding of data and dictionary pages, a zone map bound of a CHAR column is cut at its first '\0', and CLucene cuts an indexed value at its first '\0'. A bloom filter cannot tell padded hashes from unpadded ones, which is why CHAR predicates already skip bloom filters, and they keep doing so. The goldens that change are the 6 CHAR cases of PageFormatTest and the 32 SegmentFlusherFormatTest cases with a CHAR column; the other 82 and 41 cases keep their bytes. CHAR columns are stored without their zero padding outside the key encodings, so their data pages and zone maps get smaller. Query results do not change. - Test: Unit Test - StorageLayoutTest (storage_at, append_padded), RowKeyEncoderTest, KeyCoderTest, KeyProbeTest, RowCacheProbeTest, FixedPartialUpdateTest, FlexiblePartialUpdateTest, HistoricalRowRetrieverTest, VerticalSegmentWriterWritePathsTest (two new MoW cases: appends past row 0, and a sequence column grouped away from its schema position), VerticalSegmentWriterMowCheckTest, PageFormatTest, SegmentFlusherFormatTest and SegmentFlusherTransformFormatTest with strict golden comparison, ColumnZoneMapTest, BinaryPlainPageV2Test, BinaryPlainPageV3Test, EncodingInfoTest, ColumnReaderTest, TestRowCursor, OlapDataConvertorTest - Behavior changed: Yes. New segments store CHAR unpadded outside the key encodings; reads of old and new segments return the same values. - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]>
…ockDataConvertor ### What problem does this PR solve? Related PR: apache#68608, apache#68609, apache#68664 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. Convert directly into Plain/Bitshuffle buffers, bound FOR conversion scratch, and reuse string and object serialization buffers. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. ### Release note None ### Check List (For Author) - Test: Unit Test - ASAN BE build and selected BE UT build with -j 128; 129 tests in 14 suites - Writer/physical-page byte and boundary comparison, page format golden tests, storage layout tests, and vertical segment writer tests - clang-format 16, build hygiene, and clang-tidy on changed lines passed - These checks ran on the identical source tree before this history-only squash; the before/after Git tree IDs were verified equal - Full cluster SQL regression and throughput benchmarks were not rerun for the final page-interface refactoring - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor ### What problem does this PR solve? Related PR: apache#68608, apache#68609, apache#68664 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. Convert directly into Plain/Bitshuffle buffers, bound FOR conversion scratch, and reuse string and object serialization buffers. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. ### Release note None ### Check List (For Author) - Test: Unit Test - ASAN BE build and selected BE UT build with -j 128; 129 tests in 14 suites - Writer/physical-page byte and boundary comparison, page format golden tests, storage layout tests, and vertical segment writer tests - clang-format 16, build hygiene, and clang-tidy on changed lines passed - These checks ran on the identical source tree before this history-only squash; the before/after Git tree IDs were verified equal - Full cluster SQL regression and throughput benchmarks were not rerun for the final page-interface refactoring - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor ### What problem does this PR solve? Related PR: apache#68608, apache#68609, apache#68664, apache#69221 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. The scalar writer follows the structure StarRocks' ScalarColumnWriter::_append uses: a batch is first turned into contiguous storage cells, then one loop in the base class splits the cells into NULL and value runs, feeds every run to the page builder and the indexes, and flushes full pages. - `StorageLayout<FT>::column_to_storage` has one signature for every type: `(column, row_pos, n, StorageBuffers&, null_map)`. It borrows the column's bytes when they are stored unchanged and otherwise fills the caller's `StorageBuffers` (converted bytes and Slice descriptors). CHAR keeps a length-taking overload that pads; BITMAP/HLL/QUANTILE_STATE serialise a whole batch the way the removed convertor did; AGG_STATE describes fixed-length objects. - `ScalarColumnWriter` is one concrete class: `append` peels the null map, `prepare_cells` turns the batch into cells through the StorageLayout of `_layout_type` (the column's type, or what an AGG_STATE function serialises to, decided once at construction) and rejects a batch before anything is written, and `append_values` is the single run/page loop that feeds the page builder, every index, the row ids and the null bits. The composite writers feed their null maps through `append_values` directly; the offset writer only adds the `n + 1` offsets check and annotates the page footer from `page_accepted`, which replaces `FlushPageCallback`. - A batch or run is converted at once, without 1024-row chunking, in the writers, the array index feed and BUILD INDEX; memory per batch is at most what the removed convertor held for one column of the block. CHAR is padded by the writers and by BUILD INDEX per batch through the CHAR layout; the `is_padded`/`pad_column` helpers are gone. - `ArrayColumnWriter` checks the ANN item type at init, feeds an index only when it created its writer, and reuses its offsets buffer for `append_nulls`. Rebased on apache#69221: the chunked `VariantSubcolumnWriter` flushes its value chunks through `append_sparse_column`, the streaming root writer appends each root batch as a column, and all-NULL ARRAY chunks keep taking the array path. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. ### Release note None ### Check List (For Author) - Test: Unit Test - ASAN BE build; BE UT: StorageLayoutTest, PageBuilderColumnTest, ColumnWriterStorageTest, PageFormatTest, ArrayIndexInputTest, IndexBuilderTest, AnnIndexWriterTest, InvertedIndexWriterTest, InvertedIndexArrayTest, CharTypePaddingTest, the variant writer/reader suites, and every suite of the test files that include the segment and column writers, the segment flusher format cases and the key coders - clang-format 16, build hygiene, and clang-tidy on changed lines - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor ### What problem does this PR solve? Related PR: apache#68608, apache#68609, apache#68664, apache#69221 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. The scalar writer follows the structure StarRocks' ScalarColumnWriter::_append uses: a batch is first turned into contiguous storage cells, then one loop in the base class splits the cells into NULL and value runs, feeds every run to the page builder and the indexes, and flushes full pages. - `StorageLayout<FT>::column_to_storage` has one signature for every type: `(column, row_pos, n, StorageBuffers&)`. It borrows the column's bytes when they are stored unchanged and otherwise fills the caller's `StorageBuffers` (converted bytes and Slice descriptors). Every row of the range is converted, NULL rows included, since no consumer reads a NULL row's cell. CHAR keeps a length-taking overload that pads and takes the null map: a NULL row can hold a value longer than the column, which must not be copied into its slot. BITMAP/HLL/QUANTILE_STATE serialise a whole batch the way the removed convertor did; AGG_STATE stores strings, bitmaps or fixed-length objects according to the column holding the states. - `ScalarColumnWriter` is one concrete class again, constructed directly as on master: `append` peels the null map, `prepare_cells` turns the whole batch into cells through the StorageLayout of the column's type and rejects a batch before anything is written, and `append_values` is the single run/page loop that feeds the page builder, every index, the row ids and the null bits. The composite writers feed their null maps through `append_values` directly. The offset writer keeps master's `FlushPageCallback`; it only adds the `n + 1` offsets check and records the next item ordinal in `page_accepted`, where master's copy of the page loop did. - A batch or run is converted at once, without 1024-row chunking, in the writers, the array index feed and BUILD INDEX; memory per batch is at most what the removed convertor held for one column of the block. CHAR is padded by the writers and by BUILD INDEX per batch through the CHAR layout; the `is_padded`/`pad_column` helpers are gone. - `ArrayColumnWriter` checks the ANN item type at init, feeds an index only when it created its writer, and reuses its offsets buffer for `append_nulls`. Rebased on apache#69221: the chunked `VariantSubcolumnWriter` flushes its value chunks through `append_sparse_column`, the streaming root writer appends each root batch as a column, and all-NULL ARRAY chunks keep taking the array path. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. ### Release note None ### Check List (For Author) - Test: Unit Test - ASAN BE build; BE UT: StorageLayoutTest, PageBuilderColumnTest, ColumnWriterStorageTest, PageFormatTest, ArrayIndexInputTest, IndexBuilderTest, AnnIndexWriterTest, InvertedIndexWriterTest, InvertedIndexArrayTest, CharTypePaddingTest, the variant writer/reader suites, and every suite of the test files that include the segment and column writers, the segment flusher format cases and the key coders - clang-format 16, build hygiene, and clang-tidy on changed lines - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor ### What problem does this PR solve? Related PR: apache#68608, apache#68609, apache#68664, apache#69221 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. The scalar writer keeps master's shape: a batch is cut into NULL runs and value runs, and each value run is turned into contiguous storage cells and written by one page loop in the base class, which feeds the page builder and the indexes and flushes full pages. - `StorageLayout<FT>::column_to_storage` has one signature for every type: `(column, row_pos, n, StorageBuffers&)`. It borrows the column's bytes when they are stored unchanged and otherwise fills the caller's `StorageBuffers` (converted bytes and Slice descriptors). Every row of the range is converted, and no layout takes a null map. CHAR keeps a length-taking overload that pads, which needs a range without NULL rows: a NULL row can hold a value longer than the column, which must not be copied into its slot. BITMAP/HLL/QUANTILE_STATE serialise a whole batch the way the removed convertor did. AGG_STATE's layout stores fixed-length objects; string and bitmap states go through the VARCHAR and BITMAP layouts, which `create_agg_state_writer` picks from the function's serialised type when it creates the writer, as master picked the convertor. A runtime `column_to_storage(type, ...)` maps a FieldType to its layout in one place for the writer, the array index feed and BUILD INDEX; it refuses CHAR, whose callers decide whether to pad. - `ScalarColumnWriter` is one concrete class again, constructed directly as on master, and converts through the layout of its storage type: the column's type, or what an AGG_STATE function serialises to. `append` checks the whole batch for admission first, so a rejected batch writes nothing, then cuts it into NULL runs, written by `append_nulls`, and value runs, written by `append_values`: each value run is converted by `prepare_cells` and written by `append_data`, the single page loop that feeds the page builder, every index, the row ids and the null bits. NULL rows are never converted by the writer. The composite writers feed their null maps through `append_data`, as they did through master's `append_data`. The offset writer keeps master's `FlushPageCallback`; it only adds the `n + 1` offsets check and records the next item ordinal in `page_accepted`, where master's copy of the page loop did. - Conversion has no 1024-row chunking: the writer converts each value run at once, and the array index feed and BUILD INDEX convert a batch of array rows or a run at once. The array index feed converts NULL elements too, as no index writer reads their cells, except CHAR: it pads only the runs of non-NULL elements, each into its own slots. BUILD INDEX pads CHAR per run for CLucene and BKD, and feeds it as it reads back to SNII. - `ArrayColumnWriter` checks the ANN item type at init, feeds an index only when it created its writer, and reuses its offsets buffer for `append_nulls`. Rebased on apache#69221: the chunked `VariantSubcolumnWriter` flushes its value chunks through `append_sparse_column`, the streaming root writer appends each root batch as a column, and all-NULL ARRAY chunks keep taking the array path. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. ### Release note None ### Check List (For Author) - Test: Unit Test - ASAN BE build; BE UT: StorageLayoutTest, PageBuilderColumnTest, ColumnWriterStorageTest, PageFormatTest, ArrayIndexInputTest, IndexBuilderTest, AnnIndexWriterTest, InvertedIndexWriterTest, InvertedIndexArrayTest, the variant writer/reader suites, and every suite of the test files that include the segment and column writers, the segment flusher format cases and the key coders - clang-format 16, build hygiene, and clang-tidy on the changed sources - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor ### What problem does this PR solve? Related PR: apache#68608, apache#68609, apache#68664, apache#69221 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. The scalar writer keeps master's shape: a batch is cut into NULL runs and value runs, and each value run is turned into contiguous storage cells and written by one page loop in the base class, which feeds the page builder and the indexes and flushes full pages. - `StorageLayout<FT>::column_to_storage` has one signature for every type: `(column, row_pos, n, StorageBuffers&)`. It borrows the column's bytes when they are stored unchanged and otherwise fills the caller's `StorageBuffers` (converted bytes and Slice descriptors). Every row of the range is converted, and no layout takes a null map. CHAR keeps a length-taking overload that pads, which needs a range without NULL rows: a NULL row can hold a value longer than the column, which must not be copied into its slot. BITMAP/HLL/QUANTILE_STATE serialise a whole batch the way the removed convertor did. AGG_STATE's layout stores fixed-length objects; string and bitmap states go through the VARCHAR and BITMAP layouts, which `create_agg_state_writer` picks from the function's serialised type when it creates the writer, as master picked the convertor. A runtime `column_to_storage(type, ...)` maps a FieldType to its layout in one place for the writer, the array index feed and BUILD INDEX; it refuses CHAR, whose callers decide whether to pad. - `ScalarColumnWriter` is one concrete class again, constructed directly as on master, and converts through the layout of its storage type: the column's type, or what an AGG_STATE function serialises to. `append` checks the whole batch for admission first, so a rejected batch writes nothing, then cuts it into NULL runs, written by `append_nulls`, and value runs, written by `append_values`: each value run is converted by `prepare_cells` and written by `append_data`, the single page loop that feeds the page builder, every index, the row ids and the null bits. NULL rows are never converted by the writer. The composite writers feed their null maps through `append_data`, as they did through master's `append_data`. The offset writer keeps master's `FlushPageCallback` and its own copy of the page loop that records the next item ordinal; it only adds the `n + 1` offsets check. - Conversion has no 1024-row chunking: the writer converts each value run at once, and the array index feed and BUILD INDEX convert a batch of array rows or a run at once. The array index feed converts NULL elements too, as no index writer reads their cells, except CHAR: it pads only the runs of non-NULL elements, each into its own slots. BUILD INDEX pads CHAR per run for CLucene and BKD, and feeds it as it reads back to SNII. - `ArrayColumnWriter` checks the ANN item type at init, feeds an index only when it created its writer, and reuses its offsets buffer for `append_nulls`. Rebased on apache#69221: the chunked `VariantSubcolumnWriter` flushes its value chunks through `append_sparse_column`, the streaming root writer appends each root batch as a column, and all-NULL ARRAY chunks keep taking the array path. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. ### Release note None ### Check List (For Author) - Test: Unit Test - ASAN BE build; BE UT: StorageLayoutTest, PageBuilderColumnTest, ColumnWriterStorageTest, PageFormatTest, ArrayIndexInputTest, IndexBuilderTest, AnnIndexWriterTest, InvertedIndexWriterTest, InvertedIndexArrayTest, the variant writer/reader suites, and every suite of the test files that include the segment and column writers, the segment flusher format cases and the key coders - clang-format 16, build hygiene, and clang-tidy on the changed sources - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor ### What problem does this PR solve? Related PR: apache#68608, apache#68609, apache#68664, apache#69221 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. The scalar writer keeps master's shape: a batch is cut into NULL runs and value runs, and each value run is turned into contiguous storage cells and written by one page loop in the base class, which feeds the page builder and the indexes and flushes full pages. - `StorageLayout<FT>::column_to_storage` has one signature for every type: `(column, row_pos, n, StorageBuffers&)`. It borrows the column's bytes when they are stored unchanged and otherwise fills the caller's `StorageBuffers` (converted bytes and Slice descriptors). Every row of the range is converted, and no layout takes a null map. CHAR keeps a length-taking overload that pads, which needs a range without NULL rows: a NULL row can hold a value longer than the column, which must not be copied into its slot. BITMAP/HLL/QUANTILE_STATE serialise a whole batch the way the removed convertor did. AGG_STATE's layout stores fixed-length objects; string and bitmap states go through the VARCHAR and BITMAP layouts, which `create_agg_state_writer` picks from the function's serialised type when it creates the writer, as master picked the convertor. A runtime `column_to_storage(type, ...)` maps a FieldType to its layout in one place for the writer, the array index feed and BUILD INDEX; it refuses CHAR, whose callers decide whether to pad. - `ScalarColumnWriter` is one concrete class again, constructed directly as on master, and converts through the layout of its storage type: the column's type, or what an AGG_STATE function serialises to. `append` checks the whole batch for admission first, so a rejected batch writes nothing, then cuts it into NULL runs, written by `append_nulls`, and value runs, written by `append_values`: each value run is converted by `prepare_cells` and written by `append_data`, the single page loop that feeds the page builder, every index, the row ids and the null bits. NULL rows are never converted by the writer. The composite writers feed their null maps through `append_data`, as they did through master's `append_data`. The offset writer keeps master's `FlushPageCallback` and its own copy of the page loop that records the next item ordinal; it only adds the `n + 1` offsets check. - Conversion has no 1024-row chunking: the writer converts each value run at once, and the array index feed and BUILD INDEX convert a batch of array rows or a run at once. The array index feed converts NULL elements too, as no index writer reads their cells, except CHAR: it pads only the runs of non-NULL elements, each into its own slots. BUILD INDEX pads CHAR per run for CLucene and BKD, and feeds it as it reads back to SNII. - `ArrayColumnWriter` checks the ANN item type at init, feeds an index only when it created its writer, and reuses its offsets buffer for `append_nulls`. Rebased on apache#69221: the chunked `VariantSubcolumnWriter` flushes its value chunks through `append_sparse_column`, the streaming root writer appends each root batch as a column, and all-NULL ARRAY chunks keep taking the array path. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. ### Release note None ### Check List (For Author) - Test: Unit Test - ASAN BE build; BE UT: StorageLayoutTest, PageBuilderColumnTest, ColumnWriterStorageTest, PageFormatTest, ArrayIndexInputTest, IndexBuilderTest, AnnIndexWriterTest, InvertedIndexWriterTest, InvertedIndexArrayTest, the variant writer/reader suites, and every suite of the test files that include the segment and column writers, the segment flusher format cases and the key coders - clang-format 16, build hygiene, and clang-tidy on the changed sources - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor ### What problem does this PR solve? Related PR: apache#68608, apache#68609, apache#68664, apache#69221 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. The scalar writer keeps master's shape: a batch is cut into NULL runs and value runs, and each value run is turned into contiguous storage cells and written by one page loop in the base class, which feeds the page builder and the indexes and flushes full pages. - `StorageLayout<FT>::column_to_storage` has one signature for every type: `(column, row_pos, n, StorageBuffers&)`. It borrows the column's bytes when they are stored unchanged and otherwise fills the caller's `StorageBuffers` (converted bytes and Slice descriptors). Every row of the range is converted, and no layout takes a null map. CHAR keeps a length-taking overload that pads, which needs a range without NULL rows: a NULL row can hold a value longer than the column, which must not be copied into its slot. BITMAP/HLL/QUANTILE_STATE serialise a whole batch the way the removed convertor did. AGG_STATE's layout stores fixed-length objects; string and bitmap states go through the VARCHAR and BITMAP layouts, which `create_agg_state_writer` picks from the function's serialised type when it creates the writer, as master picked the convertor. A runtime `column_to_storage(type, ...)` maps a FieldType to its layout in one place for the writer, the array index feed and BUILD INDEX; it refuses CHAR, whose callers decide whether to pad. - `ScalarColumnWriter` is one concrete class again, constructed directly as on master, and converts through the layout of its storage type: the column's type, or what an AGG_STATE function serialises to. `append` checks the whole batch for admission first, so a rejected batch writes nothing, then cuts it into NULL runs, written by `append_nulls`, and value runs, written by `append_values`: each value run is converted by `prepare_cells` and written by `append_data`, the single page loop that feeds the page builder, every index, the row ids and the null bits. NULL rows are never converted by the writer. The composite writers feed their null maps through `append_data`, as they did through master's `append_data`. The offset writer keeps master's `FlushPageCallback` and its own copy of the page loop that records the next item ordinal; it only adds the `n + 1` offsets check. - Conversion has no 1024-row chunking: the writer converts each value run at once, and the array index feed and BUILD INDEX convert a batch of array rows or a run at once. The array index feed converts NULL elements too, as no index writer reads their cells, except CHAR: it pads only the runs of non-NULL elements, each into its own slots. BUILD INDEX pads CHAR per run for CLucene and BKD, and feeds it as it reads back to SNII. - `ArrayColumnWriter` checks the ANN item type at init, feeds an index only when it created its writer, and reuses its offsets buffer for `append_nulls`. Rebased on apache#69221: the chunked `VariantSubcolumnWriter` flushes its value chunks through `append_sparse_column`, the streaming root writer appends each root batch as a column, and all-NULL ARRAY chunks keep taking the array path. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. ### Release note None ### Check List (For Author) - Test: Unit Test - ASAN BE build; BE UT: StorageLayoutTest, PageBuilderColumnTest, ColumnWriterStorageTest, PageFormatTest, ArrayIndexInputTest, IndexBuilderTest, AnnIndexWriterTest, InvertedIndexWriterTest, InvertedIndexArrayTest, the variant writer/reader suites, and every suite of the test files that include the segment and column writers, the segment flusher format cases and the key coders - clang-format 16, build hygiene, and clang-tidy on the changed sources - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor Related PR: apache#68608, apache#68609, apache#68664, apache#69221 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. The scalar writer keeps master's shape: a batch is cut into NULL runs and value runs, and each value run is turned into contiguous storage cells and written by one page loop in the base class, which feeds the page builder and the indexes and flushes full pages. - `StorageLayout<FT>::column_to_storage` has one signature for every type: `(column, row_pos, n, StorageBuffers&)`. It borrows the column's bytes when they are stored unchanged and otherwise fills the caller's `StorageBuffers` (converted bytes and Slice descriptors). Every row of the range is converted, and no layout takes a null map. CHAR keeps a length-taking overload that pads, which needs a range without NULL rows: a NULL row can hold a value longer than the column, which must not be copied into its slot. BITMAP/HLL/QUANTILE_STATE serialise a whole batch the way the removed convertor did. AGG_STATE's layout stores fixed-length objects; string and bitmap states go through the VARCHAR and BITMAP layouts, which `create_agg_state_writer` picks from the function's serialised type when it creates the writer, as master picked the convertor. A runtime `column_to_storage(type, ...)` maps a FieldType to its layout in one place for the writer, the array index feed and BUILD INDEX; it refuses CHAR, whose callers decide whether to pad. - `ScalarColumnWriter` is one concrete class again, constructed directly as on master, and converts through the layout of its storage type: the column's type, or what an AGG_STATE function serialises to. `append` checks the whole batch for admission first, so a rejected batch writes nothing, then cuts it into NULL runs, written by `append_nulls`, and value runs, written by `append_values`: each value run is converted by `prepare_cells` and written by `append_data`, the single page loop that feeds the page builder, every index, the row ids and the null bits. NULL rows are never converted by the writer. The composite writers feed their null maps through `append_data`, as they did through master's `append_data`. The offset writer keeps master's `FlushPageCallback` and its own copy of the page loop that records the next item ordinal; it only adds the `n + 1` offsets check. - Conversion has no 1024-row chunking: the writer converts each value run at once, and the array index feed and BUILD INDEX convert a batch of array rows or a run at once. The array index feed converts NULL elements too, as no index writer reads their cells, except CHAR: it pads only the runs of non-NULL elements, each into its own slots. BUILD INDEX pads CHAR per run for CLucene and BKD, and feeds it as it reads back to SNII. - `ArrayColumnWriter` checks the ANN item type at init, feeds an index only when it created its writer, and reuses its offsets buffer for `append_nulls`. Rebased on apache#69221: the chunked `VariantSubcolumnWriter` flushes its value chunks through `append_sparse_column`, the streaming root writer appends each root batch as a column, and all-NULL ARRAY chunks keep taking the array path. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. None - Test: Unit Test - ASAN BE build; BE UT: StorageLayoutTest, PageBuilderColumnTest, ColumnWriterStorageTest, PageFormatTest, ArrayIndexInputTest, IndexBuilderTest, AnnIndexWriterTest, InvertedIndexWriterTest, InvertedIndexArrayTest, the variant writer/reader suites, and every suite of the test files that include the segment and column writers, the segment flusher format cases and the key coders - clang-format 16, build hygiene, and clang-tidy on the changed sources - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor Related PR: apache#68608, apache#68609, apache#68664, apache#69221 Problem Summary: Segment writing materializes block columns into intermediate storage buffers through OlapBlockDataConvertor before feeding column writers, indexes, and key encoders. Remove that intermediate converter and let these consumers read column ranges directly through IColumn, row position, and count. Share nullable-run processing, array index inputs, and StorageLayout conversion. Encode keys from block columns and migrate partial-update, variant, and BUILD INDEX paths. Keep page builders on one raw-byte add interface, with ColumnWriter supplying physical fixed-width values or Slice descriptors and recording only the rows accepted by each page. The scalar writer keeps master's shape: a batch is cut into NULL runs and value runs, and each value run is turned into contiguous storage cells and written by one page loop in the base class, which feeds the page builder and the indexes and flushes full pages. - `StorageLayout<FT>::column_to_storage` has one signature for every type: `(column, row_pos, n, StorageBuffers&)`. It borrows the column's bytes when they are stored unchanged and otherwise fills the caller's `StorageBuffers` (converted bytes and Slice descriptors). Every row of the range is converted, and no layout takes a null map. CHAR keeps a length-taking overload that pads, which needs a range without NULL rows: a NULL row can hold a value longer than the column, which must not be copied into its slot. BITMAP/HLL/QUANTILE_STATE serialise a whole batch the way the removed convertor did. AGG_STATE's layout stores fixed-length objects; string and bitmap states go through the VARCHAR and BITMAP layouts, which `create_agg_state_writer` picks from the function's serialised type when it creates the writer, as master picked the convertor. A runtime `column_to_storage(type, ...)` maps a FieldType to its layout in one place for the writer, the array index feed and BUILD INDEX; it refuses CHAR, whose callers decide whether to pad. - `ScalarColumnWriter` is one concrete class again, constructed directly as on master, and converts through the layout of its storage type: the column's type, or what an AGG_STATE function serialises to. `append` checks the whole batch for admission first, so a rejected batch writes nothing, then cuts it into NULL runs, written by `append_nulls`, and value runs, written by `append_values`: each value run is converted by `prepare_cells` and written by `append_data`, the single page loop that feeds the page builder, every index, the row ids and the null bits. NULL rows are never converted by the writer. The composite writers feed their null maps through `append_data`, as they did through master's `append_data`. The offset writer keeps master's `FlushPageCallback` and its own copy of the page loop that records the next item ordinal; it only adds the `n + 1` offsets check. - Conversion has no 1024-row chunking: the writer converts each value run at once, and the array index feed and BUILD INDEX convert a batch of array rows or a run at once. The array index feed converts NULL elements too, as no index writer reads their cells, except CHAR: it pads only the runs of non-NULL elements, each into its own slots. BUILD INDEX pads CHAR per run for CLucene and BKD, and feeds it as it reads back to SNII. - `ArrayColumnWriter` checks the ANN item type at init, feeds an index only when it created its writer, and reuses its offsets buffer for `append_nulls`. Rebased on apache#69221: the chunked `VariantSubcolumnWriter` flushes its value chunks through `append_sparse_column`, the streaming root writer appends each root batch as a column, and all-NULL ARRAY chunks keep taking the array path. Preserve CHAR padding, key encoding, and on-disk page layouts. Update writer, index, and storage-layout tests and adapt existing callers to the new APIs. None - Test: Unit Test - ASAN BE build; BE UT: StorageLayoutTest, PageBuilderColumnTest, ColumnWriterStorageTest, PageFormatTest, ArrayIndexInputTest, IndexBuilderTest, AnnIndexWriterTest, InvertedIndexWriterTest, InvertedIndexArrayTest, the variant writer/reader suites, and every suite of the test files that include the segment and column writers, the segment flusher format cases and the key coders - clang-format 16, build hygiene, and clang-tidy on the changed sources - Behavior changed: No; internal write-path refactoring - Does this need documentation: No Co-Authored-By: Claude Fable 5.1 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ockDataConvertor (#69070) Related PR: #68608, #68609, #68664 This PR removes OlapBlockDataConvertor and changes the write path to consume column ranges directly through `(const IColumn&, row_pos, num_rows)`. - Extend StorageLayout to provide column-to-storage conversion and object serialization. - Update column writers to consume IColumn directly. - Encode keys directly from block columns and migrate partial-update, variant and BUILD INDEX paths. - Existing CHAR padding, key encoding and on-disk page formats are preserved.
What problem does this PR solve?
Issue Number: None
Related PR: #68608
Problem Summary:
The first piece split out of #68608, which writes segments straight from
IColumn and removes OlapBlockDataConvertor. It changes no behavior: it pins the
page bytes the later pieces must keep, and removes code they would otherwise
carry along. Each commit message has the details.
get_zone_map_value()incolumn_predicate.h, the never-used
ColumnWriter::_null_bitmap, and theunreachable
append_nulls()of the struct and map column writers (they nowreturn NotSupported, as the variant writers already do).
PageFormatTest appends deterministic rows block by block to a
ScalarColumnWriter, or to an OffsetColumnWriter for array and map offsets,
the way a segment writer does, with small page budgets, for every
registered (storage type, page encoding) pair: 88 cases, DICT once per
dictionary page format and AGG_STATE once per serialized shape. The pages
PageIO writes are kept as they are on disk, one golden file per encoding
plus layout.txt, which lists every page, so a failure names the page that
changed. Each column is also read back through ColumnReader and must hold
the rows written. FOR is left out: nothing has written FOR pages since
bitmap indexes were removed, FrameOfReferencePageDecoder cannot read the
values back, and a frame that keeps its original values carries bytes
ForEncoder never initializes.
Release note
None