github-actions[bot] commented on code in PR #66412:
URL: https://github.com/apache/doris/pull/66412#discussion_r3714400685
##########
be/src/format_v2/parquet/parquet_scan.cpp:
##########
@@ -1834,6 +2293,297 @@ Status
ParquetScanScheduler::prepare_current_dictionary_filters(
_current_predicate_columns.emplace(local_id, std::move(column_reader));
update_counter_if_not_null(_scan_profile.dict_filter_columns, 1);
}
+
+ std::unordered_map<size_t, size_t> remaining_position_occurrences;
+ for (const auto& remaining_stage : schedule.remaining_stages) {
+ for (const size_t position : remaining_stage.required_positions) {
+ ++remaining_position_occurrences[position];
+ }
+ }
+ std::unordered_set<size_t> delete_positions;
+ for (const auto& conjunct : request.delete_conjuncts) {
+ std::set<int> positions;
+ conjunct->root()->collect_slot_column_ids(positions);
+ for (const int position : positions) {
+ if (position >= 0) {
+ delete_positions.insert(cast_set<size_t>(position));
+ }
+ }
+ }
+
+ for (const auto& stage : schedule.remaining_stages) {
+ if (stage.raw_disjunction_branches.empty() ||
+ _disabled_raw_disjunctions.contains(stage.expression.get())) {
+ continue;
+ }
+ std::vector<RawDisjunctionBranchReader> branch_readers;
+ branch_readers.reserve(stage.raw_disjunction_branches.size());
+ bool usable = true;
+ for (const auto& branch : stage.raw_disjunction_branches) {
+ const auto predicate_index_it =
+
_predicate_indices_by_position_scratch.find(branch.position);
+ if (predicate_index_it ==
_predicate_indices_by_position_scratch.end()) {
+ usable = false;
+ break;
+ }
+ const auto& column =
request.predicate_columns[predicate_index_it->second];
+ const auto local_id = column.column_id();
+ if (!local_id.is_valid() ||
+ local_id.value() >= static_cast<int32_t>(file_schema.size())) {
+ usable = false;
+ break;
+ }
+ const auto& column_schema = file_schema[local_id.value()];
+ DORIS_CHECK(column_schema != nullptr);
+ if (column_schema->leaf_column_id < 0 ||
+ column_schema->leaf_column_id >=
+ static_cast<int>(row_group_metadata.columns.size())) {
+ usable = false;
+ break;
+ }
+ const auto& column_chunk =
row_group_metadata.columns[column_schema->leaf_column_id];
+ if (!column_chunk.__isset.meta_data) {
+ usable = false;
+ break;
+ }
+ const int expression_column_id = cast_set<int>(branch.position);
+ const bool raw_eligible =
branch.expression->can_execute_on_raw_fixed_values(
+ column_schema->type,
expression_column_id) ||
+
branch.expression->can_execute_on_raw_binary_values(
+ column_schema->type,
expression_column_id) ||
+
branch.expression->can_execute_on_null_map(
+ column_schema->type,
expression_column_id);
+ if (!raw_eligible) {
+ usable = false;
+ break;
+ }
+
+ bool dictionary_eligible = false;
+ if (!branch.expression->raw_predicate_result_for_null()) {
+ dictionary_eligible = supports_row_level_dictionary_filter(
+ *column_schema,
column_chunk.meta_data) &&
+
(branch.expression->can_execute_on_raw_fixed_values(
+ column_schema->type,
expression_column_id) ||
+
branch.expression->can_execute_on_raw_binary_values(
+ column_schema->type,
expression_column_id));
+ }
+ const bool projected = !request.is_predicate_only(local_id);
+ const int minimum_null_denominator = dictionary_eligible &&
!projected ? 10 : 2;
+ if ((!dictionary_eligible && projected) ||
+ !has_null_fraction_at_least(column_chunk.meta_data, 1,
minimum_null_denominator)) {
+ // Branch readers pay off only after avoiding enough nullable
materialization. The
+ // paired matrix establishes conservative 10% dictionary and
50% PLAIN thresholds;
+ // projected PLAIN branches retain the residual path because
they decode twice.
+ usable = false;
+ break;
+ }
+ const bool use_predicate_reader =
+ request.is_predicate_only(local_id) &&
+ remaining_position_occurrences[branch.position] == 1 &&
+ !delete_positions.contains(branch.position) &&
+
!schedule.single_column_conjuncts.contains(branch.position);
+ if (dictionary_eligible) {
+
update_counter_if_not_null(_scan_profile.dict_filter_candidate_columns, 1);
+ }
+
+ std::unique_ptr<ParquetColumnReader> column_reader;
+ if (!use_predicate_reader || dictionary_eligible) {
+ RETURN_IF_ERROR(NativeColumnReader::create(
+ *column_schema, &column, file_context.native_file,
+ file_context.native_metadata, row_group_idx,
_current_selected_ranges,
+ _current_offset_indexes, _timezone,
file_context.native_io_ctx,
+ _runtime_state, file_context.native_page_cache_enabled,
+ file_context.native_page_cache_file_key,
dictionary_eligible,
+ _scan_profile.column_reader_profile, &column_reader));
+ }
+
+ std::optional<IColumn::Filter> dictionary_filter;
+ if (dictionary_eligible) {
+ MutableColumnPtr dictionary_values;
+ {
+ SCOPED_TIMER(_scan_profile.dict_filter_read_dict_time);
+ auto dictionary_result =
column_reader->dictionary_values();
Review Comment:
[P1] Disable raw OR when the dictionary probe fails
This is an optional optimization probe, but on failure this branch is still
installed with no `dictionary_filter`; for a predicate-only single-use column,
it is also installed with `reader == nullptr`. A fully dictionary-encoded
DATE/TIMESTAMP/DECIMAL column can reach this when converting one dictionary
entry fails. Execution then reuses the ordinary reader, direct fixed-width
filtering refuses the dictionary encoding, and line 3377 turns the refusal into
`InternalError` instead of running the supported residual path (even if the bad
entry is unreferenced). Mark the entire OR stage unusable when a
dictionary-eligible branch cannot produce its bitmap, and cover referenced and
unreferenced invalid dictionary entries. This probe-failure case is not fixed
by the complete-encoding preflight requested in the existing mixed-encoding
thread.
##########
be/benchmark/parquet/benchmark_parquet_reader.hpp:
##########
@@ -171,9 +203,9 @@ inline ::parquet::Encoding::type file_encoding(Encoding
encoding) {
}
inline std::string fixture_name(const ReaderScenario& scenario) {
- return "v2_" + to_string(scenario.encoding) + "_" +
to_string(scenario.value_type) + "_null" +
- std::to_string(scenario.null_percent) + "_" +
to_string(scenario.null_pattern) + "_w" +
- std::to_string(scenario.schema_width) + "_p" +
+ return "v4_" + to_string(scenario.operation) + "_" +
to_string(scenario.encoding) + "_" +
Review Comment:
[P2] Keep byte-identical operations on one fixture identity
Adding `operation` to every path splits ordinary FULL/PREDICATE/LIMIT/etc.
cases into separate files even though only `MULTI_COLUMN_DNF_SCAN` changes
generated bytes (its c0 category corpus). This contradicts the benchmark
contract that fixture keys contain only content-changing axes, multiplies the
retained temp corpus, and gives identical data different footer/page-cache
identities across operations. Add an explicit corpus/data-shape tag for the
exceptional DNF contents while preserving the shared key for byte-identical
operations.
##########
be/benchmark/parquet/benchmark_parquet_reader.hpp:
##########
@@ -408,6 +448,75 @@ inline VExprContextSPtr
make_complex_residual_predicate(int selectivity_percent,
return VExprContext::create_shared(std::move(compound));
}
+inline VExprSPtr make_compound_predicate(TExprOpcode::type opcode, VExprSPtr
left,
+ VExprSPtr right) {
+ const auto bool_type = make_nullable(std::make_shared<DataTypeUInt8>());
+ TExprNode node;
+ node.__set_node_type(TExprNodeType::COMPOUND_PRED);
+ node.__set_opcode(opcode);
+ node.__set_type(bool_type->to_thrift());
+ node.__set_num_children(2);
+ node.__set_is_nullable(true);
+ auto compound = VCompoundPred::create_shared(node);
+ compound->add_child(std::move(left));
+ compound->add_child(std::move(right));
+ return compound;
+}
+
+inline VExprContextSPtr make_multi_column_or_predicate(const ReaderScenario&
scenario,
+ const std::array<int,
3>& positions,
+ const DataTypePtr&
value_type) {
+ const auto literal = [&](int value) -> VExprSPtr {
+ if (scenario.value_type == ValueType::DECIMAL64) {
+ return VLiteral::create_shared(
+ remove_nullable(value_type),
+ Field::create_field<TYPE_DECIMAL64>(Decimal64 {value *
100}));
+ }
+ return VLiteral::create_shared(remove_nullable(value_type),
+ Field::create_field<TYPE_INT>(value));
+ };
+ const auto make_between = [&](int position) {
+ auto slot = [&] {
+ return VSlotRef::create_shared(position, position, -1, value_type,
+ "c" + std::to_string(position));
+ };
+ auto lower = make_int32_comparison("ge", TExprOpcode::GE, slot(),
literal(0));
+ auto upper = make_int32_comparison("lt", TExprOpcode::LT, slot(),
+
literal(scenario.selectivity_percent));
+ return make_compound_predicate(TExprOpcode::COMPOUND_AND,
std::move(lower),
+ std::move(upper));
+ };
+ auto first_two = make_compound_predicate(TExprOpcode::COMPOUND_OR,
make_between(positions[0]),
Review Comment:
[P2] Make the simple-OR benchmark branch-sensitive
All three predicate columns reuse the same DECIMAL `row % 100` values/nulls,
and `make_multi_column_or_predicate()` applies the same interval to each one.
Consequently `p(c0) OR p(c1) OR p(c2)` is indistinguishable from a single
branch (or even an AND of the maps). Since `scan_reader()` checks only total
row count, dropped branches and equal-cardinality wrong output can benchmark as
correct. Give the columns/bounds deterministic cross-counterexamples and
validate an independent row-id or payload checksum plus legacy/raw equality
outside the timed region. This is separate from the existing DNF-fixture oracle
thread.
--
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]
