github-actions[bot] commented on code in PR #66423:
URL: https://github.com/apache/doris/pull/66423#discussion_r3711371812
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -814,30 +896,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) {
Review Comment:
[P1] Preserve earlier errors before nested Bloom pruning
`request.conjuncts` preserves row-level order, but this new loop can
evaluate a later nested Bloom past an earlier error-preserving predicate. For
example, with `assert_true(x <> 0, 'bad')` followed by `element_at(s, 'a') =
2`, a Row Group containing `x = 0` whose nested-leaf Bloom excludes 2 returns
`BLOOM_FILTER` here, so no row reaches `assert_true` and the required error
becomes a successful empty result. The existing TODO at lines 507-508 already
records the needed invariant. Please fence metadata pruning at the first
conjunct that is unsafe on selected rows (across all metadata stages), and add
a regression with an earlier `assert_true` plus a later Bloom-negative nested
predicate.
##########
be/src/exprs/expr_zonemap_filter.cpp:
##########
@@ -213,6 +264,123 @@ 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:
Review Comment:
[P2] Connect LIST-to-STRUCT probes to production localization
The recursive extractor accepts a `LIST_ELEMENT -> STRUCT_FIELD` path, but
`TableColumnMapper::collect_struct_element_chain()` rejects a struct accessor
whose parent is the computed array element. Thus `element_at(element_at(items,
1), 'a') = 7` produces no file-local conjunct even when the table and file
`ARRAY<STRUCT<a: INT>>` schemas are identical, and this Parquet Bloom path is
never reached; the existing `ArrayWrapperDoesNotBuildNestedPredicateFilter`
test confirms that request is empty. Please add a schema-safe localization path
plus a mapper-to-Parquet regression, or narrow the advertised recursive
LIST/STRUCT support to the path shapes that can actually reach the reader.
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -806,6 +855,39 @@ ParquetRowGroupPruneReason
native_bloom_filter_prune_reason(
if (file_context == nullptr || file_context->native_file == nullptr) {
return ParquetRowGroupPruneReason::NONE;
}
+ std::map<int, std::unique_ptr<native::BlockSplitBloomFilter>>
bloom_filters_by_leaf;
Review Comment:
[P1] Bound the lifetime of decoded Blooms across leaves
This map retains every successfully decoded filter until the Row Group check
returns, and each `BlockSplitBloomFilter` owns a copy of a payload accepted up
to `BloomFilter::MAXIMUM_BYTES` (128 MiB). On the no-prune path, eight
predicate leaves with valid maximum-size Blooms therefore keep roughly 1 GiB
live in one scanner (plus each transient read buffer), whereas the previous
loop released one leaf before loading the next. Please preserve the one-read
guarantee for repeated predicates without retaining all distinct leaves—for
example, track remaining uses and erase a leaf after its last evaluation—and
cover the multi-leaf retention bound.
--
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]
