github-actions[bot] commented on code in PR #68780:
URL: https://github.com/apache/doris/pull/68780#discussion_r4217241671
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.cpp:
##########
@@ -1067,11 +1067,11 @@ Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::load_dictionary_page(bool
template <bool IN_COLLECTION, bool OFFSET_INDEX>
Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::ensure_first_data_page_parsed() {
- if (_first_data_page_parsed) {
+ if (_first_data_page_parsed && _page_reader->has_active_offset_index()) {
Review Comment:
[P1] Reconcile page bounds before dictionary predicate reads.
`ScalarColumnReader::read_dictionary_filter` still reads `page_end_row()`
without this guard when compiled with `OFFSET_INDEX=true`. If an otherwise
readable dictionary-encoded chunk has a stale first-page OffsetIndex size, page
one discards the index; on the next call `next_page()` has advanced
`_start_row` but `_end_row` still describes page one. The dictionary reader
sees an empty page range and calls `next_page()` again, which parses and skips
page two. Later predicate bits can then be applied to the wrong rows, or the
scan fails. Call the guard before that reader computes `right_row` too, and
cover a multi-page dictionary predicate with an index fallback.
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.cpp:
##########
@@ -1067,11 +1067,11 @@ Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::load_dictionary_page(bool
template <bool IN_COLLECTION, bool OFFSET_INDEX>
Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::ensure_first_data_page_parsed() {
- if (_first_data_page_parsed) {
+ if (_first_data_page_parsed && _page_reader->has_active_offset_index()) {
return Status::OK();
}
- // OffsetIndex row bounds are untrusted until page zero has been
reconciled and its declared
- // cardinality checked, so no indexed skip may observe them before this
one-time parse.
+ // OffsetIndex bounds need an initial reconciliation. If the index is
discarded, every later
+ // page needs its header parsed before callers use its sequential row
bounds.
Review Comment:
[P1] Validate row coordinates when a later OffsetIndex fallback follows
indexed skips. A structurally accepted index can advertise page starts
`[0,1,3]` for V2 pages that actually contain `[1,1,2]` rows. If the reader
selects only row 3, it skips page two using the unverified `[1,3)` span. When
page three has a stale compressed-size entry, parsing discards the index, but
the new sequential bounds still start at the indexed row 3. Flat or nested
output then returns page three's first value (physical row 2) as row 3 without
a count error. Reject this transition or verify skipped page row counts before
trusting the sequential cursor; test a mismatch after an indexed skip.
--
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]
