github-actions[bot] commented on code in PR #66423:
URL: https://github.com/apache/doris/pull/66423#discussion_r3714634452
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -806,39 +864,91 @@ ParquetRowGroupPruneReason
native_bloom_filter_prune_reason(
if (file_context == nullptr || file_context->native_file == nullptr) {
return ParquetRowGroupPruneReason::NONE;
}
+ struct BloomProbeGroup {
+ const ParquetColumnSchema* column_schema = nullptr;
+ int slot_index = -1;
+ VExprContextSPtrs conjuncts;
+ };
+ std::map<int, std::vector<BloomProbeGroup>> probes_by_leaf;
+ const auto add_probe = [&](const ParquetColumnSchema& column_schema, int
slot_index,
+ VExprContextSPtrs conjuncts) {
+ if (column_schema.type == nullptr ||
+ !native_metadata_predicate_is_type_safe(column_schema) ||
+ !bloom_filter_supported(column_schema) ||
column_schema.leaf_column_id < 0 ||
+ column_schema.leaf_column_id >=
static_cast<int>(row_group.columns.size())) {
+ return;
+ }
+ const auto& chunk = row_group.columns[column_schema.leaf_column_id];
+ if (!chunk.__isset.meta_data) {
+ return;
+ }
+ probes_by_leaf[column_schema.leaf_column_id].push_back({.column_schema
= &column_schema,
+ .slot_index =
slot_index,
+ .conjuncts =
std::move(conjuncts)});
+ };
+
+ const size_t safe_count =
+ std::min(request.metadata_pruning_safe_conjunct_count,
request.conjuncts.size());
+ const VExprContextSPtrs safe_conjuncts(request.conjuncts.begin(),
+ request.conjuncts.begin() +
safe_count);
const auto conjuncts_by_slot = collect_conjuncts_by_single_slot(
- request.conjuncts, expr_zonemap::single_slot_bloom_filter_index);
+ safe_conjuncts, expr_zonemap::single_slot_bloom_filter_index);
for (const auto& [slot_index, conjuncts] : conjuncts_by_slot) {
const auto file_column_id = file_column_id_by_block_position(request,
slot_index);
if (!file_column_id.has_value()) {
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) {
+ add_probe(*column_schema, slot_index, conjuncts);
+ }
+
+ for (const auto& conjunct : safe_conjuncts) {
+ if (conjunct == nullptr || conjunct->root() == nullptr ||
+ !conjunct->root()->can_evaluate_bloom_filter()) {
continue;
}
+ 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;
+ }
+ 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;
+ }
+ add_probe(*column_schema, probe->slot_index, {conjunct});
+ }
+
+ for (const auto& [leaf_column_id, probes] : probes_by_leaf) {
std::unique_ptr<native::BlockSplitBloomFilter> bloom_filter;
- Status status;
+ int64_t timer_sink = 0;
{
- 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);
+ const auto status = read_native_bloom_filter(
Review Comment:
[P2] Expose Bloom probe fallback outcomes
This broadened per-leaf path erases every non-OK Bloom load result—missing
metadata, unsupported headers, malformed/truncated payloads, and remote I/O
failures all become the same silent Row Group retention. The Profile exposes
only Bloom read time and successfully pruned groups, so operators cannot
distinguish a legitimate may-match from the feature failing to load every
Bloom. The mandatory format-v2 guide calls for attempts, successes,
conservative fallbacks, and corrupt rejections separately. Please add and
publish those counters (with tests for missing, malformed, truncated, and
I/O-error cases).
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -842,11 +842,21 @@ static bool needs_complex_file_slot_cast(const
DataTypePtr& file_type,
static bool collect_struct_element_chain(const VExprSPtr& expr,
std::vector<VExprSPtr>* chain) {
DORIS_CHECK(chain != nullptr);
- if (!is_struct_element_expr(expr)) {
+ const auto is_supported_element = [](const VExprSPtr& candidate) {
+ if (is_struct_element_expr(candidate)) {
+ return true;
+ }
+ return candidate != nullptr && candidate->get_num_children() == 2 &&
Review Comment:
[P1] Preserve required child nullability through ARRAY localization
This newly admits ARRAY accessors into a guard that compares each physical
child with the accessor expression's result type. Production
`FunctionArrayElement` always makes ARRAY and STRUCT access results Nullable,
so for table `items ARRAY<STRUCT<a: INT NOT NULL>>`, file `items
ARRAY<STRUCT<a: Nullable(INT)>>`, and `items[1].a > 10`, the check later sees
Nullable(INT) versus Nullable(INT) and localizes the filter. The file reader
can then discard `a=NULL` before TableReader's required-child alignment reports
the schema violation, changing an error into a successful smaller result. The
existing guard test uses a synthetic non-null result type, and the new ARRAY
test uses identical schemas. Please validate against the mapped table child
types/nullability and add a production-expression schema-evolution regression.
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -760,8 +760,13 @@ Status TableReader::_build_table_filters_from_conjuncts() {
if (in_safe_prefix && !_is_safe_to_pre_execute(conjunct)) {
in_safe_prefix = false;
}
+ const size_t first_new_filter = _table_filters.size();
RETURN_IF_ERROR(
build_table_filters_from_conjunct(conjunct, _runtime_state,
&_table_filters));
+ for (size_t filter_idx = first_new_filter; filter_idx <
_table_filters.size();
+ ++filter_idx) {
+ _table_filters[filter_idx].metadata_pruning_safe = in_safe_prefix;
Review Comment:
[P1] Keep the new Bloom predicates inside the safe prefix
Production `element_at`/`struct_element` nodes are `VectorizedFnCall`s, but
their selected-row-safety allowlist excludes both accessors; it also excludes
`eq_for_null` itself. Thus a nested equality/IN fails on its accessor child and
even top-level `x <=> 7` fails at the root. This assignment excludes each of
those filters from the safe prefix; in a request containing only such a
predicate, the safe count is zero, so Statistics/Dictionary/Bloom/PageIndex
never run the new production capabilities. The tests bypass this with custom
default-safe expressions or hand-built `TableFilter`/`FileScanRequest` objects.
Please classify the proven-total accessor/null-safe shapes without weakening
the error barrier, and add a real TableReader-to-Parquet test that observes a
positive safe count and Bloom read/prune.
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -806,39 +864,91 @@ ParquetRowGroupPruneReason
native_bloom_filter_prune_reason(
if (file_context == nullptr || file_context->native_file == nullptr) {
return ParquetRowGroupPruneReason::NONE;
}
+ struct BloomProbeGroup {
+ const ParquetColumnSchema* column_schema = nullptr;
+ int slot_index = -1;
+ VExprContextSPtrs conjuncts;
+ };
+ std::map<int, std::vector<BloomProbeGroup>> probes_by_leaf;
+ const auto add_probe = [&](const ParquetColumnSchema& column_schema, int
slot_index,
+ VExprContextSPtrs conjuncts) {
+ if (column_schema.type == nullptr ||
+ !native_metadata_predicate_is_type_safe(column_schema) ||
+ !bloom_filter_supported(column_schema) ||
column_schema.leaf_column_id < 0 ||
+ column_schema.leaf_column_id >=
static_cast<int>(row_group.columns.size())) {
+ return;
+ }
+ const auto& chunk = row_group.columns[column_schema.leaf_column_id];
+ if (!chunk.__isset.meta_data) {
+ return;
+ }
+ probes_by_leaf[column_schema.leaf_column_id].push_back({.column_schema
= &column_schema,
+ .slot_index =
slot_index,
+ .conjuncts =
std::move(conjuncts)});
+ };
+
+ const size_t safe_count =
+ std::min(request.metadata_pruning_safe_conjunct_count,
request.conjuncts.size());
+ const VExprContextSPtrs safe_conjuncts(request.conjuncts.begin(),
+ request.conjuncts.begin() +
safe_count);
const auto conjuncts_by_slot = collect_conjuncts_by_single_slot(
- request.conjuncts, expr_zonemap::single_slot_bloom_filter_index);
+ safe_conjuncts, expr_zonemap::single_slot_bloom_filter_index);
for (const auto& [slot_index, conjuncts] : conjuncts_by_slot) {
const auto file_column_id = file_column_id_by_block_position(request,
slot_index);
if (!file_column_id.has_value()) {
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) {
+ add_probe(*column_schema, slot_index, conjuncts);
+ }
+
+ for (const auto& conjunct : safe_conjuncts) {
+ if (conjunct == nullptr || conjunct->root() == nullptr ||
+ !conjunct->root()->can_evaluate_bloom_filter()) {
continue;
}
+ 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;
+ }
+ 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;
+ }
+ add_probe(*column_schema, probe->slot_index, {conjunct});
+ }
+
+ for (const auto& [leaf_column_id, probes] : probes_by_leaf) {
Review Comment:
[P2] Preserve first-probe order when grouping Blooms
This physical-leaf-keyed map delays evaluation and sorts groups by Parquet
leaf ID. With a reordered field-ID schema, logical slot 0 can map to leaf 1 and
exclude while slot 1 maps to leaf 0 and may-match; the previous loop stopped
after leaf 1, but this loop reads leaf 0 first. Since each accepted payload may
be 128 MiB, lower-numbered leaves can add large remote I/O before the
inevitable prune. Please retain per-leaf sharing and one-live-payload ownership
while iterating groups in first-probe order, and add a reversed
block-slot/physical-leaf test that proves the later may-match payload is not
read.
--
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]
