[Exec](be) Support offset prue column and null column in BE - #61888
Merged
Merged
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.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
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.
Comment on lines
+2097
to
+2106
| 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).
| 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).
Suggested change
| chars.resize(offsets[old_rows + num - 1]); | |
| const size_t old_chars_size = chars.size(); | |
| const size_t new_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); | |
| } |
Comment on lines
+534
to
+535
| // 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.
Suggested change
| // 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. |
Comment on lines
+322
to
+336
| 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.
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 26729 ms |
TPC-DS: Total hot run time: 169329 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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; |
Contributor
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); | ||
|
|
Contributor
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.
BiteTheDDDDt
previously approved these changes
Mar 31, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
Author
|
/review |
1 similar comment
Contributor
|
/review |
Contributor
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.
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 29305 ms |
TPC-DS: Total hot run time: 178979 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BiteTheDDDDt
approved these changes
Apr 3, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
zclllyybb
approved these changes
Apr 3, 2026
HappenLee
added a commit
that referenced
this pull request
Apr 23, 2026
### 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)
This was referenced Jun 4, 2026
yiguolei
pushed a commit
that referenced
this pull request
Jun 8, 2026
## 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>
16 tasks
englefly
added a commit
that referenced
this pull request
Sep 9, 2026
…ed fields (#65805) ### What problem does this PR solve? Issue Number: None Related PR: #65591, #66380 Problem Summary: Nested-column pruning (feature series #59263 / #61888 / #64535) makes Doris read only the referenced parts of a struct/map/array column. Two kinds of special *meta* access paths serve that purpose: a terminal `NULL` component means the query only needs the null flag (`IS [NOT] NULL` on a column or nested field), and a terminal `OFFSET` component means only offsets are needed (`length`/`cardinality`-style functions). The BE satisfies them in `NULL_MAP_ONLY` / `OFFSET_ONLY` meta-read modes that skip the payload data entirely. While the optimization itself is sound, the way meta paths were expressed on the wire and handled on both sides had several correctness holes: * **Wire ambiguity.** Every access path was sent as a `DATA`-typed list of string components, and `NULL`/`OFFSET` lived in the same list as real child names. There was no way to tell a metadata selector apart from a struct field literally named `OFFSET` or `NULL`, so such a field could be swallowed as metadata and its data silently pruned away (wrong query results), and there was no FE/BE version contract to evolve the encoding during rolling upgrades. * **FE null-map correctness.** A slot can be nullable for two different reasons: its physical column has a null bitmap, or an outer join merely made the output slot nullable. The old logic keyed on expression/slot nullability, so a physical `NOT NULL` dimension column of a `LEFT JOIN` received a `[col, NULL]` path and the BE aborted trying to read a null map from a `NOT NULL` column. Meta paths were also generated for non-nullable struct fields / non-null structs and for variant sub-columns, which do not support meta-only reads. * **FE pruning bookkeeping.** `NULL`/`OFFSET` were detected by string suffix, so real fields with those names collided inside the pruning logic too; and predicate meta paths were stripped or broadened (map-star) whenever regular data paths existed, losing metadata-only reads or over-reading payload data. * **BE routing/read-mode logic.** A terminal `NULL`/`OFFSET` was treated as current-level metadata at every container, meta-only modes could be entered even when a sibling `DATA` path still required payload reads, sibling `META` paths were dropped when unrelated data projections existed, and Map descendant routing depended on physical key/value child column names while silently ignoring unknown selectors. What this PR does: * **Versioned, type-selected access paths.** Adds an optional `version` to `TColumnAccessPath` (thrift + protobuf + FE/BE descriptor conversion; `TCOLUMN_ACCESS_PATH_VERSION_LEGACY = 0`, `TCOLUMN_ACCESS_PATH_VERSION_TYPED = 1`). In the typed format the path type is authoritative: `DATA` selects `data_access_path`, `META` selects `meta_access_path`, and `NULL`/`OFFSET` are only ever emitted as typed `META` paths. New BEs still decode the legacy all-`DATA` encoding from old FEs, so the supported rolling upgrade is: upgrade all BEs first, then let FEs send typed paths. * **FE** (`AccessPathExpressionCollector`, `NestedColumnPruning`, `AccessPathPlanCollector`, `DescriptorToThriftConverter`): marks `NULL`/`OFFSET` collector contexts as `META`; checks the physical column's nullability (`getOriginalColumn().isAllowNull`) instead of slot nullability before emitting null-map paths, so outer-join-nullable `NOT NULL` columns and non-nullable fields/structs no longer get `NULL` paths; emits the parent-struct `NULL` path for `element_at(...) IS NULL` only when actually needed; falls back to plain data reads for variant sub-columns; keeps the exact meta path when a sibling data path is also read; preserves fields referenced by unchanged predicates; and keys pruning on the path type instead of string suffixes, rejecting unsupported `META` paths. * **BE** (`ColumnIterator` / `column_reader.cpp`): validates path versions and type-selected payloads; partitions current data / current metadata / descendant routing with explicit per-container ownership (Struct owns the null map; Map/Array and string-like scalar columns own both null map and offsets); derives `NULL_MAP_ONLY` / `OFFSET_ONLY` only when no current or predicate data path requires payload, and marks data children `SKIP` consistently; keeps sibling `META` paths explicit so an unrelated `DATA` projection cannot disable a metadata-only predicate read; routes Map children by the logical `KEYS`/`VALUES` selectors (wildcard expansion, arbitrary nesting) independent of physical child column names; rejects unrecognized Map selectors instead of silently pruning everything; and documents the full routing flow. * **Tests & docs.** BE `ColumnReaderTest` coverage for typed-vs-legacy encoding, metadata-vs-same-named-data disambiguation, meta-read-mode legality, sibling-data-path handling and Map selector validation; `SlotDescriptor` protobuf round-trip of versions and payloads; FE `PruneNestedColumnTest` / `DescriptorToThriftConverterTest` cases; the `nereids_rules_p0/column_pruning` regression suites updated to assert the typed `META` paths, plus the new `left_join_not_null_column` suite guarding the outer-join null-map crash. ### Release note None ### Check List (For Author) - Test: - FE unit test: `./run-fe-ut.sh --run 'org.apache.doris.nereids.rules.rewrite.PruneNestedColumnTest,org.apache.doris.analysis.DescriptorToThriftConverterTest'` (74 passed on the final rebased FE tree) - BE unit test: `./run-be-ut.sh --run --filter='ColumnReaderTest.*:SlotDescriptorTest.AccessPathsPreservedThroughProtobuf'` (50 passed before the final rebase; the final rebuild was blocked before GTest by the missing master-added `thirdparty/installed/lib64/liblance_c.a` in this environment) - Regression test: `nereids_rules_p0/column_pruning` — `string_length_column_pruning`, `null_column_pruning`, `nested_container_offset_pruning`, `lambda_null_pruning`, `left_join_not_null_column` (passed before the final rebase; the final rerun was blocked by the same missing dependency) - Format checks: `build-support/check-format.sh`, `git diff --check` - Behavior changed: Yes. Typed `META` access paths with an explicit version replace the legacy all-`DATA` encoding between new FEs and BEs (upgrade BEs before FEs); struct fields literally named `NULL`/`OFFSET` are now read correctly instead of being pruned as metadata; `IS NULL` on outer-join-nullable `NOT NULL` columns no longer crashes the BE; invalid Map descendant selectors now return an internal error instead of being silently ignored. - Does this need documentation: No --------- Co-authored-by: Hu Shenggang <hushenggang@selectdb.com>
morningman
added a commit
that referenced
this pull request
Sep 16, 2026
…on PRs from master in merge order (#65805 #67813) (#68073) Cherry-picked from #65805, #67813 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830 and #68017. | # | Master commit | PR | Title | |---|---|---|---| | 1 | ec886f3 | #65805 | [fix](nereids) Disambiguate NULL/OFFSET metadata from same-named nested fields | | 2 | 3dbe4d3 | #67813 | [fix](cloud) Invalidate version caches on visible commit retries | Not included on purpose: - The 17 labelled PRs that already carry `incremental-computation-picked` (#62606 in the fork point, #67508 via #67712, the nine of #67830, the six of #68017). - #68012 carries the label but is a PR against this branch itself (merged as `6f7c87fa892`); nothing to pick. - #67820 is still open on master; this branch already carries its content via #67861. ### Prerequisite check - **#65805** lists #65591 and #66380 as related PRs: #65591 is still open and #66380 was closed unmerged, both superseded by #65805 itself. The nested-column-pruning series it builds on (#59263, #61888, #64535) is before the fork point, so it is already on this branch. No master commit between the fork point and #65805 touches `column_reader.{cpp,h}`, `descriptors.cpp`, the nereids pruning rules or the BE tests. The only overlap is the unrelated #66761 (TIMESTAMP_NS), which touches `Descriptors.thrift` (a different struct) and adds an unrelated test plus its `Config` import to `DescriptorToThriftConverterTest.java`; that caused the one conflict, see below. - **#67813** declares no related PR (closes #67099). Its behavior does not depend on any master commit missing here. The one unlabelled master commit that touches the same main files, #66296 ("Reduce cloud version sync config"), only adds `maxAttempts` overloads that none of #67813's main-code hunks use. The other drift in `CloudGlobalTransactionMgr.java` / `CloudGlobalTransactionMgrTest.java` is this branch's own #67861 (the branch-side version of the still-open #67820), which lives in `commitTxn` / `releaseFinishedTso` / `afterAbortTxnResp` and does not intersect the two hunks #67813 adds (`checkTransactionStateBeforeCommit` and the empty-partition-list branch of the commit response handling). Neither #66761 nor #66296 was picked; three mechanical adaptations were needed instead, each recorded in the pick's commit message: - **#65805** conflicted only in the import block of `DescriptorToThriftConverterTest.java` (the master hunk sits next to the `Config` import that #66761 added). Resolved by adding only `import org.apache.doris.thrift.DescriptorsConstants;`. Everything else is byte-identical to the master commit. - **#67813** conflicted only in the import block of `CloudGlobalTransactionMgrTest.java`: the branch already has `import org.apache.doris.rpc.RpcException;` through #67861, so that line became context; the other ten imports were taken as-is. - **#67813**, test-only: the master helper `mockVersionHelper()` stubs `VersionHelper.getVersionFromMeta(request, maxAttempts)`, an overload that only exists on master because of #66296, so `fe-core` test compilation failed (`method getVersionFromMeta ... cannot be applied to given types`). On this branch every read goes through the single-argument overload, so the helper now just returns `Mockito.mockStatic(VersionHelper.class)`. No main-code hunk of #67813 uses the `maxAttempts` overloads. ### Drift check against master For each pick, the diff of the touched files against the master commit's parent before the pick and against the master commit after the pick have identical `+`/`-` lines (only the import context lines differ as described above), i.e. each pick added exactly its master hunks. Leftover differences to master after the picks are: - #65805's files: `DescriptorToThriftConverterTest.java` and `Descriptors.thrift` differ from master by exactly #66761's hunks. - #67813's files: `OlapTable.java`, `CloudPartition.java`, `CloudGlobalTransactionMgr.java` and `CloudGlobalTransactionMgrTest.java`. Applying #66296 forward and #67861 in reverse in a temporary index brings the four main files to zero diff against master `3dbe4d3ca53`; the test file's remaining difference is the `RpcException` import overlap plus the `mockVersionHelper()` adaptation. `CloudFEVersionSynchronizer.java` is byte-identical to master. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main + test) with the test classes touched by the picks plus the two related cloud version-cache classes: 5 classes, 139 tests, 0 failures, 0 errors, BUILD SUCCESS (2:12 min) — `PruneNestedColumnTest` 61, `DescriptorToThriftConverterTest` 21, `CloudGlobalTransactionMgrTest` 37 (all 13 tests #67813 adds included), `CloudPartitionTest` 3, `OlapTableTest` 17. (`VersionHelperTest` from #67813's checklist does not exist on this branch; #66296 added it.) - FE checkstyle on fe-core: 0 violations. - BE: `-fsyntax-only` with the flags of the Release build (`compile_commands.json`, regenerated `gen_cpp` headers) passes for `storage/segment/column_reader.cpp`, `runtime/descriptors.cpp`, and with `-DBE_TEST -fno-access-control` for `test/storage/segment/column_reader_test.cpp` and `test/runtime/descriptor_test.cpp`. - The three touched groovy suites (`lambda_null_pruning`, `left_join_not_null_column`, `null_column_pruning`) parse cleanly (groovy parser check). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: minghong <zhouminghong@selectdb.com> Co-authored-by: Luwei <814383175@qq.com>
englefly
added a commit
to englefly/incubator-doris
that referenced
this pull request
Sep 20, 2026
apache#61888) ### 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>
yiguolei
pushed a commit
that referenced
this pull request
Sep 21, 2026
… path series (#68214) Cherry-pick of #62205, #62315, #62631, #62304, #63229, #63736, #64486 to branch-4.2 (the meta path series, applied in merge order). Adaptations for branch-4.2: * `AccessPathInfo` already lives in `org.apache.doris.analysis` on this branch; the new `ACCESS_STRING_OFFSET` constant was ported there (aliasing the existing `ACCESS_OFFSET`). * master-only types renamed to this branch's thrift types (`ColumnAccessPath* -> TColumnAccessPath`, `ColumnAccessPathType -> TAccessPathType`); `NestedColumnPruning` keeps branch-4.2's `comparePathSegments`/variant branch alongside upstream's new logic. * `LazyMaterializeTopN` merges upstream's restructure while preserving branch-4.2's Lance lazy-materialization gate; `LogicalJoin.getRightConditionSlot()` (a master-only helper) had to be added for the ported `PushDownTopNThroughJoin`. * `StringEmptyToLengthRuleTest` uses `ElementAt` (branch-4.2 folded `StructElement` into `ElementAt`); the regression suite expectation for the same reason. REQUIRED EXTRA COMMIT: the FE now emits offset-only / null-only nested access paths, which branch-4.2's BE could not read — `null_column_pruning.groovy` aborted the BE with `std::vector<...ColumnIterator>::operator[]: Assertion '__n < this->size()' failed`. This PR therefore also cherry-picks the BE-side prerequisite #61888 "[Exec](be) Support offset prue column and null column in BE" (adapted to branch-4.2's `IColumn::mutate`/Defer COW shape and `ColumnNullable::get_null_map_column_ptr()` signature). Please review that extra commit carefully. Testing on branch-4.2 (FE + BE rebuilt from this branch): * FE unit tests: PruneNestedColumnTest (57), PullUpProjectExprUnderTopNTest (28), OperativeColumnDeriveTest (4), StringEmptyToLengthRuleTest (11), MaterializeProbeVisitorTest (8) — 108 tests, 0 failures. * Regression: nereids_rules_p0/column_pruning (7 suites incl. the new string_length/null/nested_container_offset/topn_expr_pullup/topn_lazy_nested suites) 0 failures; shape_check clickbench/tpcds_sf100/tpcds_sf1000/tpcds_sf10t_orc/tpcds_sf1000_constraints suites touched by the series regenerated with -forceGenOut and green; nereids_rules_p0 push_filter_through / null_un_safe_equals / lazy_materialize_topn green. * Known deviations: #62446 (struct field slot type for OFFSET-only access) is not part of the requested series and was not included; the BE-side `MapAccessAllWithOffsetDoesNotPropagateOffsetToKey` unit test from #61888 was kept byte-identical to upstream although it may not hold (test binary not built with MAKE_TEST=ON). --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: HappenLee <happenlee@selectdb.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
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