github-actions[bot] commented on code in PR #66036:
URL: https://github.com/apache/doris/pull/66036#discussion_r3649461094
##########
be/src/format_v2/parquet/parquet_scan.cpp:
##########
@@ -1659,12 +1659,23 @@ Status
ParquetScanScheduler::read_filter_columns(int64_t batch_rows,
const uint16_t selected_rows_before = *selected_rows;
IColumn::Filter compact_filter;
bool used_filter = false;
+ const bool predicate_only = request.is_predicate_only(local_id);
Review Comment:
[P1] Keep hidden values that later filters still consume
`predicate_only` only means the slot is not part of final output; it can
still be referenced by a remaining residual or delete conjunct. This branch
ignores `residual_predicate_positions` (unlike the fixed-width direct path) and
passes `nullptr`, then installs defaults before later stages run. For example,
with hidden dictionary column `id`, `id > 2` plus residual `id + score = 33`
filters IDs first and then evaluates the residual with `id=0`, dropping the
valid `(3,30)` row. Please retain/project the payload whenever any
residual/delete expression references this position, and add the corresponding
hidden-column test.
##########
be/benchmark/parquet/parquet_benchmark_scenarios.h:
##########
@@ -193,6 +193,17 @@ inline std::vector<ReaderScenario> reader_scenarios() {
}
}
}
+ for (const int selectivity : {1, 10, 50, 90}) {
Review Comment:
[P2] Update the benchmark's required registration contract
These additions move the deduplicated reader matrix to 159 cases (as the
changed tests assert), but `be/benchmark/parquet/AGENTS.md` still tells
benchmark users and reviewers to expect 152 at lines 49, 129, and 302.
Following that mandatory smoke-validation guide will now flag the correct
binary as inconsistent. Please update all three counts and the matrix
description with this change.
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.cpp:
##########
@@ -1712,6 +1712,135 @@ Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::filter_fixed_width_values
return Status::OK();
}
+template <bool IN_COLLECTION, bool OFFSET_INDEX>
+Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::filter_dictionary_indices(
+ const IColumn::Filter& dictionary_filter, ColumnSelectVector&
select_vector,
+ const IColumn* typed_dictionary, IColumn* projected_values,
+ ColumnInt32* matched_dictionary_ids, IColumn::Filter* row_filter,
bool* projected_directly,
+ bool* used_filter) {
+ DORIS_CHECK(row_filter != nullptr);
+ DORIS_CHECK(projected_directly != nullptr);
+ DORIS_CHECK(used_filter != nullptr);
+ DORIS_CHECK((typed_dictionary == nullptr) == (projected_values ==
nullptr));
+ *projected_directly = false;
+ *used_filter = false;
+ row_filter->clear();
+ if (_current_encoding != tparquet::Encoding::RLE_DICTIONARY ||
_page_decoder == nullptr ||
Review Comment:
[P1] Handle all-NULL dictionary pages as a successful fragment
A nullable data page can legally contain only definition levels even when
the Column Chunk otherwise uses `RLE_DICTIONARY`; `load_page_data()`
deliberately installs `EmptyValueSectionDecoder` for that shape. It reports no
dictionary, so this new gate returns `used_filter=false` after
`_read_dictionary_filter_values()` has already consumed the definition levels,
and the caller immediately hits `DORIS_CHECK(used_filter)`. A clustered-null
page in a chunk with non-NULL dictionary values can therefore terminate the BE
on valid input. Please treat the zero-physical-value page as a successful
all-false fragment that advances logical progress without dictionary IDs, while
preserving (or making neutral) the requested projection mode across page
fragments; otherwise projected INT batches can next fail the mode-consistency
check. Add predicate-only and projected nullable-INT multi-page coverage with
the all-NULL page on both sides of a non-NULL dictionary page.
##########
be/src/format_v2/parquet/reader/native_column_reader.cpp:
##########
@@ -530,6 +583,55 @@ Status
NativeColumnReader::select_with_dictionary_filter(const SelectionVector&
const uint8_t* filter_data = nullptr;
RETURN_IF_ERROR(selection.materialize_filter(selected_rows, batch_rows,
&filter_data));
+ ColumnInt32* direct_matched_ids = nullptr;
+ const IColumn* typed_dictionary = nullptr;
+ IColumn* projected_values = projected_column;
+ ColumnNullable* projected_nullable = nullptr;
+ if (projected_column != nullptr) {
+ if (!_matched_dictionary_ids) {
+ _matched_dictionary_ids = ColumnInt32::create();
+ }
+ _matched_dictionary_ids->clear();
+ direct_matched_ids =
check_and_get_column<ColumnInt32>(*_matched_dictionary_ids);
+ DORIS_CHECK(direct_matched_ids != nullptr);
+ RETURN_IF_ERROR(_native_reader->prepare_typed_dictionary(_type,
&typed_dictionary));
+ projected_nullable =
check_and_get_column<ColumnNullable>(*projected_column);
+ if (projected_nullable != nullptr) {
+ projected_values = &projected_nullable->get_nested_column();
+ }
+ }
+ int64_t direct_rows_read = 0;
+ bool projected_directly = false;
+ bool direct_filter_used = false;
+ RETURN_IF_ERROR(read_with_dictionary_filter(
+ batch_rows, filter_data, selected_rows == 0, dictionary_filter,
typed_dictionary,
+ projected_values, direct_matched_ids, row_filter,
&direct_rows_read,
+ &projected_directly, &direct_filter_used));
+ if (direct_filter_used) {
+ advance_selected_span(direct_rows_read);
+ const size_t survivor_count =
Review Comment:
[P2] Reuse the survivor count from bitmap construction
`filter_dictionary_indices()` has just visited every selected row to build
`row_filter`, but this recount scans the full bitmap again, and
`read_filter_columns()` scans it once more with `count_selected_rows()`. Those
extra O(selected_rows) memory passes run for every direct dictionary batch,
including the predicate-only workload this PR is optimizing. Please
accumulate/return the survivor count while building the bitmap and reuse it for
reader statistics and scheduler selection updates.
--
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]
