Gabriel39 commented on code in PR #68780:
URL: https://github.com/apache/doris/pull/68780#discussion_r4220473614
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.cpp:
##########
@@ -1142,19 +1140,58 @@ Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::parse_page_header() {
_remaining_def_nums = page_num_values;
_remaining_num_values = page_num_values;
- // no offset will parse all header.
- if (!active_offset_index) {
- _chunk_parsed_values += _remaining_num_values;
- }
+ // Count each parsed data header exactly once, including the indexed
prefix. A safe fallback
+ // uses this physical value count for EOF; nested logical rows are not
interchangeable with it.
+ _chunk_parsed_values += _remaining_num_values;
_first_data_page_parsed = true;
_state = HEADER_PARSED;
return Status::OK();
}
+template <bool IN_COLLECTION, bool OFFSET_INDEX>
+Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::_finish_indexed_nested_page() {
+ if constexpr (IN_COLLECTION && OFFSET_INDEX) {
+ if (_state != INITIALIZED && _page_reader->has_active_offset_index() &&
+ !_page_reader->is_header_v2()) {
+ // A V1 header has no logical row count. Loading only a prefix
does not validate its
+ // indexed span: drain the remaining level runs before trusting
the next indexed row.
+ // No value decoding or per-value scratch is needed for a page we
are leaving.
+ RETURN_IF_ERROR(load_page_data_idempotent());
+ SCOPED_RAW_TIMER(&_chunk_statistics.decode_level_time);
+ const size_t end_row = _page_reader->end_row();
+ while (_remaining_rep_nums > 0) {
+ level_t level = -1;
+ const size_t run = _rep_level_decoder.get_next_run(&level,
_remaining_rep_nums);
+ if (run == 0) {
+ return Status::Corruption(
+ "Parquet repetition level stream ended while
leaving an indexed page");
+ }
+ if (level == 0) {
+ if (_current_row > end_row || run > end_row -
_current_row) {
+ return Status::Corruption(
+ "Parquet nested page exceeds its indexed row
span");
+ }
+ _current_row += run;
+ }
+ _remaining_rep_nums -= run;
+ }
+ if (_current_row != end_row) {
+ return Status::Corruption(
+ "Parquet nested page does not match its indexed row
span");
+ }
+ }
+ }
+ return Status::OK();
+}
+
template <bool IN_COLLECTION, bool OFFSET_INDEX>
Status ColumnChunkReader<IN_COLLECTION, OFFSET_INDEX>::next_page() {
if constexpr (OFFSET_INDEX) {
RETURN_IF_ERROR(ensure_first_data_page_parsed());
+ RETURN_IF_ERROR(_finish_indexed_nested_page());
+ if (_state == INITIALIZED && _page_reader->has_active_offset_index()) {
Review Comment:
Fixed in e7615abf15. Every visited page is now parsed before either using
its row bounds or advancing past it. Flat and nested V2 pages validate the
header row count; indexed nested V1 pages drain/validate their repetition
levels before the next indexed coordinate is used. Ordinary, fixed-width,
dictionary and nested seek paths all reconcile pages before making the
range/skip decision. The previous unverified-skip flag is removed because
skipped prefixes are now verified, rather than rejected only if the index is
later discarded.
The regression failed before the fix and now passes. It covers
understated/overstated skipped spans, valid indexes, and a stale byte size that
switches navigation to a safe sequential path, across all 16 four-page V1/V2
layouts, flat/nested readers, cache states and value/predicate/levels-only
modes (1,152 combinations). No-index controls instantiate the non-indexed
reader specialization. All 742 related BE ASAN tests pass, including the
existing navigation and exhaustive selection matrices; clang-format 16 and diff
checks pass.
The tradeoff is explicit: unselected flat/V2 pages need a header read but
retain payload skipping; nested V1 pages additionally load/decompress their
levels, without materializing skipped values or allocating per-value validation
scratch. Read-count assertions cover this behavior. The PR description has been
updated accordingly, and be/src/format remains unchanged.
--
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]
