github-actions[bot] commented on code in PR #66423:
URL: https://github.com/apache/doris/pull/66423#discussion_r3710564099
##########
be/src/exprs/vin_predicate.cpp:
##########
@@ -218,7 +218,7 @@ ZoneMapFilterResult
VInPredicate::evaluate_bloom_filter(const BloomFilterEvalCon
bool VInPredicate::can_evaluate_bloom_filter() const {
return _zonemap_materialized && !_is_not_in &&
Review Comment:
[P1] Materialize nested IN probes before checking Bloom capability
This new accessor-aware check still requires `_zonemap_materialized`, but
`_materialize_for_zonemap_filter()` returns early unless child 0 is a direct
slot. A production `element_at(s, 'a') IN (2, 4)` therefore always reports
false here, so the nested-IN part of this feature is unreachable; the test's
`BloomInExpr` masks this by hard-coding capability and values. Please allow
valid primitive nested probes to materialize their constant set while retaining
the direct-slot gates for ZoneMap/dictionary/raw paths, and add a prepared
production `VInPredicate` test.
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -814,30 +889,34 @@ ParquetRowGroupPruneReason
native_bloom_filter_prune_reason(
continue;
}
const auto* column_schema = resolve_local_leaf_schema(file_schema,
*file_column_id);
- if (column_schema == nullptr || column_schema->type == nullptr ||
- !native_metadata_predicate_is_type_safe(*column_schema) ||
- !bloom_filter_supported(*column_schema) ||
- column_schema->leaf_column_id >=
static_cast<int>(row_group.columns.size())) {
+ if (column_schema == nullptr) {
continue;
}
- const auto& chunk = row_group.columns[column_schema->leaf_column_id];
- if (!chunk.__isset.meta_data) {
+ if (bloom_filter_excludes(*column_schema, slot_index, conjuncts)) {
+ return ParquetRowGroupPruneReason::BLOOM_FILTER;
+ }
+ }
+
+ for (const auto& conjunct : request.conjuncts) {
+ if (conjunct == nullptr || conjunct->root() == nullptr ||
+ !conjunct->root()->can_evaluate_bloom_filter()) {
continue;
}
- std::unique_ptr<native::BlockSplitBloomFilter> bloom_filter;
- Status status;
- {
- int64_t timer_sink = 0;
- SCOPED_RAW_TIMER(pruning_stats == nullptr ? &timer_sink
- :
&pruning_stats->bloom_filter_read_time);
- status = read_native_bloom_filter(chunk.meta_data,
file_context->native_file,
- file_context->native_io_ctx,
&bloom_filter);
+ auto probe =
expr_zonemap::extract_bloom_filter_predicate_probe(conjunct->root());
+ if (!probe.has_value() || probe->path.empty()) {
+ continue;
+ }
+ const auto file_column_id = file_column_id_by_block_position(request,
probe->slot_index);
+ if (!file_column_id.has_value()) {
+ continue;
}
- if (!status.ok() || bloom_filter == nullptr) {
+ const auto* column_schema =
+ resolve_bloom_filter_leaf_schema(file_schema, *file_column_id,
*probe);
+ if (column_schema == nullptr ||
+ !expr_zonemap::data_types_compatible(column_schema->type,
probe->value_type)) {
continue;
}
- if (ParquetStatisticsUtils::NativeBloomFilterExcludes(*column_schema,
slot_index, conjuncts,
- *bloom_filter)) {
+ if (bloom_filter_excludes(*column_schema, probe->slot_index,
{conjunct})) {
Review Comment:
[P2] Share one Bloom read across predicates on the same nested leaf
This loop calls `bloom_filter_excludes` once per conjunct, and each call
reads the header and payload and reparses the same physical Bloom. Two
independently retained Bloom-capable predicates, such as equality and non-null
null-safe equality on the same nested leaf, therefore duplicate remote I/O for
every surviving Row Group, whereas the top-level path groups predicates and
reads once. Please group by the resolved physical leaf (or cache the decoded
Bloom per leaf) and add a counting-reader test that asserts one header/payload
pair.
##########
be/src/exprs/expr_zonemap_filter.cpp:
##########
@@ -213,6 +246,113 @@ std::optional<SlotLiteral> extract_slot_and_literal(const
VExprSPtrs& args) {
return std::nullopt;
}
+std::optional<BloomFilterProbe> extract_bloom_filter_probe(const VExprSPtr&
expr) {
+ if (expr == nullptr || expr->data_type() == nullptr) {
+ return std::nullopt;
+ }
+ if (auto slot = std::dynamic_pointer_cast<VSlotRef>(expr); slot) {
+ return BloomFilterProbe {
+ .slot_index = slot->column_id(), .value_type =
slot->data_type(), .path = {}};
+ }
+ if ((expr->fn().name.function_name != "element_at" &&
+ expr->fn().name.function_name != "struct_element") ||
+ expr->get_num_children() != 2) {
+ return std::nullopt;
+ }
+
+ auto probe = extract_bloom_filter_probe(expr->get_child(0));
+ auto selector = field_from_literal_expr(expr->get_child(1));
+ if (!probe.has_value() || !selector.has_value() ||
selector->first.is_null()) {
+ return std::nullopt;
+ }
+ const auto parent_type = remove_nullable(expr->get_child(0)->data_type());
+ if (parent_type == nullptr) {
+ return std::nullopt;
+ }
+
+ BloomFilterPathElement path_element;
+ switch (parent_type->get_primitive_type()) {
+ case TYPE_STRUCT: {
+ path_element.kind = BloomFilterPathKind::STRUCT_FIELD;
+ const auto selector_type = remove_nullable(selector->second);
+ if (selector_type == nullptr) {
+ return std::nullopt;
+ }
+ if (is_string_type(selector_type->get_primitive_type())) {
+ path_element.field_name = selector->first.get<TYPE_STRING>();
+ } else {
+ auto ordinal = struct_field_ordinal(selector->first);
+ if (!ordinal.has_value()) {
+ return std::nullopt;
+ }
+ path_element.field_ordinal = *ordinal;
+ }
+ break;
+ }
+ case TYPE_ARRAY:
+ // Array element positions share one repeated Parquet leaf; membership
in that leaf is a
+ // necessary condition for any element_at(array, constant) equality to
match.
+ path_element.kind = BloomFilterPathKind::LIST_ELEMENT;
+ break;
+ default:
+ return std::nullopt;
+ }
+ probe->value_type = expr->data_type();
+ probe->path.push_back(std::move(path_element));
+ return probe;
+}
+
+std::optional<BloomFilterProbe> extract_bloom_filter_predicate_probe(const
VExprSPtr& expr) {
+ if (auto probe = extract_bloom_filter_probe(expr); probe.has_value()) {
+ return probe;
+ }
+ if (expr == nullptr) {
+ return std::nullopt;
+ }
+ std::optional<BloomFilterProbe> result;
+ for (uint16_t child_idx = 0; child_idx < expr->get_num_children();
++child_idx) {
+ const auto& child = expr->get_child(child_idx);
+ if (child == nullptr || child->is_literal()) {
+ continue;
+ }
+ auto child_probe = extract_bloom_filter_predicate_probe(child);
+ if (!child_probe.has_value()) {
Review Comment:
[P1] Reject compound trees without one unique Bloom probe
Here `nullopt` means either that a child has no probe or that it contains
conflicting nested probes, and the parent silently ignores both cases. For
`((s.a = 1 AND s.b = 2) OR s.a = 3)`, the inner AND is skipped, this returns
`s.a`, and `VCompoundPred` evaluates `s.b = 2` against the `s.a` Bloom; a Row
Group containing `(1,2)` can therefore be reported as `kNoMatch` (or
incompatible leaf types can hit the type check). If a Bloom-capable child does
not resolve to the same unique probe, please make the whole compound
ineligible, and cover same- and mixed-type sibling leaves.
##########
be/src/exprs/expr_zonemap_filter.cpp:
##########
@@ -213,6 +246,113 @@ std::optional<SlotLiteral> extract_slot_and_literal(const
VExprSPtrs& args) {
return std::nullopt;
}
+std::optional<BloomFilterProbe> extract_bloom_filter_probe(const VExprSPtr&
expr) {
+ if (expr == nullptr || expr->data_type() == nullptr) {
+ return std::nullopt;
+ }
+ if (auto slot = std::dynamic_pointer_cast<VSlotRef>(expr); slot) {
+ return BloomFilterProbe {
+ .slot_index = slot->column_id(), .value_type =
slot->data_type(), .path = {}};
+ }
+ if ((expr->fn().name.function_name != "element_at" &&
+ expr->fn().name.function_name != "struct_element") ||
+ expr->get_num_children() != 2) {
+ return std::nullopt;
+ }
+
+ auto probe = extract_bloom_filter_probe(expr->get_child(0));
+ auto selector = field_from_literal_expr(expr->get_child(1));
+ if (!probe.has_value() || !selector.has_value() ||
selector->first.is_null()) {
+ return std::nullopt;
+ }
+ const auto parent_type = remove_nullable(expr->get_child(0)->data_type());
+ if (parent_type == nullptr) {
+ return std::nullopt;
+ }
+
+ BloomFilterPathElement path_element;
+ switch (parent_type->get_primitive_type()) {
+ case TYPE_STRUCT: {
+ path_element.kind = BloomFilterPathKind::STRUCT_FIELD;
+ const auto selector_type = remove_nullable(selector->second);
+ if (selector_type == nullptr) {
+ return std::nullopt;
+ }
+ if (is_string_type(selector_type->get_primitive_type())) {
+ path_element.field_name = selector->first.get<TYPE_STRING>();
+ } else {
+ auto ordinal = struct_field_ordinal(selector->first);
+ if (!ordinal.has_value()) {
+ return std::nullopt;
+ }
+ path_element.field_ordinal = *ordinal;
+ }
+ break;
+ }
+ case TYPE_ARRAY:
+ // Array element positions share one repeated Parquet leaf; membership
in that leaf is a
+ // necessary condition for any element_at(array, constant) equality to
match.
+ path_element.kind = BloomFilterPathKind::LIST_ELEMENT;
+ break;
+ default:
+ return std::nullopt;
+ }
+ probe->value_type = expr->data_type();
+ probe->path.push_back(std::move(path_element));
+ return probe;
+}
+
+std::optional<BloomFilterProbe> extract_bloom_filter_predicate_probe(const
VExprSPtr& expr) {
+ if (auto probe = extract_bloom_filter_probe(expr); probe.has_value()) {
+ return probe;
+ }
+ if (expr == nullptr) {
+ return std::nullopt;
+ }
+ std::optional<BloomFilterProbe> result;
+ for (uint16_t child_idx = 0; child_idx < expr->get_num_children();
++child_idx) {
+ const auto& child = expr->get_child(child_idx);
+ if (child == nullptr || child->is_literal()) {
+ continue;
+ }
+ auto child_probe = extract_bloom_filter_predicate_probe(child);
+ if (!child_probe.has_value()) {
+ continue;
+ }
+ if (result.has_value() && !bloom_filter_probes_equal(*result,
*child_probe)) {
+ return std::nullopt;
+ }
+ result = std::move(child_probe);
+ }
+ return result;
+}
+
+std::optional<SlotLiteral> extract_bloom_filter_slot_and_literal(const
VExprSPtrs& args) {
+ if (args.size() != 2) {
+ return std::nullopt;
+ }
+ for (size_t probe_idx = 0; probe_idx < args.size(); ++probe_idx) {
+ auto probe = extract_bloom_filter_probe(args[probe_idx]);
+ auto literal = field_from_literal_expr(args[1 - probe_idx]);
+ if (!probe.has_value() || !literal.has_value()) {
+ continue;
+ }
+ auto [literal_value, literal_type] = std::move(*literal);
+ return SlotLiteral {.slot_index = probe->slot_index,
+ .slot_type = probe->value_type,
+ .literal = std::move(literal_value),
+ .literal_type = std::move(literal_type),
+ .literal_on_left = probe_idx == 1};
+ }
+ return std::nullopt;
+}
+
+bool can_evaluate_bloom_filter_equality(const VExprSPtrs& args) {
+ auto slot_literal = extract_bloom_filter_slot_and_literal(args);
+ return slot_literal.has_value() && !slot_literal->literal.is_null() &&
Review Comment:
[P1] Preserve FLOAT/DOUBLE equality classes when probing Parquet Blooms
This capability now admits nested FLOAT/DOUBLE equality (and is reused by
null-safe equality), but the native adapter hashes the literal's raw IEEE
bytes. Doris considers `+0.0 == -0.0` and equates NaNs, while Parquet Bloom
filters hash their distinct PLAIN encodings, so a Bloom containing `-0.0` can
reject a `+0.0` probe and falsely skip a matching Row Group. Please probe both
zero encodings and conservatively return `kMayMatch` for NaN, or disable
FLOAT/DOUBLE Bloom pruning; add an external-writer signed-zero/NaN regression.
--
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]
