Gabriel39 commented on code in PR #68780:
URL: https://github.com/apache/doris/pull/68780#discussion_r4219749647
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.cpp:
##########
@@ -1096,15 +1096,14 @@ Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::parse_page_header() {
}
int32_t page_num_values = _page_reader->is_header_v2() ?
header->data_page_header_v2.num_values
:
header->data_page_header.num_values;
- if constexpr (IN_COLLECTION && OFFSET_INDEX) {
- if (!_page_reader->is_header_v2() &&
_page_reader->has_active_offset_index()) {
- // V1 nested pages do not declare their logical row count. An
OffsetIndex span cannot
- // be trusted until repetition levels are decoded, so keep the
sequential cursor path.
- _page_reader->discard_offset_index();
- _offset_index = nullptr;
- }
- }
+ // A later V1 page can still use a valid OffsetIndex in a mixed-version
chunk. Its first
Review Comment:
Fixed in 65f2abf240. Before advancing from a parsed indexed nested V1 data
page, the chunk reader drains the remaining repetition-level runs and verifies
the complete logical row span. This also covers header-only and partially
consumed pages. Skipped values are not materialized, and the validation
allocates no per-value scratch. Wholly unparsed indexed pages retain lazy
skipping; a later fallback still rejects their unverified prefix.
The reported disjoint selection returned success before the fix and now
returns corruption, with cache on/off and both value and levels-only reads.
Additional tests cover valid partial-page advances and assert that unparsed
page skipping remains active. All 741 related ASAN tests passed, including
4,032 navigation-matrix cases and 30,480 exhaustive row-selection cases.
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.cpp:
##########
@@ -1096,15 +1096,14 @@ Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::parse_page_header() {
}
int32_t page_num_values = _page_reader->is_header_v2() ?
header->data_page_header_v2.num_values
:
header->data_page_header.num_values;
- if constexpr (IN_COLLECTION && OFFSET_INDEX) {
- if (!_page_reader->is_header_v2() &&
_page_reader->has_active_offset_index()) {
- // V1 nested pages do not declare their logical row count. An
OffsetIndex span cannot
- // be trusted until repetition levels are decoded, so keep the
sequential cursor path.
- _page_reader->discard_offset_index();
- _offset_index = nullptr;
- }
- }
+ // A later V1 page can still use a valid OffsetIndex in a mixed-version
chunk. Its first
+ // repetition level is checked when loaded, and its row count when the
levels are exhausted.
const bool active_offset_index = _page_reader->has_active_offset_index();
+ if (_skipped_unverified_indexed_page && !active_offset_index) {
Review Comment:
Fixed in 65f2abf240. Every parsed data header now contributes its physical
num_values exactly once in both indexed and sequential modes. Thus safe
fallback retains the indexed prefix in EOF accounting, without substituting
logical row counts for repeated physical values. Repeated header calls are
idempotent, and an unparsed skipped prefix still prevents fallback.
The final nested V1 fallback reproduced the out-of-bounds read before the
fix and now returns the exact expected values, including repeated values and
cache hits/misses. The systematic review also found and fixed sequential
V1-to-V2 cursor synchronization and candidate-page reevaluation after parsing
discards the index. All 741 related ASAN tests passed; clang-format 16 and diff
checks passed. All changes remain within format_v2 and its tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
