[enhancement](scan) Optimize Parquet V2 direct predicate filtering - #66360
Conversation
|
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.
Request changes.
I found two PR-introduced issues in the late runtime-filter refresh path:
- [P1] A COUNT(*) carrier that is still represented by synthesized placeholder defaults can be promoted to a late predicate while the active Parquet Row Group continues returning defaults; Scanner immediately evaluates the new RF and can silently drop or retain the wrong rows.
- [P2] Activating the new request retains footer-planning and adaptive-prefetch state derived from the old predicate snapshot, losing cheap pruning and potentially warming wide lazy output before a selective RF is sampled.
Critical checkpoint summary:
- Runtime-filter lifecycle and COUNT(*) semantics: blocked by the placeholder transition above.
- Parquet Row Group pruning and adaptive lazy-read state: needs request versioning or equivalent invalidation at activation.
- Direct OR/NULL behavior, fused SelectionVector compaction, dictionary-reader reuse, and MergeRange ordering: no additional PR-introduced defect after focused recheck.
- Mapping/layout compatibility, condition-cache handling, JNI/Hudi/Paimon forwarding, and expression ownership: no additional issue found.
- Coverage: the changed tests exercise request queuing/activation, but not mid-Row-Group COUNT(*) result correctness or request-versioned multi-Row-Group I/O.
- User focus: no additional focus was supplied; the full 39-file diff was reviewed.
- Validation: static review only as required; no builds or tests were run.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
db8ebab to
4b5c6c4
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes.
The existing [P2] blocker remains valid and is not duplicated here: refreshed requests retain footer-planning and adaptive-prefetch state derived from the older predicate snapshot (comment 3698725295).
The live [P1] COUNT() comment 3698725294 is also not duplicated, but the final production-plan audit could not prove its required combination of storage-level COUNT() and a late runtime filter targeting the arbitrary retained scan carrier, so this review does not rely on it.
This review adds one distinct [P2]: late-runtime-filter request rebuilding and concrete-reader validation execute outside the owning TableReader/FileReader profile scopes, so refresh latency is visible only at the FileScannerV2 level.
Critical checkpoint summary:
- Parquet request activation, pruning, and adaptive lazy-read state remain blocked by comment 3698725295.
- Profile ownership for the new refresh lifecycle is covered by the new inline comment.
- COUNT(*) late-filter mechanisms were examined but dismissed from this review because the current FE cannot form the required storage-COUNT-plus-scan-RF plan.
- SelectionVector compaction, mapping/layout compatibility, condition-cache behavior, request ownership, and JNI/Hudi/Paimon forwarding exposed no additional production defect after reachability and duplicate checks.
- Coverage still lacks request-versioned multi-row-group I/O assertions and an open-reader refresh profile assertion.
- User focus: no additional focus was supplied; the full authoritative 26-file diff was reviewed.
- Validation: static review only as required; no builds or tests were run.
- Workflow limitation: the bundle-required repository code-review SKILL.md was absent after hidden/no-ignore repository search, so it could not be loaded.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/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. |
eca8f5e to
4445b7e
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
I found three new issues: the implicit-selection reset repeatedly initializes the full owned index scratch on every filtered batch (matching the PR's measured dense/successive compaction regressions), the changed-dictionary regression branch preserves the same dictionary-ID bitmap and cannot catch stale reuse, and the mandatory benchmark guide still contains an obsolete conflicting registration count.
Critical checkpoint summary:
-
Late-runtime-filter lifecycle: active/pending request ownership, row-group/pending-output activation, fixed-layout fallback, and cache snapshot handling are otherwise sound. The existing COUNT(*) carrier, stale planning/adaptive-state, and refresh-profile threads remain the authoritative coverage for those issues.
-
Schema/layer boundaries: refreshed mapping stays file-local, incompatible scalar/nested/hidden/delete/virtual layouts fall back conservatively, and original mapping state remains the final-materialization owner.
-
JNI and hybrid readers: refreshed contexts are initialized at the executable lifecycle boundary and the active child receives the new snapshot; later split switches receive the current per-split snapshot.
-
Selection correctness: identity, row-index/compact-index coordinates, bounds, and cursor alignment remain correct; the new finding is the repeated hot-path initialization cost.
-
Tests and benchmarks: no builds or tests were run because this review runner explicitly forbids them. The PR supplies benchmark evidence, but the two inline coverage/record issues must be corrected. No repository code-review SKILL.md was present, so the required AGENTS.md files and mandatory FileScannerV2/Parquet guides were used directly.
User focus: no additional review focus was provided; the whole PR was reviewed.
Keep identity selections implicit, refresh late predicates at row-group boundaries, and reuse dictionary predicate bitmaps within a query.
4445b7e to
4eeb65b
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes.
I found one new issue in this head: the new ParquetSelection benchmark validates only survivor count, so an implementation that keeps the wrong source-row identities can still publish accepted timings. The inline comment has the concrete failure mode and requested oracle.
Five previously raised issues remain visible in the current head, so I did not duplicate them inline:
- [P1] The COUNT(*) placeholder can hide real carrier values before a late runtime-filter request activates.
- [P2] Late-filter activation retains stale unopened Row Group planning and adaptive predicate state.
- [P2] Runtime-filter refresh work is missing the owning TableReader and concrete-reader profile scopes.
- [P2] SelectionVector clears and value-initializes its retained scratch on every filtered batch.
- [P2] The dictionary-replacement test keeps the same matching entry-ID bitmap and cannot detect stale reuse.
The earlier benchmark-guide registration-count inconsistency is resolved in this head.
Critical checkpoints:
- Correctness and row identity: active/pending request switching is safe for ordinary projected columns, pending output, EOF, repeated refreshes, and hybrid native/JNI forwarding. The known COUNT(*) carrier exception remains blocking.
- Lifecycle and cache safety: no new cleanup, ownership, ConditionCache, or split-transition defect was substantiated; the pre-open stale-cache-key hypothesis was dismissed after caller and timing tracing.
- Performance and observability: the existing stale planning/adaptive state, refresh profile attribution, and per-batch selection scratch regressions remain unresolved.
- Compatibility and layer boundaries: fixed file-local layouts, mapper finalization, and Hudi/Paimon/JNI forwarding did not expose a new compatibility or ownership violation.
- Tests and benchmarks: the new exact-survivor oracle issue and existing dictionary-snapshot oracle gap remain; the registration counts are now internally consistent.
No additional user-provided review focus was supplied, so I reviewed the full PR. This was a static review only; builds and tests were not run per the runner instructions.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes.
I found two new P2 benchmark-validation defects in this head. Neither duplicates an existing thread:
- The Direct-IN lifecycle benchmark times clone/prepare/open without ever proving that shared and rematerialized clones preserve the exact predicate truth set; its focused unit test's no-match assertion also passes for an empty or truncated set.
- The SelectionVector identity case does not expose or validate the state written by
resize(), so the resize can be a no-op or be eliminated while the default vector's implicit identity still passes every current check.
The inline comments give the concrete failure modes and deterministic oracle requirements.
Critical checkpoints:
- Correctness and row identity: late runtime-filter evaluation, COUNT(*) carriers, pending predicate output, repeated refreshes, fixed local layouts, and SelectionVector coordinate/generation behavior did not expose another defect in the current head.
- Lifecycle and cache safety: row-group-boundary activation, unopened-plan replanning, condition-cache behavior, adaptive-state reset, expression ownership, and close/error paths are internally consistent after the current fixes.
- Performance and observability: refresh work is attributed through the TableReader/concrete-reader hierarchy and retained selection scratch is covered; the two new benchmark-oracle gaps still make part of the claimed performance evidence unreliable.
- Compatibility and layer boundaries: shared Direct-IN pruning state remains immutable and type-stable on reachable clone/localization paths, while JNI/Hudi/Paimon forwarding preserves one independently owned prepared snapshot per active child.
- Tests and benchmarks: registration counts and mandatory documentation are consistent, and the earlier dictionary and exact-survivor oracle gaps are fixed. The remaining lifecycle and identity cases need the independent validations described inline.
I did not duplicate the earlier threads; their author follow-ups and current-head changes were included in this review. No additional user-provided focus was supplied, so I reviewed the complete PR. This was a static review only; builds and tests were not run under the runner contract. The prompt-required repository code-review skill was not present in this checkout, so I followed the review prompt, both required AGENTS.md files, and the three FileScannerV2 design/review documents directly.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
changed the title
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
3 participants
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.VDirectInPredicatepruning materialization across split-local expression clonesTest
./run-be-ut.sh --run --filter='FileScannerV2Test.*:*Parquet*:*TableReaderTest.*:Hudi*ReaderTest.*:Paimon*ReaderTest.*:SelectionVectorTest.*:DictionaryFilterCostTest.*' -j48git diff --checkSelection-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 -mavx2on 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.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
_ownedavoids 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_openisolates 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.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.
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.