Uh oh!
There was an error while loading. Please reload this page.
[Exec](be) Support offset prue column and null column in BE - #61888
Conversation
…backend rows and return error instead of DCHECK Replace a debug-only DCHECK in MaterializationSharedState::merge_multi_response with an explicit runtime check and a clear InternalError when a backend's response block contains fewer rows than expected. In release builds DCHECK may be disabled which could cause undefined behavior (out-of-bounds reads) if a backend returns fewer rows than block_order_results expects. This change: - Adds a bounds check on the per-backend read cursor before copying columns. - Returns an informative InternalError including backend_id, current index and total rows to aid debugging. - Prevents silent memory corruption in production builds and makes failures observable. File: be/src/pipeline/exec/materialization_opertor.cpp Function: MaterializationSharedState::merge_multi_response Rationale: - Mismatch between expected rows and returned rows can happen due to concurrent compaction/DELETE, RPC truncation, snapshot/version differences, or other transient issues. A runtime error allows the caller to handle the failure and preserves process stability. Testing: - Existing unit tests in be/test/pipeline/operator/materialization_shared_state_test.cpp cover merge paths; run BE unit tests to validate behavior.
…g element_at with sub-column meta access ### What problem does this PR solve? Issue Number: N/A Problem Summary: When `experimental_enable_sub_column_meta_access = true`, a query like `SELECT sum(length(c_info_map['c_phone'])) FROM customer_map` returns wrong results. FE generates access path `[c_info_map, *, OFFSET]` (ACCESS_ALL + OFFSET), meaning "look up a specific key, and only read the string length of the value". In `MapFileColumnIterator::set_access_paths`, the ACCESS_ALL branch was forwarding the same sub-path (including the OFFSET qualifier) to both the key and value iterators. As a result, the key column entered `OFFSET_ONLY` read mode and stopped reading actual character data, making it impossible to match the requested key at runtime. The fix separates the sub-path forwarding for ACCESS_ALL: the key iterator receives a plain full-read path (column name only), while the value iterator receives the original sub-path with any qualifiers (e.g. OFFSET) preserved. This matches the semantics already encoded by FE's `DataTypeAccessTree`: keys are accessed in full (accessAll=true), values follow the sub-path. ### Release note None ### Check List (For Author) - Test: BE unit test `ColumnReaderTest.MapAccessAllWithOffsetDoesNotPropagateOffsetToKey` - Unit Test - Behavior changed: Yes — with `experimental_enable_sub_column_meta_access` enabled, `element_at(map, key)` queries now correctly read key data and return accurate results - Does this need documentation: No
This reverts commit 6d01772.
This reverts commit 2aa17bd.
This reverts commit df4c1e2.
…ficient backend rows and return error instead of DCHECK" This reverts commit 0e19b17.
Thearas
commented
Mar 30, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
HappenLee
commented
Mar 30, 2026
run buildall |
There was a problem hiding this comment.
Pull request overview
Adds BE support for meta-only column reads (offset-only for length/size computations and null-map-only for null predicates), plus updates access-path handling to avoid mis-propagating OFFSET into map keys.
Changes:
- Introduces
ReadMode(DEFAULT/OFFSET_ONLY/NULL_MAP_ONLY) and propagatesonly_read_offsetsinto page decoders for string/binary columns. - Implements offset-only decoding paths in binary plain/dict page decoders and adds
insert_offsets_from_lengths(...)to string/nullable columns. - Extends
/api/clear_cache/{type}handling with more cache type options and changes the “all” behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| be/test/storage/segment/column_reader_test.cpp | Adds regression test for [map_col, *, OFFSET] access-path behavior. |
| be/src/storage/segment/options.h | Extends PageDecoderOptions with only_read_offsets. |
| be/src/storage/segment/column_reader.h | Adds read-mode API/constants and introduces StringFileColumnIterator. |
| be/src/storage/segment/column_reader.cpp | Implements read-mode handling in iterators and passes decoder options into ParsedPage::create. |
| be/src/storage/segment/binary_plain_page.h | Adds OFFSET_ONLY fast path that fills offsets from lengths without copying chars. |
| be/src/storage/segment/binary_dict_page.cpp | Adds OFFSET_ONLY fast path that resolves dict codes to lengths. |
| be/src/service/http/action/clear_cache_action.cpp | Changes/extends clear-cache type routing and aliases. |
| be/src/core/column/column_string.h | Adds insert_offsets_from_lengths implementation for string columns. |
| be/src/core/column/column_nullable.h | Adds nullable wrapper for insert_offsets_from_lengths. |
| be/src/core/column/column.h | Adds new virtual IColumn::insert_offsets_from_lengths API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (read_null_map_only()) { | ||
| DLOG(INFO) << "File column iterator column " << _column_name | ||
| << " in NULL_MAP_ONLY mode, reading only null map."; | ||
| DORIS_CHECK(dst->is_nullable()); | ||
| auto& nullable_col = assert_cast<ColumnNullable&>(*dst); | ||
| nullable_col.get_nested_column().insert_many_defaults(*n); | ||
| size_t read_rows = *n; | ||
| auto& null_map_data = nullable_col.get_null_map_data(); | ||
| RETURN_IF_ERROR(read_null_map(&read_rows, null_map_data)); | ||
| *has_null = true; |
There was a problem hiding this comment.
FileColumnIterator::next_batch() calls read_null_map(&read_rows, null_map_data) but there is no definition of read_null_map in this TU or in the class declaration. This will fail to compile (and even if added later, this branch should also update *n to read_rows and keep nested/default sizes consistent with the actual rows read).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| running_offset += lengths[i]; | ||
| offsets_ptr[i] = static_cast<T>(running_offset); | ||
| } | ||
| chars.resize(offsets[old_rows + num - 1]); |
There was a problem hiding this comment.
The comment says the chars buffer is extended with “zero-filled padding”, but PaddedPODArray::resize() does not guarantee zero-initialization. In OFFSET_ONLY mode this can leave uninitialized bytes in chars, which is risky if the column is later materialized/serialized. Consider explicitly zero-filling the newly resized region (or update the comment if zero-fill is not required/guaranteed).
| chars.resize(offsets[old_rows+num-1]); | |
| constsize_told_chars_size=chars.size(); | |
| constsize_tnew_chars_size=offsets[old_rows+num-1]; | |
| check_chars_length(new_chars_size, old_rows+num); | |
| if (new_chars_size>old_chars_size) { | |
| chars.resize(new_chars_size); | |
| std::memset(chars.data() +old_chars_size, 0, new_chars_size-old_chars_size); | |
| } else { | |
| chars.resize(new_chars_size); | |
| } |
| // set_access_paths, it sets only_read_offsets on the ColumnIteratorOptions so | ||
| // that the BinaryPlainPageDecoder skips chars memcpy and only fills offsets. |
There was a problem hiding this comment.
This comment is slightly misleading: StringFileColumnIterator::set_access_paths() sets _read_mode, but only_read_offsets is actually propagated into ColumnIteratorOptions inside StringFileColumnIterator::init(). Consider rewording to reflect the actual mechanism (set mode in set_access_paths, propagate option during init) to avoid confusion for future readers.
| // set_access_paths, it sets only_read_offsets on the ColumnIteratorOptions so | |
| // that the BinaryPlainPageDecoder skips chars memcpy and only fills offsets. | |
| // set_access_paths, it records an offset-only read mode, which is then | |
| // propagated as only_read_offsets in ColumnIteratorOptions during init so that | |
| // the BinaryPlainPageDecoder skips chars memcpy and only fills offsets. |
| if (_options.only_read_offsets) { | ||
| // OFFSET_ONLY mode: resolve dict codes to get real string lengths | ||
| // without copying actual char data. This allows length() to work. | ||
| const auto* data_array = reinterpret_cast<const int32_t*>(_bit_shuffle_ptr->get_data(0)); | ||
| size_t start_index = _bit_shuffle_ptr->_cur_index; | ||
| // Reuse _buffer (int32_t vector) to store uint32_t lengths. | ||
| // int32_t and uint32_t have the same size/alignment, and string | ||
| // lengths are always non-negative, so the bit patterns are identical. | ||
| _buffer.resize(max_fetch); | ||
| for (size_t i = 0; i < max_fetch; ++i) { | ||
| int32_t codeword = data_array[start_index + i]; | ||
| _buffer[i] = static_cast<int32_t>(_dict_word_info[codeword].size); | ||
| } | ||
| dst->insert_offsets_from_lengths(reinterpret_cast<const uint32_t*>(_buffer.data()), | ||
| max_fetch); |
There was a problem hiding this comment.
In OFFSET_ONLY mode, _buffer is a std::vector<int32_t> but is used to store string byte lengths and then passed as reinterpret_cast<const uint32_t*>(_buffer.data()). This relies on strict-aliasing-unsafe type punning and also on implementation-defined behavior when casting sizes > INT32_MAX to int32_t. Prefer using a std::vector<uint32_t> (or another properly typed buffer) for lengths to avoid UB and portability issues.
HappenLee
commented
Mar 30, 2026
run buildall |
doris-robot
commented
Mar 30, 2026
TPC-H: Total hot run time: 26729 ms |
doris-robot
commented
Mar 30, 2026
TPC-DS: Total hot run time: 169329 ms |
hello-stephen
commented
Mar 30, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Mar 31, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
HappenLee
commented
Mar 31, 2026
/review |
There was a problem hiding this comment.
Code Review Summary
This PR adds offset-only and null-map-only read optimizations for string, array, map, and struct column iterators. The overall design is sound — propagating meta-only read modes through the column iterator hierarchy to skip unnecessary I/O. However, there are critical data correctness bugs in the FileColumnIterator NULL_MAP_ONLY paths.
Critical Checkpoints
Goal & Correctness: The PR introduces two meta-read modes (OFFSET_ONLY, NULL_MAP_ONLY) for complex and string types. The OFFSET_ONLY path for string/array/map types looks correct. The NULL_MAP_ONLY paths for Map/Array/Struct iterators correctly delegate to _null_iterator->next_batch(). However, the FileColumnIterator::next_batch NULL_MAP_ONLY path has a critical data correctness bug — it does not actually read the null bitmap from the page and fills the null map with uninitialized memory.
Concurrency: No new concurrency concerns. The changes are within single-threaded iterator paths.
Error Handling: Status returns are properly checked. DORIS_CHECK is used appropriately for invariants (e.g., ensuring dst is nullable when NULL_MAP_ONLY mode is active).
Memory Safety: The PODArray::resize() used in the buggy path does NOT zero-initialize memory (by design — see pod_array.h:68: "It differs from std::vector in that it does not initialize the elements"). This leads to uninitialized memory being interpreted as null flags.
Incompatible Changes:FileColumnIterator is changed from final to non-final to support StringFileColumnIterator inheritance. No ABI/serialization compatibility issues.
Parallel Code Paths:next_batch and read_by_rowids are the two parallel read paths. Both have been updated, but next_batch has the critical bug while read_by_rowids at least attempts to read the null bitmap (though with its own issues).
Test Coverage: One new test MapAccessAllWithOffsetDoesNotPropagateOffsetToKey verifies ACCESS_ALL path splitting logic. However, there are no tests for the actual NULL_MAP_ONLY data reading behavior, no tests for string OFFSET_ONLY reading through the full pipeline, and no negative tests. The test coverage for a feature of this complexity is insufficient.
Configuration: No new config items.
Observability: DLOG(INFO) messages added for mode transitions — appropriate.
Issues Found
See inline comments for details. Summary:
- [Critical]
FileColumnIterator::next_batchNULL_MAP_ONLY:null_map_data.resize()uses PODArray which does NOT zero-fill — null map contains uninitialized memory, causing silently wrong query results. - [Critical] Same
next_batchNULL_MAP_ONLY path never reads the actual null bitmap from pages — even if resize were fixed, it would report all rows as not-null (or all-null), not the real null status. - [Medium]
FileColumnIterator::read_by_rowidsNULL_MAP_ONLY: Sameresize()bug — pre-allocates with uninitialized memory before the loop fills it. If the loop terminates early (partial page reads), some entries remain garbage.
| nullable_col.get_nested_column().insert_many_defaults(*n); | ||
| auto& null_map_data = nullable_col.get_null_map_data(); | ||
| null_map_data.resize(null_map_data.size() + *n); | ||
| *has_null = true; |
There was a problem hiding this comment.
[Critical Bug — Data Correctness]PODArray::resize() does NOT zero-initialize new elements (see pod_array.h:68: "It differs from std::vector in that it does not initialize the elements"). The new null map entries will contain uninitialized memory (garbage), causing random rows to be incorrectly classified as null or non-null.
But fixing this alone is insufficient — this entire NULL_MAP_ONLY block never reads the actual null bitmap from the segment pages. The normal path (lines 2119-2162) iterates through pages, decodes the null bitmap via _page.null_decoder.GetNextRun(), and processes each run. This path skips all of that.
For scalar FileColumnIterator, the null bitmap is embedded in the data pages (unlike complex types which have a separate _null_iterator). To correctly implement NULL_MAP_ONLY here, you need to:
- Iterate through pages (like the normal path does)
- Read
_page.null_decoderto get actual null/non-null status - Write the correct values into
null_map_data - Skip
_page.data_decoder->next_batch()calls (since we only want nulls)
Alternatively, if this mode is only used for complex type sub-iterators that have their own _null_iterator, consider whether FileColumnIterator actually needs NULL_MAP_ONLY support at all — the Map/Array/Struct iterators already handle it via their own null iterators.
| auto& null_map_data = nullable_col.get_null_map_data(); | ||
| const size_t base_size = null_map_data.size(); | ||
| null_map_data.resize(base_size + count); | ||
There was a problem hiding this comment.
[Medium Bug — Uninitialized Memory] Same PODArray::resize() issue as in next_batch: null_map_data.resize(base_size + count) allocates count new entries with uninitialized memory.
Unlike next_batch, this read_by_rowids path does subsequently fill the null map via memset in the loop below (lines 2215-2228). However, if the while loop terminates early (e.g., all remaining rows are processed but some entries weren't reached), entries could remain uninitialized.
Suggestion: Use resize_fill(base_size + count) or memset the entire range to 0 after resize, then let the loop overwrite with actual values.
PR approved by at least one committer and no changes requested. |
doris-robot
commented
Mar 31, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Mar 31, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
HappenLee
commented
Apr 2, 2026
/review |
There was a problem hiding this comment.
I found 2 correctness issues in the new meta-only read paths.
ColumnStr::insert_offsets_from_lengths()readsoffsets[old_rows - 1]even when the destination string column is empty. In this codebase the-1sentinel is only valid when you actually index with-1; hereold_rowsissize_t, so the first append goes through a huge unsigned index instead. Any first-page / first-batch OFFSET_ONLY read into a freshColumnStringcan therefore build offsets from garbage and corrupt the column state.- The new
NULL_MAP_ONLYbranches inStructFileColumnIterator::next_batch()andArrayFileColumnIterator::next_batch()unconditionally write*has_null = true. But both iterators still implementread_by_rowids()by coalescing rowid runs and callingnext_batch(&num_read, dst, nullptr). Once a struct/array column entersNULL_MAP_ONLYmode andread_by_rowids()is used, that becomes a null-pointer dereference.
Critical checkpoints:
- Goal / correctness: The PR aims to add offset-only/null-only reads for string/array/map/struct paths. The current implementation does not fully achieve that safely because the two bugs above can corrupt string columns or crash rowid-based reads.
- Small / focused change: Mostly focused on column readers and tests, but the new read modes need a bit more hardening before merge.
- Concurrency: I did not find new lock-order or thread-safety issues in the touched code paths.
- Lifecycle / initialization: No cross-TU/static lifecycle issues found.
- Configuration: No new config added.
- Compatibility: No storage-format or protocol compatibility change identified.
- Parallel paths: The review checked scalar string, array, map, and struct iterator paths; the null-only rowid regression affects array/struct specifically because they reuse
next_batch(..., nullptr). - Conditional checks: The new mode checks are understandable, but the
NULL_MAP_ONLYbranches need to respect existing caller contracts forhas_null. - Test coverage: The added map access-path unit test is useful, but there is no coverage for first-batch string OFFSET_ONLY reads or
read_by_rowids()underNULL_MAP_ONLYfor array/struct. - Observability: Existing logs are sufficient for these reader-path changes.
- Transaction / persistence: Not applicable.
- Data writes / atomicity: Not applicable.
- FE/BE variable passing: Not applicable.
- Performance: The intended optimization looks reasonable; I did not identify a separate performance regression beyond the correctness issues above.
- Other issues: None beyond the 2 findings above.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| return Status::OK(); | ||
| } | ||
There was a problem hiding this comment.
This new branch unconditionally writes *has_null, but StructFileColumnIterator::read_by_rowids() and ArrayFileColumnIterator::read_by_rowids() still implement their rowid path by coalescing runs and calling next_batch(&num_read, dst, nullptr).
So after set_access_paths() switches a struct/array column into NULL_MAP_ONLY, any read_by_rowids() on that iterator will hit this branch and dereference a null pointer. The same issue exists in the analogous array branch below.
Please either guard the store with if (has_null != nullptr) or keep the rowid path from calling next_batch(..., nullptr) when these meta-only modes are active.
HappenLee
commented
Apr 2, 2026
/review |
HappenLee
commented
Apr 2, 2026
/review |
1 similar comment
zclllyybb
commented
Apr 2, 2026
/review |
There was a problem hiding this comment.
Findings:
be/src/storage/segment/column_reader.cpp:2219and:2270break the newNULL_MAP_ONLYrowid path.FileColumnIterator::read_by_rowids()inserts defaults into the nullable nested column before the scan loop and then inserts them again after shrinking the null map. On a successful read this leaves the nested column with roughly2 * countrows while the null map is resized back tocount, which violatesColumnNullable's size invariant and can corrupt any rowid-basedIS NULL/IS NOT NULLexecution path once FE starts emittingNULLmeta access.be/src/core/column/column_string.haddsinsert_offsets_from_lengths()without the samecheck_chars_length()guard used by the other string insertion helpers. InOFFSET_ONLYmode we still accumulate the full logical byte length intochars/offsets; once that sum exceeds theUInt32limit, offsets wrap and the synthesized string column becomes corrupt.be/test/storage/segment/column_reader_test.cppadds a unit test that readsMapFileColumnIterator::_key_iterator/_val_iteratorandColumnIterator::_reading_flagdirectly. Those members are private/protected, so this UT does not compile as written.
Critical checkpoint conclusions:
- Goal of the task: The PR aims to add offset-only/meta-only reads for complex and string columns. The implementation is incomplete because the new null-only rowid path is incorrect, and the added BE UT does not validate a green build.
- Scope/minimality: Not minimal. The branch mixes multiple unrelated BE/FE changes, which makes review and regression isolation harder.
- Concurrency: I did not find a new locking/thread-safety issue in the touched iterator paths.
- Lifecycle/static init: No special lifecycle or static-initialization problem found in the reviewed code.
- Configuration:
enable_sub_column_meta_accessis added as a forwarded session variable; that part looks consistent, but the new behavior it gates is not fully validated. - Compatibility: No storage-format or symbol-compatibility issue stood out in the reviewed changes.
- Parallel code paths: The new optimization touches sequential and rowid-based readers, but the null-only rowid path is inconsistent with the sequential path.
- Special conditional checks: The new meta-only branches are understandable, but they need stronger correctness coverage around rowid reads and mixed access paths.
- Test coverage: Insufficient. The new UT itself does not compile, and there is no coverage for
NULL_MAP_ONLYrowid reads. - Observability: Existing logs are adequate for these iterator changes; no extra metric is obviously required.
- Transaction/persistence: Not applicable.
- Data writes/modifications: Not applicable.
- FE-BE variable/path propagation: The new access-path token is wired on both sides, but the end-to-end behavior still has the correctness gaps above.
- Performance: The intended optimization is good, but the missing overflow guard can turn a scan into a corruption path on large string workloads.
- Other issues: No additional must-fix issue beyond the findings above.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
HappenLee
commented
Apr 2, 2026
run buildall |
HappenLee
commented
Apr 2, 2026
run buildall |
doris-robot
commented
Apr 2, 2026
TPC-H: Total hot run time: 29305 ms |
doris-robot
commented
Apr 2, 2026
TPC-DS: Total hot run time: 178979 ms |
doris-robot
commented
Apr 2, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
PR approved by at least one committer and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
### What problem does this PR solve? Problem Summary: This PR includes two main changes: Add offset-only read optimization support for string, array, and map types in column reader ### Release note - [Storage] Add offset-only read optimization for complex types (string, array, map) to improve read performance ### Check List - Test: BE unit tests passed - Behavior changed: No (materialization fix prevents silent failures, now returns error explicitly) - Does this need documentation: No --------- Co-authored-by: englefly <englefly@gmail.com> (cherry picked from commit 632f791)
## Summary - Pick #63417 to branch-4.1. - Pick #63969 to branch-4.1. - Do not include #62854 in this PR because branch-4.1 does not have the offset-only prerequisite infrastructure (`ACCESS_STRING_OFFSET`, `only_read_offsets`; prerequisites such as #61888/#62205 are not on branch-4.0/4.1). Direct conflict resolution would effectively backport a larger optimization stack. ## Testing - `build-support/check-format.sh` - `./run-be-ut.sh --run --filter=RuntimePredicateTest.*` - `./run-fe-ut.sh --run org.apache.doris.qe.runtime.ThriftPlansBuilderTest,org.apache.doris.qe.OldCoordinatorTest` --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
What problem does this PR solve?
Problem Summary: This PR includes two main changes:
Add offset-only read optimization support for string, array, and map types in column reader
Release note
Check List