[improvement](be) Prune nested Parquet leaves with Bloom filters - #66423
[improvement](be) Prune nested Parquet leaves with Bloom filters#66423Gabriel39 wants to merge 5 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: File Scanner V2 only used native Parquet Bloom filters for top-level primitive slots, and null-safe equality did not advertise Bloom evaluation. Resolve exact STRUCT and LIST accessor paths to localized primitive leaves, evaluate equality and IN predicates against the leaf Bloom filter, and allow null-safe equality only for non-null literals. Uncertain or missing paths remain conservative. The focused metadata test reduces selected row groups from one to zero for an absent nested value while retaining the row group for present values and missing paths.
### Release note
Enable native Parquet Bloom pruning for STRUCT and LIST leaves and for null-safe equality with non-null constants.
### Check List (For Author)
- Test: Unit Test
- ExprZonemapFilterTest.*
- ParquetBloomFilterPruningTest.*
- Behavior changed: Yes. Eligible nested predicates and non-null null-safe equality can now prune row groups through native Parquet Bloom filters.
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes. The implementation is focused, but four material issues remain: compound predicates can bind a Boolean subtree to the wrong nested Bloom leaf; production nested IN never materializes and cannot reach the new capability; FLOAT/DOUBLE raw Parquet Bloom hashes do not preserve Doris signed-zero/NaN equality; and same-leaf nested predicates reread and reparse the Bloom per conjunct.
Critical checkpoint conclusions:
- Goal and correctness: simple STRUCT/LIST leaf resolution is conservative, but M1 and M4 can falsely skip matching Row Groups, and M2 leaves a stated production feature unreachable.
- Compatibility and mapping: supported name/ordinal localization and missing-path fallback are sound; external Parquet FLOAT/DOUBLE PLAIN-byte compatibility is not.
- Parallel paths and lifecycle: direct-slot preparation/grouping was not carried through to nested
INmaterialization or nested same-leaf Bloom sharing. - Performance and observability: M3 duplicates remote header/payload reads and parsing; existing timing records the cost but the tests do not count reads.
- Tests: the new tests cover synthetic happy paths but bypass production
VInPredicatepreparation, complete mapper/footer integration, multi-leaf compound ownership, repeated-read counting, and external-writer FLOAT/DOUBLE bytes. - Error handling, ownership, concurrency, configuration, and persistence: missing/unsupported/unreadable metadata otherwise retains the Row Group; temporary Bloom ownership is scoped; no new shared-state, locking, configuration, persistence, transaction, data-write, or FE/BE propagation surface was introduced.
- User focus: no additional focus was supplied, so the entire PR was reviewed without a narrower scope.
Validation was static-only as required by the review environment; builds and tests were intentionally not run. The bundled and live head/base were verified as 19d48a3a90d615c07ab73711b079e90f6839fe33 / 99dd2b330c32f903f39e4a9f1bda154544682298. Three review rounds converged with all full-review and risk-focused reviewers returning NO_NEW_VALUABLE_FINDINGS on this exact four-comment set.
| continue; | ||
| } | ||
| auto child_probe = extract_bloom_filter_predicate_probe(child); | ||
| if (!child_probe.has_value()) { |
There was a problem hiding this 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.
| @@ -218,7 +218,7 @@ ZoneMapFilterResult VInPredicate::evaluate_bloom_filter(const BloomFilterEvalCon | |||
|
|
|||
| bool VInPredicate::can_evaluate_bloom_filter() const { | |||
| return _zonemap_materialized && !_is_not_in && | |||
There was a problem hiding this 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.
| } | ||
| if (ParquetStatisticsUtils::NativeBloomFilterExcludes(*column_schema, slot_index, conjuncts, | ||
| *bloom_filter)) { | ||
| if (bloom_filter_excludes(*column_schema, probe->slot_index, {conjunct})) { |
There was a problem hiding this 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.
|
|
||
| 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() && |
There was a problem hiding this 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.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes. The implementation is focused and the current head addresses the four existing review threads, but three material issues remain: nested Bloom pruning can suppress an earlier required query error; the decoded-Bloom cache can retain unbounded aggregate memory across distinct leaves; and LIST-to-STRUCT probes remain unreachable through the production mapper even for identical schemas.
Critical checkpoint conclusions:
- Goal, scope, and correctness: direct STRUCT/LIST leaf resolution, nested IN materialization, unique compound ownership, null-safe equality, and FLOAT/DOUBLE equality handling are now conservative. MAIN-001 is a correctness regression, MAIN-002 is an availability/memory regression, and MAIN-003 leaves part of the stated nested path support undelivered. The eight-file change is otherwise focused.
- Concurrency and lifecycle: pruning state is stack-local to one Row Group call and introduces no shared mutable state or locks. Scan-local expressions are cloned, prepared, and opened before metadata selection. The cache is released at return, but its aggregate successful-entry lifetime is the MAIN-002 defect.
- Compatibility, parallel paths, and conditions: no storage format, protocol, configuration, persistence, transaction, data-write, or FE/BE variable surface changes. Missing, unsupported, unreadable, or type-incompatible metadata retains the Row Group; top-level and nested paths share the same physical-leaf identity and conservative fallback. The v1 reader path is unchanged.
- Tests and results: the changed unit tests cover prepared nested IN, direct STRUCT/LIST resolution, compound ownership, signed zero/NaN, and same-leaf read reuse. They do not cover the error-order, multi-leaf retention, or mapper-to-Parquet LIST-to-STRUCT regressions requested inline. Review validation was static-only as required; builds and tests were not run.
- Performance and observability: Bloom read timing and the filtered-Row-Group counter remain correctly scoped, and one read is reused per physical leaf. Aggregate decoded-filter retention still needs a bound; no additional logging or metrics gap was substantiated.
- User focus: no extra focus was supplied, so the complete PR was reviewed without a narrower scope.
Review status: complete and converged after two rounds; every Round 2 full-review and risk-focused agent returned NO_NEW_VALUABLE_FINDINGS on this exact three-comment set. The live base/head were reverified as 99dd2b330c32f903f39e4a9f1bda154544682298 / 866a6906913eb9cd2a5eb3c94c2b57499f77afc9 immediately before submission.
| } | ||
| } | ||
|
|
||
| for (const auto& conjunct : request.conjuncts) { |
There was a problem hiding this 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.
| if (file_context == nullptr || file_context->native_file == nullptr) { | ||
| return ParquetRowGroupPruneReason::NONE; | ||
| } | ||
| std::map<int, std::unique_ptr<native::BlockSplitBloomFilter>> bloom_filters_by_leaf; |
There was a problem hiding this 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.
| } | ||
| break; | ||
| } | ||
| case TYPE_ARRAY: |
There was a problem hiding this 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.
TPC-H: Total hot run time: 29346 ms |
TPC-DS: Total hot run time: 166475 ms |
ClickBench: Total hot run time: 23.93 s |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: the patch is focused and its supported Bloom decisions are otherwise conservative, but two P1 correctness/reachability blockers and two P2 production-operability/performance gaps remain.
Critical checkpoints
- Goal and proof: Not achieved in production. Nested equality/IN and direct
eq_for_nullare excluded from the metadata-safe prefix, so the advertised Bloom path never runs through the real TableReader flow. The added helper, synthetic-expression, and direct-request tests bypass that gate. They also miss nullable-file-to-required-table ARRAY/STRUCT evolution, Bloom fallback outcomes, and cross-leaf read order. - Scope and clarity: The change is reasonably small and concentrated in expression capability, table-to-file localization, request fencing, and Parquet metadata pruning. No unrelated source changes were found.
- Concurrency: No new thread, lock, or shared mutable state is introduced. Initial and refreshed scan requests remain isolated, layout-checked, and activated at Row Group boundaries; no lock-order or deadlock issue was found.
- Lifecycle: Per-leaf grouping now reads a reached leaf once and keeps at most one decoded cross-leaf Bloom payload alive, with early-return destruction. No static-initialization or circular-lifetime issue was found.
- Configuration: No configuration item is added.
- Compatibility: There is no function-symbol, protocol, or storage-format write change. Read-side schema evolution is not safe, however: ARRAY localization can filter away a nullable physical child before TableReader enforces a required table child, changing an error into a successful smaller result.
- Parallel paths and conditions: Statistics, Dictionary, Bloom, PageIndex, constant, and partition pruning all honor the new monotone safety barrier, and late refresh does not cross it. The blocker is the production classifier being too conservative for the newly supported total shapes. Existing compound, nested-IN, signed-zero/NaN, same-leaf-read, payload-lifetime, unsafe-order, and original LIST-localization threads were rechecked and not duplicated.
- Tests and results: The new C++ tests cover helper semantics, conservative barriers, and same-leaf reuse, but not the four production contracts above; no result-file update is applicable. Static inspection only was performed because this review runner explicitly forbids builds and test execution.
- Observability: Insufficient. The query Profile cannot distinguish Bloom attempts, usable filters, missing/unsupported metadata, corruption, or I/O fallbacks; process-wide Bloom residency gauges do not provide those per-query outcomes.
- Transactions, persistence, writes, and FE/BE variables: None are involved.
- Performance: Same-leaf deduplication and one-live-payload ownership are improvements, but sorting groups by physical leaf ID can read lower-numbered may-match payloads (up to 128 MiB each) before the logical first probe that would already prune the Row Group.
- Other issues: No additional distinct correctness, lifecycle, compatibility, configuration, persistence, protocol, performance, observability, or coverage issue survived the final sweep.
Review coverage and completion
review_focus.txtsupplied no additional user focus; the complete 16-file PR was reviewed.- The live head/base still match the authoritative bundle, and no live review/comment appeared beyond the seven already deduplicated threads.
- Three full review rounds completed. Every final-round reviewer returned
NO_NEW_VALUABLE_FINDINGSfor this exact four-comment payload, so the review is complete.
| 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; |
There was a problem hiding this comment.
[P1] Keep the new Bloom predicates inside the safe prefix
Production element_at/struct_element nodes are VectorizedFnCalls, 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.
| if (is_struct_element_expr(candidate)) { | ||
| return true; | ||
| } | ||
| return candidate != nullptr && candidate->get_num_children() == 2 && |
There was a problem hiding this 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.
| : &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( |
There was a problem hiding this 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).
| add_probe(*column_schema, probe->slot_index, {conjunct}); | ||
| } | ||
|
|
||
| for (const auto& [leaf_column_id, probes] : probes_by_leaf) { |
There was a problem hiding this 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.
TPC-H: Total hot run time: 28567 ms |
TPC-DS: Total hot run time: 165948 ms |
ClickBench: Total hot run time: 23.94 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#66423 Problem Summary: Production nested accessors and null-safe equality were excluded from the metadata-pruning safe prefix, ARRAY accessor result nullability could hide a required table child, physical-leaf grouping could reorder large Bloom reads, and unavailable Bloom payloads were not observable. Classify only total accessor shapes, retain declared table child types during localization, preserve first-probe order with one decoded payload at a time, and publish separate attempt, success, fallback, and corruption counters. ### Release note Make nested Parquet Bloom pruning reachable for production accessors while preserving schema validation, probe order, and observable conservative fallbacks. ### Check List (For Author) - Test: Unit Test - 6 focused BE unit tests covering production TableReader integration, nested accessor safety, ARRAY child nullability, Bloom fallback counters, shared reads, and first-probe order - Behavior changed: Yes. Eligible nested predicates can use Parquet Blooms without hiding required-child errors or reordering physical reads. - Does this need documentation: No
|
run buildall |
|
/review |
TPC-H: Total hot run time: 28897 ms |
TPC-DS: Total hot run time: 165446 ms |
ClickBench: Total hot run time: 24.12 s |
There was a problem hiding this comment.
Requesting changes. The Parquet nested-Bloom path is substantially corrected and the change is focused, but one P1 cross-format correctness issue remains: the new request-wide metadata-pruning safe prefix is not consumed by ORC SARG pruning, so ORC can suppress an earlier required error.
Critical checkpoint conclusions:
- Goal and tests: Nested equality, non-NULL null-safe equality, and IN now reach the Parquet Bloom path with conservative physical-leaf and equality semantics. The goal is not fully achieved because the shared error-order fence is incomplete across format readers. The changed BE tests cover mapper localization, safe-prefix classification, leaf grouping, fallback counters, and hash edge cases, but no ORC regression proves an earlier throwing predicate survives a later SARG-negative equality.
- Scope, minimality, and parallel paths: The implementation is concentrated in expression capability, table-to-file localization, request metadata, Parquet pruning/profile state, and focused tests. Parquet Statistics, Dictionary, Bloom, and PageIndex consumers honor the prefix; ORC is the one unmigrated pre-row metadata consumer.
- Concurrency, lifecycle, and memory safety: No new thread, lock, atomic, shared mutable state, cross-TU static, or circular lifetime is introduced. Bloom state is scanner/Row-Group local, each reached physical leaf is read once, and only one decoded payload remains live while that leaf's probes run.
- Error handling, data correctness, and nullable handling: Parquet metadata failures and unsupported types retain the Row Group, physical hashing covers Doris equality classes conservatively, and declared table-child nullability is checked before file filtering. The inline ORC path is the remaining correctness failure because stripe pruning can turn
assert_trueinto a successful empty result. - Compatibility and non-applicable stateful surfaces: No configuration, storage/wire format, transaction, persistence, data-write, or FE/BE variable-propagation change is introduced.
- Performance and observability: Same-leaf read sharing and bounded payload ownership are sound, and attempt/success/fallback/corruption counters are published for eager and deferred Parquet work. The remaining first-probe-order inefficiency is already covered by live discussion 3714634458 and is not duplicated here.
- Test results: Static review only; this review contract forbids builds and test execution, so no test result is claimed and no result-file change applies.
- User focus:
review_focus.txtsupplied no additional guidance, so the complete 21-file PR was reviewed without a narrower scope.
Completion and deduplication: all 11 live inline discussions were rechecked and no substantially similar comment is repeated. Two complete review rounds converged; every Round 2 normal and risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS for this exact one-comment set. The live base/head were reverified as 99dd2b330c32f903f39e4a9f1bda154544682298 / 096a8af5237bad228bf4e0e974e588544ca13ad4 immediately before submission.
| VExprContextSPtrs conjuncts; | ||
| // Metadata pruning may use only this prefix. A later predicate must not jump over an earlier | ||
| // non-deterministic or error-preserving conjunct in the original row-level order. | ||
| size_t metadata_pruning_safe_conjunct_count = std::numeric_limits<size_t>::max(); |
There was a problem hiding this comment.
[P1] Apply this safe prefix to ORC SARG pruning too
This request-wide contract is now honored by Parquet, but OrcReader::_init_search_argument_from_local_filters() still compiles every localized conjunct. For [assert_true(x <> 0, 'bad'), y = 7], this count is zero; the ORC compiler drops the unsupported assert_true yet emits y = 7, so a stripe containing (x=0, y=1) can be rejected before _execute_conjuncts() raises the required error. Please bound ORC SARG construction by min(metadata_pruning_safe_conjunct_count, conjuncts.size()) as well, and add an ORC regression with an earlier throwing predicate and later SARG-negative equality.
### What problem does this PR solve? Issue Number: None Related PR: apache#66423 Problem Summary: ORC built search arguments from every localized conjunct even when an earlier expression was unsafe for metadata pruning. Because ORC can omit unsupported expressions, a later pushdown predicate could prune every stripe and suppress a required row-level error. Limit SARG compilation to the request metadata-safe prefix so stripe pruning cannot cross the error-preserving barrier. ### Release note Preserve row-level error behavior when ORC stripe pruning is enabled. ### Check List (For Author) - Test: Unit Test - Focused ORC safe-prefix and stripe-pruning BE unit tests - Behavior changed: Yes. ORC SARG pruning now stops at the metadata-safe conjunct prefix. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: two P1 correctness gaps remain in ARRAY predicate localization and its interaction with table-side constant pruning. Both can turn a required file-to-table nullability error into successful output or EOF.
Critical checkpoint conclusions:
- Goal and proof: The change broadly reaches nested STRUCT/LIST equality, IN, and null-safe Bloom pruning, with shared physical-leaf reads and bounded error-preserving metadata prefixes. The goal is not yet complete because full ARRAY projections can hide an invalid sibling, and the new accessor-safe classification can let a later constant/default filter bypass a rejected accessor. The added unit tests do not cover either end-to-end sequence.
- Scope and clarity: The implementation is focused on expression capability, format-v2 mapping, Parquet/ORC pruning, profiles, and their tests. The new contracts are mostly clear, but the mapper and TableReader do not carry the same materialization barrier through every early-pruning consumer.
- Concurrency and thread safety: No new threads, shared cross-reader mutable state, locks, or lock ordering are introduced. Request, pruning, and profile state remain reader/scanner scoped.
- Lifecycle and static initialization: Decoded Bloom ownership is bounded to one physical-leaf group at a time, request refresh occurs at reader boundaries, and eager/deferred counters have a single publication path. No new cross-TU static initialization or circular ownership issue was found.
- Configuration: No configuration item is added, so dynamic-reload behavior is not applicable.
- Compatibility: No storage, wire, function-symbol, or FE-BE protocol format changes are introduced. The internal
FileScanRequestdefault preserves conservative behavior for callers that do not set the new prefix explicitly. - Parallel paths: Parquet Statistics, Dictionary, Bloom, and PageIndex pruning and ORC SARG construction all consume the bounded metadata prefix. Parquet and ORC row filtering were also traced. The remaining constant/default pruning gap is table-side and therefore bypasses those format-specific fences.
- Conditions, errors, and nullability: The selected-chain nullability condition is insufficient when ARRAY access forces a full descendant projection. Separately, expression-totality is not enough to establish that mapping/materialization cannot fail later; both accepted comments provide concrete failure paths. No unchecked new
Status, exception boundary, or const/nullable column-shape issue was found outside those paths. - Test coverage: The diff adds substantial BE unit coverage for extraction, mapping, safe prefixes, Parquet Bloom behavior, ORC SARG behavior, counters, and production accessor reachability. Missing negative coverage is precisely the unreferenced required sibling and the rejected accessor followed by a false missing/default-column predicate.
- Test results: This review runner explicitly forbids builds and test execution, so no tests were run here. No regression result files are changed; the reviewed unit-test expectations are internally consistent but do not prove the two missing cases.
- Observability: Bloom attempts, successes, conservative fallbacks, corrupt rejections, and eager/deferred publication are now represented. No new observability issue remains beyond already-known review threads.
- Transactions, persistence, and writes: The PR changes read-side predicate/pruning behavior only; transactionality, EditLog replay, data-write atomicity, crash recovery, and MoW visibility are not applicable.
- Transmitted variables: No new FE-BE or persisted variable is added.
- Performance and memory: Physical-leaf grouping shares repeated probes and keeps one decoded Bloom payload live. The remaining first-probe ordering concern is already covered by existing thread r3714634458; no duplicate is submitted.
- Other reviewed areas: Direct primitive ARRAY equality/IN reaches localization and Parquet leaf resolution; negative ARRAY indices remain a conservative optimization gap; fallback/profile, refresh, condition-cache, and aggregate-pushdown interactions yielded no additional distinct issue.
User focus: no additional user-provided review focus was supplied, so the complete PR was reviewed without a narrower focus.
Review completion: complete and converged. All three final-round reviewers returned NO_NEW_VALUABLE_FINDINGS for the same frozen two-comment set; every other candidate was independently dismissed or deduplicated against the 12 existing inline threads.
| // Accessor results become nullable for missing ARRAY indices and NULL parents. Compare the | ||
| // file child with the declared table child instead, or that execution-only wrapper can | ||
| // hide a nullable-file-to-required-table contract violation before alignment reports it. | ||
| if (!can_filter_before_table_nullability_alignment(resolved.file_child_types[idx], |
There was a problem hiding this comment.
[P1] Validate every child in an ARRAY's full projection
These checks cover only the selected accessor chain, but ARRAY indexing requests project_all_children, so TableReader later aligns unreferenced siblings too. For table items ARRAY<STRUCT<a Nullable(INT), b INT NOT NULL>>, file items ARRAY<STRUCT<a Nullable(INT), b Nullable(INT)>>, rows [{a=0,b=NULL}] and [{a=20,b=1}], and items[1].a > 5, this guard sees compatible ARRAY/element/a types and localizes the filter. The file reader then discards the first row before required sibling b is aligned, changing the schema error into successful output. Please validate every projected descendant before localizing a full ARRAY projection (or keep the filter table-level), and add an end-to-end sibling-nullability regression.
| "not_in", | ||
| "is_null_pred", | ||
| "is_not_null_pred", | ||
| "element_at", |
There was a problem hiding this comment.
[P1] Fence constant pruning after a rejected accessor
Adding element_at here also keeps the table-side constant-pruning prefix open. For table items ARRAY<STRUCT<a INT NOT NULL>>, z INT DEFAULT 0, a file containing only items ARRAY<STRUCT<a Nullable(INT)>>, predicates [items[1].a > 10, z = 7], and a row with a=NULL, the mapper correctly leaves the first predicate above TableReader so required-child alignment can report the schema error. But missing z is a constant/default filter entry, so _evaluate_constant_filters() skips the nonconstant first filter, evaluates z=7 as false, and closes the split before any row reaches alignment. Before this accessor was classified safe, the first conjunct closed that prefix. Please propagate a rejected mapping/materialization barrier into constant pruning too, and add a missing/default-column regression for this order.
Issue Number: None Related PR: apache#66423 Problem Summary: Production nested accessors and null-safe equality were excluded from the metadata-pruning safe prefix, ARRAY accessor result nullability could hide a required table child, physical-leaf grouping could reorder large Bloom reads, and unavailable Bloom payloads were not observable. Classify only total accessor shapes, retain declared table child types during localization, preserve first-probe order with one decoded payload at a time, and publish separate attempt, success, fallback, and corruption counters. Make nested Parquet Bloom pruning reachable for production accessors while preserving schema validation, probe order, and observable conservative fallbacks. - Test: Unit Test - 6 focused BE unit tests covering production TableReader integration, nested accessor safety, ARRAY child nullability, Bloom fallback counters, shared reads, and first-probe order - Behavior changed: Yes. Eligible nested predicates can use Parquet Blooms without hiding required-child errors or reordering physical reads. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66423 Problem Summary: ORC built search arguments from every localized conjunct even when an earlier expression was unsafe for metadata pruning. Because ORC can omit unsupported expressions, a later pushdown predicate could prune every stripe and suppress a required row-level error. Limit SARG compilation to the request metadata-safe prefix so stripe pruning cannot cross the error-preserving barrier. ### Release note Preserve row-level error behavior when ORC stripe pruning is enabled. ### Check List (For Author) - Test: Unit Test - Focused ORC safe-prefix and stripe-pruning BE unit tests - Behavior changed: Yes. ORC SARG pruning now stops at the metadata-safe conjunct prefix. - Does this need documentation: No
Issue Number: None Related PR: apache#66423 Problem Summary: Production nested accessors and null-safe equality were excluded from the metadata-pruning safe prefix, ARRAY accessor result nullability could hide a required table child, physical-leaf grouping could reorder large Bloom reads, and unavailable Bloom payloads were not observable. Classify only total accessor shapes, retain declared table child types during localization, preserve first-probe order with one decoded payload at a time, and publish separate attempt, success, fallback, and corruption counters. Make nested Parquet Bloom pruning reachable for production accessors while preserving schema validation, probe order, and observable conservative fallbacks. - Test: Unit Test - 6 focused BE unit tests covering production TableReader integration, nested accessor safety, ARRAY child nullability, Bloom fallback counters, shared reads, and first-probe order - Behavior changed: Yes. Eligible nested predicates can use Parquet Blooms without hiding required-child errors or reordering physical reads. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66423 Problem Summary: ORC built search arguments from every localized conjunct even when an earlier expression was unsafe for metadata pruning. Because ORC can omit unsupported expressions, a later pushdown predicate could prune every stripe and suppress a required row-level error. Limit SARG compilation to the request metadata-safe prefix so stripe pruning cannot cross the error-preserving barrier. ### Release note Preserve row-level error behavior when ORC stripe pruning is enabled. ### Check List (For Author) - Test: Unit Test - Focused ORC safe-prefix and stripe-pruning BE unit tests - Behavior changed: Yes. ORC SARG pruning now stops at the metadata-safe conjunct prefix. - Does this need documentation: No
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: File Scanner V2 only used native Parquet Bloom filters for top-level primitive slots, and null-safe equality did not advertise Bloom evaluation. Nested STRUCT and LIST predicates also require stricter ownership and equality handling because one expression tree can mention multiple leaves, multiple predicates can share one physical leaf, and Parquet hashes physical floating-point bytes.
This change:
Release note
Enable native Parquet Bloom pruning for STRUCT and LIST leaves and non-null null-safe equality, with conservative floating-point semantics and shared per-leaf Bloom reads.
Check List (For Author)