github-actions[bot] commented on code in PR #66036:
URL: https://github.com/apache/doris/pull/66036#discussion_r3649660102
##########
be/src/format_v2/parquet/parquet_scan.cpp:
##########
@@ -1392,11 +1394,172 @@ uint16_t count_selected_rows(const IColumn::Filter&
filter) {
return selected_rows;
}
-IColumn::Filter build_dictionary_entry_filter(size_t block_position,
- const ParquetColumnSchema&
column_schema,
- const VExprContextSPtrs&
conjuncts,
- const IColumn& dictionary) {
- IColumn::Filter dictionary_filter(dictionary.size(), 1);
+enum class DictionaryEntryFilterKernel {
+ GENERIC,
+ TYPED_FIXED_WIDTH,
+ TYPED_STRING,
+};
+
+template <typename ColumnType>
+bool get_fixed_dictionary_raw_values(const IColumn& dictionary, const
uint8_t** values,
+ size_t* value_width) {
+ const auto* typed_dictionary =
check_and_get_column<ColumnType>(dictionary);
+ if (typed_dictionary == nullptr) {
+ return false;
+ }
+ *values = reinterpret_cast<const
uint8_t*>(typed_dictionary->get_data().data());
+ *value_width = sizeof(typename ColumnType::value_type);
+ return true;
+}
+
+bool get_numeric_dictionary_raw_values(PrimitiveType primitive_type, const
IColumn& dictionary,
+ const uint8_t** values, size_t*
value_width) {
+ switch (primitive_type) {
+ case TYPE_INT:
+ return get_fixed_dictionary_raw_values<ColumnInt32>(dictionary,
values, value_width);
+ case TYPE_BIGINT:
+ return get_fixed_dictionary_raw_values<ColumnInt64>(dictionary,
values, value_width);
+ case TYPE_FLOAT:
Review Comment:
[P2] Add end-to-end FLOAT/DOUBLE dictionary-filter coverage
This admits FLOAT and DOUBLE to an exact bitmap path whose covered conjunct
may be removed, but the new comparison scans exercise only INT32, BIGINT, and
BYTE_ARRAY. The pre-existing accept-all dictionary scan asserts only row
count/broad counters, and the raw NaN unit test does not cover FLOAT/DOUBLE ID
filtering, nullable placement, or fused projection. Please add differential
FLOAT/DOUBLE comparison scans for projected and predicate-only modes, including
NaN, signed zero, infinities, NULLs, and operand reversal, and assert output
values plus direct-path counters.
##########
be/src/format_v2/parquet/reader/native_column_reader.cpp:
##########
@@ -395,6 +381,73 @@ Status
NativeColumnReader::read_with_fixed_width_filter(int64_t rows, const uint
return Status::OK();
}
+Status NativeColumnReader::read_with_dictionary_filter(
+ int64_t rows, const uint8_t* filter_data, bool filter_all,
+ const IColumn::Filter& dictionary_filter, const IColumn*
typed_dictionary,
+ IColumn* projected_values, ColumnInt32* matched_dictionary_ids,
IColumn::Filter* row_filter,
+ int64_t* rows_read, bool* projected_directly, bool* used_filter) {
+ DORIS_CHECK(rows >= 0);
+ DORIS_CHECK(row_filter != nullptr);
+ DORIS_CHECK(rows_read != nullptr);
+ DORIS_CHECK(projected_directly != nullptr);
+ DORIS_CHECK(used_filter != nullptr);
+ row_filter->clear();
+ *rows_read = 0;
+ *projected_directly = false;
+ *used_filter = false;
+ if (rows == 0) {
+ return Status::OK();
+ }
+
+ native::FilterMap filter;
+ RETURN_IF_ERROR(filter.init(filter_data, static_cast<size_t>(rows),
filter_all));
+ _native_reader->reset_filter_map_index();
+ bool eof = false;
+ int64_t consecutive_empty_calls = 0;
+ while (*rows_read < rows && !eof) {
+ size_t loop_rows = 0;
+ IColumn::Filter loop_filter;
+ bool loop_projected_directly = false;
+ bool loop_used = false;
+ RETURN_IF_ERROR(_native_reader->read_dictionary_filter(
+ dictionary_filter, filter, static_cast<size_t>(rows -
*rows_read), typed_dictionary,
+ projected_values, matched_dictionary_ids, &loop_filter,
&loop_rows, &eof,
+ &loop_projected_directly, &loop_used));
+ if (!loop_used) {
+ if (UNLIKELY(*rows_read != 0)) {
+ return Status::Corruption(
+ "Parquet dictionary predicate encoding changed after
{} rows for column {}",
+ *rows_read, _name);
+ }
+ row_filter->clear();
+ return Status::OK();
+ }
+ if (*rows_read != 0) {
+ DORIS_CHECK_EQ(*projected_directly, loop_projected_directly);
Review Comment:
[P1] Keep empty page transitions projection-mode neutral
A zero-value Data Page V1/V2 is a valid input that the native reader already
skips. Here, after an earlier dictionary page has appended fixed-width
survivors and set `projected_directly=true`, the zero-row page returns
`loop_rows=0`, `used_filter=true`, and the default
`loop_projected_directly=false`, so this check terminates the BE before the
following page is read. This is distinct from the all-NULL-page thread because
the zero-row page consumes no definition/value cursor. Please compare/update
projection mode only for fragments that process logical rows, and add projected
RLE_DICTIONARY/PLAIN_DICTIONARY V1/V2 coverage with a zero-value page between
nonempty pages.
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.cpp:
##########
@@ -1712,6 +1712,201 @@ Status ColumnChunkReader<IN_COLLECTION,
OFFSET_INDEX>::filter_fixed_width_values
return Status::OK();
}
+namespace {
+
+template <typename ColumnType>
+bool try_filter_and_project_dictionary_values(
+ const IColumn* typed_dictionary, IColumn* projected_values,
+ const std::vector<uint32_t>& selected_dictionary_indices,
+ const NullMap& nullable_selection_nulls, const IColumn::Filter&
dictionary_filter,
+ IColumn::Filter* row_filter) {
+ if (typed_dictionary == nullptr || projected_values == nullptr) {
+ return false;
+ }
+ const auto* dictionary =
check_and_get_column<ColumnType>(*typed_dictionary);
+ auto* projected = check_and_get_column<ColumnType>(*projected_values);
+ if (dictionary == nullptr || projected == nullptr) {
+ return false;
+ }
+
+ const auto& dictionary_data = dictionary->get_data();
+ auto& projected_data = projected->get_data();
+ projected_data.reserve(projected_data.size() +
selected_dictionary_indices.size());
+ row_filter->reserve(nullable_selection_nulls.size());
+ size_t physical_row = 0;
+ for (const uint8_t is_null : nullable_selection_nulls) {
+ bool keep = false;
+ if (is_null == 0) {
+ const uint32_t dictionary_id =
selected_dictionary_indices[physical_row++];
+ // The decoder validates the complete id batch first, preserving
atomic output while
+ // allowing this hot gather loop to use unchecked dictionary
lookups.
+ keep = dictionary_filter[dictionary_id] != 0;
+ if (keep) {
+ projected_data.push_back(dictionary_data[dictionary_id]);
+ }
+ }
+ row_filter->push_back(keep ? 1 : 0);
+ }
+ DORIS_CHECK_EQ(physical_row, selected_dictionary_indices.size());
+ return true;
+}
+
+bool try_filter_and_project_fixed_width_dictionary(
+ const IColumn* typed_dictionary, IColumn* projected_values,
+ const std::vector<uint32_t>& selected_dictionary_indices,
+ const NullMap& nullable_selection_nulls, const IColumn::Filter&
dictionary_filter,
+ IColumn::Filter* row_filter) {
+#define TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnType)
\
+ if (try_filter_and_project_dictionary_values<ColumnType>(
\
+ typed_dictionary, projected_values,
selected_dictionary_indices, \
+ nullable_selection_nulls, dictionary_filter, row_filter)) {
\
+ return true;
\
+ }
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnUInt8)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnInt8)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnInt16)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnInt32)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnInt64)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnInt128)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnFloat32)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnFloat64)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnDate)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnDateTime)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnDateV2)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnDateTimeV2)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnTimeV2)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnTimeStampTz)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnIPv4)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnIPv6)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnOffset32)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnOffset64)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnDecimal32)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnDecimal64)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnDecimal128V2)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnDecimal128V3)
+ TRY_FIXED_WIDTH_DICTIONARY_COLUMN(ColumnDecimal256)
+#undef TRY_FIXED_WIDTH_DICTIONARY_COLUMN
+ return false;
+}
+
+} // namespace
+
+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 ||
+ !_page_decoder->has_dictionary()) {
+ return Status::OK();
+ }
+ if (UNLIKELY(_remaining_num_values < select_vector.num_values())) {
+ return Status::IOError("Decode too many values in current page");
+ }
+ if (UNLIKELY(dictionary_filter.size() !=
_page_decoder->dictionary_size())) {
+ return Status::Corruption("Parquet predicate dictionary has {}
entries, expected {}",
+ dictionary_filter.size(),
_page_decoder->dictionary_size());
+ }
+
+ ParquetSelection selection;
Review Comment:
[P2] Reuse sparse selection-range scratch
A staged earlier predicate can leave one disjoint physical run per surviving
row. This local `ParquetSelection` then grows `ranges` to that fragmentation
and destroys the capacity at the end of every page fragment, so later
dictionary predicates repeatedly allocate/free the same range storage. The
mandatory scanner guide requires selection ranges to live in persistent reader
scratch specifically to avoid this hot-path allocation thrash. Please retain
and clear a reusable allocator-aware selection (including its capacity in the
existing active/retained scratch policy), and cover a two-predicate
fragmented-selection benchmark.
##########
be/src/format_v2/parquet/reader/native/column_chunk_reader.h:
##########
@@ -408,6 +420,7 @@ class ColumnChunkReader {
// Plain or Dictionary encoding. If the dictionary grows too big, the
encoding will fall back to the plain encoding
std::unordered_map<int, std::unique_ptr<Decoder>> _decoders;
NullMap _nullable_selection_nulls;
+ std::vector<uint32_t> _selected_dictionary_indices;
Review Comment:
[P2] Track the retained selected-ID scratch
This is a persistent per-leaf buffer, but `std::vector` growth does not pass
through Doris `Allocator::consume_memory`; the checked-in malloc interposer
only forwards allocations to jemalloc. The retained-byte bookkeeping here
drives the scratch-release policy, not the query MemTracker. With the uint16_t
batch bound this can retain about 256 KiB per dictionary predicate leaf, and
the 4 MiB per-buffer release threshold means that normal maximum capacity
survives until reader teardown, multiplying across wide scans. Please use
allocator-aware storage such as `DorisVector` (updating the decoder interface)
or explicitly charge/release capacity to the owning query tracker.
##########
be/benchmark/parquet/parquet_benchmark_scenarios.h:
##########
@@ -326,11 +354,12 @@ inline std::string to_string(ReaderOperation value) {
}
inline std::string reader_scenario_name(const ReaderScenario& scenario) {
- return to_string(scenario.operation) + "/" + to_string(scenario.encoding)
+ "/null_" +
- std::to_string(scenario.null_percent) + "/" +
to_string(scenario.null_pattern) +
- "/sel_" + std::to_string(scenario.selectivity_percent) + "/" +
- to_string(scenario.projection) + "/width_" +
std::to_string(scenario.schema_width) +
- "/predicate_" + std::to_string(scenario.predicate_position);
+ return to_string(scenario.operation) + "/" + to_string(scenario.encoding)
+ "/" +
Review Comment:
[P2] Update the exact benchmark filters for the new name shape
Adding `/<value_type>/` changes every reader registration to
`.../<encoding>/<value_type>/null_...`, but the exact filters in
`be/benchmark/parquet/AGENTS.md:93` and `be/benchmark/parquet/README.md:75,87`
still use `.../<encoding>/null_...` and now select no cases. This is distinct
from the existing stale-count/matrix thread: correcting those descriptions
still leaves the documented comparisons as no-ops. Please update all exact
filters and validate them with `--benchmark_list_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]
