Uh oh!
There was an error while loading. Please reload this page.
[enhancement](scan) Optimize Parquet V2 predicate filtering - #66261
[enhancement](scan) Optimize Parquet V2 predicate filtering#66261Gabriel39 wants to merge 12 commits into
Conversation
Keep identity selections implicit, refresh late predicates at row-group boundaries, and reuse dictionary predicate bitmaps within a query.
hello-stephen
commented
Jul 30, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Gabriel39
commented
Jul 30, 2026
run buildall |
Gabriel39
commented
Jul 30, 2026
/review |
There was a problem hiding this comment.
Review completion: complete static review of all 21 changed files at 2640d4cf399ec8df9ee8cccf868644f935acd380.
Blocking finding:
- The query-wide dictionary predicate cache is only tested as a standalone map. The TopN counter test captures a null cache pointer, so the production key, miss→hit reuse, output equivalence, and mutable-TopN bypass remain untested.
Critical checkpoints:
- Late runtime-filter boundary: the newest conjunct remains a scanner residual while pending output and the active row group drain, and request-derived Parquet state switches together at the boundary; no correctness defect found.
- Dictionary-cache correctness/lifetime: key composition, residual handling, TopN detection, query lifetime, concurrency, and memory bound were reviewed; no implementation defect was substantiated beyond the blocking coverage gap.
- SelectionVector: implicit identity, coordinate-specific compaction, generation/verification caching, and all production callers were checked; no issue found.
User focus: no additional focus was provided, so the complete PR was reviewed without a narrower focus.
Validation: static review only. No builds or tests were run because the authoritative review prompt forbids them. No repository-local code-review SKILL.md exists in this checkout; the required AGENTS.md and format-v2/Parquet review guides were applied directly.
Uh oh!
There was an error while loading. Please reload this page.
Gabriel39
commented
Jul 30, 2026
run buildall |
Gabriel39
commented
Jul 30, 2026
/review |
There was a problem hiding this comment.
Found two late-runtime-filter lifecycle issues that should be fixed before merging: refreshed mapper projections can be executed without preparation, and Paimon/Hudi hybrid readers do not forward refreshes to their active child. I did not duplicate the existing dictionary-cache production-coverage thread.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Gabriel39
commented
Jul 30, 2026
run buildall |
Gabriel39
commented
Jul 30, 2026
/review |
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Gabriel39
commented
Jul 30, 2026
run buildall |
Gabriel39
commented
Jul 30, 2026
/review |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65998 Problem Summary: Cost sorting could move a late runtime filter ahead of an older unsafe predicate when FileScannerV2 rebuilt partition-pruning predicates for the next split. TableReader also prepared and opened Scanner-owned predicate suffixes even though it retained them only for pruning analysis. Preserve append identity separately from the cost-sorted Scanner snapshot, and clone Scanner-owned TableReader predicates without creating duplicate execution state. ### Release note None ### Check List (For Author) - Test: Unit Test - FileScannerV2Test.*, TableReaderTest.*, and ScannerLateArrivalRfTest.* - Behavior changed: Yes. Late runtime filters retain predicate ordering barriers, and Scanner-owned predicates have a single execution-state owner. - Does this need documentation: No
Gabriel39
commented
Jul 30, 2026
run buildall |
Gabriel39
commented
Jul 30, 2026
/review |
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Pass post-filter slot liveness from FE while keeping exact predicate localization a split-local BE decision. Preserve residual and unsafe predicate ordering so payloads are discarded only when all referencing predicates execute in the file reader.
Gabriel39
commented
Jul 30, 2026
run buildall |
Gabriel39
commented
Jul 30, 2026
/review |
hello-stephen
commented
Jul 30, 2026
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Found two P1 correctness issues in predicate-only value lifetime and filter ownership:
- A late runtime filter is retained by TableReader even when the active/refreshed Parquet request returns only default placeholders for its predicate-only slot, so valid rows can be filtered out.
- A dictionary-filtered predicate-only value can be discarded before a later multi-column predicate or Iceberg equality-delete stage consumes it, producing incorrect filtering or deleted rows.
I reviewed all 44 changed paths and the mandatory format-v2 guides, covering predicate localization and residual ownership across every reader family, late-RF ordering and hybrid forwarding, Parquet request activation/pending output/condition-cache boundaries, FE liveness and legacy/virtual dependencies, SelectionVector/progress accounting, and regression coverage. The remaining mechanisms produced no additional findings.
No additional user focus was provided, so this was a full-PR review. Static review only: the authoritative prompt prohibits builds/tests. The repository-local code-review SKILL.md named by the prompt is absent from this checkout.
| if (_current_task != nullptr && conjunct_index < owned_count) { | ||
| // Keep late predicates residual for this split even after a refreshed request is | ||
| // queued: the physical reader may not activate it until the next row-group boundary. | ||
| _remaining_conjuncts.push_back(std::move(conjunct)); |
There was a problem hiding this comment.
[P1] Preserve late-filter values until ownership transfers
This appends the late RF to TableReader residuals immediately, but the active Parquet request may already have marked its slot predicate-only and replaced its values with defaults. The new residual then filters those placeholders instead of the real values; after the next row-group boundary, the refreshed request can still be predicate-only while this residual remains, so valid rows continue to be dropped. Please keep every possible late-RF slot materialized, or defer this residual until the reader acknowledges activation and retire it atomically when file-level ownership begins.
| // `filter_key` while evaluating the predicate, but not afterwards. Discard its payload | ||
| // only when every predicate referencing the physical column was localized exactly for this | ||
| // split; schema evolution or a TableReader residual must retain the value. | ||
| if (referenced_by_filter && referenced_only_by_localized_filters && |
There was a problem hiding this comment.
[P1] Keep values needed by later file-local stages
referenced_only_by_localized_filters only says each table filter moved into the file request; it does not mean this column has no later consumer. With dictionary-encoded Parquet, id > 1 can run as the single-column dictionary stage and, because id is predicate-only here, replace surviving IDs with defaults. A later localized id + other > 10 stage then evaluates those defaults. Iceberg has the same problem because customize_file_scan_request() appends equality-delete conjuncts only after this flag is computed, allowing deleted rows to survive when their key is defaulted. Please derive predicate-only status from all request consumers/stages, including delete conjuncts, or defer payload destruction until the last such consumer has run.
Restore independent predicate localization and add coverage showing that a predicate localized for one split returns to TableReader when the next split cannot localize it. Residual predicates on unrelated physical columns do not prevent predicate-only payload discard.
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
There was a problem hiding this comment.
Found two new blocking issues on current head 7535c807247389a7197f5d9421d292d157018245:
- A later safe predicate can cross an earlier unsafe/error-preserving predicate during localization or early pruning and suppress its required evaluation.
- JNI and refreshed Parquet can consume batches rejected by a newly transferred late runtime filter without exposing materialized progress to Scanner's cooperative row/byte budget.
I did not duplicate the two existing P1 threads covering late-filter placeholder values and later file-local consumers; both remain applicable to this head.
I also traced exact predicate ownership across every reader family, hybrid native/JNI refresh propagation, FE post-filter liveness and old-FE fallback, SelectionVector coordinate changes, and adaptive materialized-input accounting. No additional distinct issue survived reachability and duplicate checks. There was no extra user-provided review focus.
Validation was static only, as required by the review prompt; no builds or tests were run.
| if (in_safe_prefix && !_is_safe_to_pre_execute(conjunct)) { | ||
| in_safe_prefix = false; | ||
| } | ||
| const bool can_localize = is_safe_to_pre_execute(conjunct); |
There was a problem hiding this comment.
[P1] Keep unsafe conjuncts as ordering barriers
This marks each later conjunct localizable independently, so an ordered pair such as an earlier assert_true/failing cast/stateful predicate followed by a safe predicate can execute in the opposite order: FileReader or split/constant pruning evaluates the later predicate first. If it rejects the batch or split, _filter_remaining_conjuncts() never executes the earlier unsafe predicate, turning an error or required stateful evaluation into silent empty output. is_safe_to_execute_on_selected_rows() only proves the moved predicate itself tolerates selected rows; it does not prove that crossing an earlier unsafe predicate preserves semantics. Please retain the safe-prefix barrier for localization and pruning.
| status.prepend("Orc row reader nextBatch failed. reason = "); | ||
| } | ||
| return status; | ||
| size_t FileScannerV2::_last_block_rows_read(const Block& block) const { |
There was a problem hiding this comment.
[P2] Preserve filtered-reader progress before budgeting
A late RF is now moved below Scanner for an active split. JniTableReader::get_block() loops when it rejects a Java batch, and once Parquet activates a queued refreshed request at a row-group boundary, ParquetScanScheduler::read_next_batch() likewise loops and widens zero-survivor predicate batches. Neither path returns or publishes the rejected batch's pre-filter shape, so this fallback sees only a survivor or EOF and Scanner's row/byte budget can be bypassed while the remaining split is drained. Please make each predicate-owning reader publish and yield bounded pre-filter progress, including zero-survivor batches.
hello-stephen
commented
Jul 31, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jul 31, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jul 31, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jul 31, 2026
FE Regression Coverage ReportIncrement line coverage |
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Gabriel39
commented
Jul 31, 2026
run buildall |
Gabriel39
commented
Jul 31, 2026
/review |
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Review status: changes requested on current head 81a5187e37ab46628f17d6b768ef27139745bdc6.
I am adding no new inline comments in this pass: every substantiated current-head defect is already covered by a live thread, so another inline would be a duplicate. These existing blockers remain applicable:
- P1: keep unsafe conjuncts as ordering barriers. A later localized/pruning predicate can suppress required evaluation of an earlier unsafe predicate.
- P1: preserve late-filter values until ownership transfers. A delayed Parquet refresh leaves a TableReader residual able to observe predicate-only placeholders.
- P1: keep values needed by later file-local stages. Predicate-only payloads can be discarded before a later Parquet stage or Iceberg equality-delete consumer.
- P2: preserve filtered-reader progress before budgeting. JNI and refreshed Parquet can hide zero-survivor materialization from Scanner's cooperative progress budget.
Critical checkpoint conclusions:
- Architecture, lifecycle, and ownership: the Scanner/TableReader/FileReader transfer is not yet correct because of the unsafe-order and late-refresh ownership blockers above. EOF, cancellation, split cleanup, generic TableReader yielding, and Iceberg synthetic-delete yielding did not reveal another distinct defect.
- Mapping, identities, and materialization: the isolated mapper/request snapshot now addresses the older refresh-snapshot thread, and global/local identity and nested projection paths produced no additional finding. Predicate-value lifetime remains blocked by the two P1 threads above.
- Filtering, deletes, and pushdown: exact-filter paths across reader families were traced. Iceberg equality deletes and multi-stage Parquet filtering remain exposed to the existing early-payload-destruction issue; no separate delete or NULL-semantics defect was substantiated.
- Format boundaries and compatibility: Hudi/Paimon now forward snapshots and deltas to initialized and future native/JNI children. FE post-filter liveness is conservative for virtual inputs, old planners, and both optional-Thrift rolling-upgrade directions.
- Parquet correctness: request activation, merged dictionary/data ranges, adaptive dictionary fallback, direct OR/NULL behavior, selection coordinates, malformed dictionary IDs, and output alignment were rechecked without a new non-duplicate issue.
- Performance and observability: materialized-input statistics are preserved by the generic and Iceberg paths, but the existing JNI/Parquet zero-survivor budgeting thread remains unresolved.
- Tests: changed tests and production dictionary-cache coverage were inspected, but validation was static only as required by the review prompt; no builds or tests were run. Remote merged-range and end-to-end refresh coverage remain comparatively thin, though static tracing did not establish another failure.
All 57 authoritative changed paths were reviewed. The main pass, two normal full-coverage reviewers, and a separate risk-focused reviewer converged with NO_NEW_VALUABLE_FINDINGS after duplicate suppression, and the final ledger has zero unresolved candidates. No additional user-provided review focus was supplied, so the whole PR was reviewed.
Please resolve the four existing blocker threads before merge.
hello-stephen
commented
Jul 31, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jul 31, 2026
FE Regression Coverage ReportIncrement line coverage |
…66360) ## Proposed changes Backport the selected Parquet V2 direct-predicate filtering changes from #66261 to `branch-4.1`, then address the review and performance findings on this branch. - keep identity selection-vector state implicit and compact selected rows in bulk - retain the selection scratch high-water mark across scanner batches and specialize first compaction from implicit identity - refresh late runtime-filter requests at safe row-group boundaries - re-run footer-statistics pruning and reset adaptive predicate state for unopened row groups after a refresh - preserve real COUNT(*) carrier values while runtime filters are pending - initialize refreshed JNI predicates and attribute refresh work to TableReader/FileReader/Parquet profiles - preserve Hudi/Paimon child-reader predicate state - remove query-scoped dictionary-filter cache state - share immutable `VDirectInPredicate` pruning materialization across split-local expression clones - add correctness-checked selection and direct-IN lifecycle microbenchmarks ## Test - `./run-be-ut.sh --run --filter='FileScannerV2Test.*:*Parquet*:*TableReaderTest.*:Hudi*ReaderTest.*:Paimon*ReaderTest.*:SelectionVectorTest.*:DictionaryFilterCostTest.*' -j48` - 639 tests from 47 test suites passed under ASAN - targeted late-RF, COUNT(*), dictionary-snapshot, shared-IN-state, and SelectionVector tests - 19 tests from 5 test suites passed under ASAN - Release benchmark build and smoke run - expected registrations: 228 decoder, 92 kernel, 25 selection, 167 reader, and 8 expression-lifecycle cases - all 25 selection and 8 expression-lifecycle cases executed with zero benchmark errors - `git diff --check` ## Selection-vector microbenchmark The final benchmark source validates every surviving original row ID after the timed region. Base, pre-fix PR, and final binaries use the same benchmark source and Clang `-O3 -DNDEBUG -mavx2` on the same host. Each comparison uses one pinned CPU, three warmups, eight adjacent A-B-B-A quartets, and at least 0.3 seconds per invocation. The table reports median paired CPU-time ratios; negative values are improvements. | Operation | Final selectivity | Final vs pre-fix PR | Final vs branch base | |---|---:|---:|---:| | Identity initialization | 100% | -15.23% | -99.12% | | Row filter | 1% | -24.23% | -23.76% | | Row filter | 50% | -16.10% | -34.50% | | Row filter | 90% | -45.91% | -45.95% | | Row filter | 100% | -31.72% | -17.32% | | Successive filters | 1% | -33.25% | -35.79% | | Successive filters | 50% | -29.80% | -35.10% | | Successive filters | 90% | -25.27% | -25.16% | | Successive filters | 100% | -24.93% | -23.72% | All final-vs-base paired-ratio CVs are at most 5.85%. The previous 16.43%/59.94% dense row-filter regressions and 27.92%-61.05% successive-filter regressions are no longer present. Retaining `_owned` avoids repeated value initialization; the implicit-identity specialization removes the remaining source/coordinate branches from the first compaction. ## Direct-IN expression lifecycle microbenchmark `FileScannerExpr/direct_in_clone_prepare_open` isolates deep-clone, prepare, and open for an already prepared direct-IN runtime filter. Set construction and the original fragment prepare/open are outside the timed region. Shared and forced-rematerialization implementations run in the same Release binary on one pinned CPU, with 10 repetitions and at least 0.5 seconds per repetition. | IN values | Rematerialize median | Shared median | Speedup | |---:|---:|---:|---:| | 128 | 207.470 us | 1.634 us | 126.9x | | 1,024 | 1.674 ms | 1.642 us | 1,019.5x | | 8,192 | 13.514 ms | 1.672 us | 8,082.2x | | 65,536 | 108.337 ms | 1.650 us | 65,663.1x | The shared path remains approximately constant because split clones reuse the immutable, fragment-originated pruning state; the rematerialization path scales linearly with runtime-filter cardinality. ## Reader-level microbenchmark The earlier reader-level comparison used an identical Parquet-only Release benchmark binary on the same host, with one pinned CPU, warm cache, three warmups, A-B-B-A order, 10 repetitions, and at least one second per repetition. It covers nullable INT32 predicate scans with a lazy payload for PLAIN and dictionary encoding. | Encoding | Selectivity | CPU time vs base | Wall time vs base | |---|---:|---:|---:| | Dictionary | 1% | +0.44% | +0.46% | | Dictionary | 10% | +1.13% | +1.22% | | Dictionary | 50% | +1.41% | +1.51% | | Dictionary | 90% | +0.41% | +0.53% | | PLAIN | 1% | -0.65% | -0.66% | | PLAIN | 10% | +0.32% | +0.38% | | PLAIN | 50% | -1.21% | -1.18% | | PLAIN | 90% | -1.34% | -1.29% | The reader-level point estimates span -1.34% to +1.41% CPU time with mixed signs, so this benchmark did not detect a material aggregate regression. It starts at `format::parquet::ParquetReader`; it does not cover scanner scheduling or end-to-end SQL execution.
Proposed changes
Test
doris_be_testtargetgit diff --check