[Improvement](file scan) Keep File Scanner V2 predicate pruning enabled - #66357
Open
Gabriel39 wants to merge 3 commits into
Open
[Improvement](file scan) Keep File Scanner V2 predicate pruning enabled#66357Gabriel39 wants to merge 3 commits into
Gabriel39 wants to merge 3 commits into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: FileScannerV2 inherited legacy session gates that could disable partition predicate pruning and Parquet expression ZoneMap pruning. ### Release note FileScannerV2 always applies partition predicate pruning and Parquet expression ZoneMap pruning. ### Check List (For Author) - Test: Unit Test\n- Behavior changed: Yes. FileScannerV2 ignores the two legacy pruning switches.\n- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28642 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes for one P1 correctness issue and two P2 issues.
Accepted findings:
- P1: unavoidable Parquet metadata pruning can skip an earlier error-preserving conjunct and prune on a later ZoneMap-safe conjunct, changing an error into an empty result.
- P2: the scanner now performs a second full conjunct-tree clone per split even when there are no partition values and the sole consumer immediately returns.
- P2: the two still-forwarded session switches acquire path- and backend-version-dependent semantics without a durable FE/Thrift contract update or a stable V2 differential/escape path.
Critical checkpoint conclusions:
- Goal and proof: the production hunks do make V2 partition and expression ZoneMap pruning ignore the two false gates. The added unit tests prove scanner vector plumbing and helper-level footer/Page pruning under
false, but they do not prove end-to-end semantic equivalence; the P1 error-order counterexample means the goal is not safely accomplished yet. - Scope and clarity: the production edit is narrow and localized. The required fixes remain focused: apply an unsafe-expression prefix fence to both footer and Page metadata evaluation, avoid ineligible empty-map clone work, and align the exposed option contract/test controls.
- Concurrency and locking: no new thread, lock, atomic, or shared mutable state is introduced. Late runtime-filter snapshots are lock-protected and refreshed before the scanner turn; split preparation is serial. No deadlock or lock-order issue was found.
- Lifecycle and static initialization: split-local expression contexts retain RAII close behavior and split state is reset before reuse. No static/global initialization dependency, ownership cycle, leak, or double advancement was found.
- Configuration and compatibility: no new configuration field, symbol, serialization, storage-format, or wire-layout change is added. The existing forwarded fields remain wire-compatible, but V1/older-BE and new V2 readers interpret the same explicit
falsevalue differently; this behavioral compatibility issue is reported inline. - Parallel paths and conditions: V1 remains gated intentionally. All reviewed V2 TableReader variants delegate partition evaluation to the same base path. Footer and Page-index metadata paths both flow through the changed capability decision and both need the same error-order barrier. The new comments explain intent but do not update the public option contract.
- Tests and expected results: the two new unit tests correctly assert their narrow helper/plumbing behavior, and no generated result file is involved. Coverage is missing for the reachable earlier-error/later-prunable case, the no-partition clone path, and a genuine V2 disabled-versus-enabled result baseline. No build or tests were run because this review task explicitly forbids them.
- Error handling: changed
Statusvalues remain propagated and malformed/unsupported metadata generally retains candidates. The substantive error-handling defect is semantic: metadata pruning can suppress an earlierassert_trueerror. - Memory safety and BE nullability: no new raw ownership, untracked large buffer, nullable-column cast, or const/nullable shape handling is introduced. Conservative NULL, NaN, order, type, timezone, and malformed-index fallbacks remain intact.
- Data correctness: no version, MoW, write, or persistence path is touched. The P1 finding is a read-result correctness/error-observability regression; no additional wrong-row case survived review.
- Observability: existing scanner prepare/prune and Parquet statistics/Page-index counters and timers cover these paths. No new metric or log is required beyond retaining an operable and testable control contract.
- Transactions, persistence, and writes: not applicable; this is read-only scan planning and adds no EditLog, transaction, atomicity, or crash-recovery behavior.
- FE-BE propagation: no new variable is added. The issue is that the two existing variables continue to be forwarded while their meaning diverges across reader paths and versions.
- Performance: the accepted scanner finding is concrete redundant per-file CPU/allocation work for non-partitioned many-file scans. No other unbounded I/O, memory, or asymptotic regression was substantiated.
- Other issues and user focus: no additional review focus was supplied, so the whole PR was reviewed. The review converged in Round 2: every normal and risk-focused reviewer returned
NO_NEW_VALUABLE_FINDINGSfor this same frozen three-comment set, and every other candidate was dismissed with evidence or merged as a duplicate.
| if (!expr_zonemap::is_expr_zonemap_filter_enabled(runtime_state)) { | ||
| return false; | ||
| } | ||
| bool has_expr_zonemap_filter(const format::FileScanRequest& request, const RuntimeState*) { |
Contributor
There was a problem hiding this comment.
[P1] Preserve error order before making metadata pruning unavoidable
request.conjuncts can contain an earlier localized error-preserving expression (for example assert_true(y != 0, 'bad')) followed by a ZoneMap-safe x > 100. With a PLAIN row group where y=0 and max(x)=2, and with cost-based conjunct reordering disabled, the old enable_expr_zonemap_filter=false path reaches row evaluation and raises bad; after this change the metadata evaluator skips the unsupported first conjunct, prunes on the later comparison, and returns no rows. The row scheduler and partition pruner already treat such expressions as ordering barriers. Please apply the same safe-prefix fence to both footer and page-index metadata evaluation before ignoring this switch, and add an error-preservation test.
| } | ||
| // FileScannerV2 owns its complete pruning pipeline, so safe partition predicates must not | ||
| // inherit the legacy scanner's session gate. | ||
| RETURN_IF_ERROR(_build_table_conjuncts(&partition_prune_conjuncts)); |
Contributor
There was a problem hiding this comment.
[P2] Skip pruning-clone work when this split has no partition values
The scanner already has the generated partition_values map here, but this now deep-clones and rewrites every conjunct even when that map is empty. TableReader::_evaluate_partition_prune_conjuncts() immediately returns for an empty map, and no other reader consumes these clones, so a non-partitioned many-file scan pays a second expression-tree allocation/traversal per file with no possible pruning benefit. Please keep pruning unconditional for eligible V2 splits while guarding this clone on a non-empty/eligible partition-value set, and cover the no-partition path in the unit test.
| RETURN_IF_ERROR(_build_table_conjuncts(&partition_prune_conjuncts)); | ||
| } | ||
| // FileScannerV2 owns its complete pruning pipeline, so safe partition predicates must not | ||
| // inherit the legacy scanner's session gate. |
Contributor
There was a problem hiding this comment.
[P2] Align the public switch contract before treating it as legacy
enable_runtime_filter_partition_prune and enable_expr_zonemap_filter are still forwarded session controls; FE/Thrift describe the latter as controlling scanner/BE-reader ZoneMap pruning, and V1/older-BE paths still honor both. This makes the meaning of false depend on scanner selection or backend version, while existing Iceberg/Paimon suites that use false as the non-pruned correctness baseline now exercise the same default-V2 pruning path in both halves. Please either keep honoring these controls, or update the durable FE/Thrift/user-facing contract and provide an explicit V2 per-feature test/escape path before removing those differential baselines; the PR release note and local legacy comment do not update the exposed session semantics.
Contributor
TPC-DS: Total hot run time: 169525 ms |
Contributor
ClickBench: Total hot run time: 24.04 s |
Gabriel39
marked this pull request as ready for review
August 2, 2026 06:39
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
There was a problem hiding this comment.
Review status: the current head still cannot be approved because the existing P1 metadata error-order thread remains unresolved. I am not adding a duplicate inline comment; the prior discussion_r3698036618 already identifies the exact changed line, reachable input, and required fix. The existing clone-cost thread also still covers the narrower nonempty-but-predicate-ineligible partition case. No new unduplicated inline issue was found.
Current-head assessment:
- The empty-partition-map guard and its unit assertion address the original unconditional clone work for nonpartitioned splits.
- The FE and Thrift descriptions now make the scanner-dependent semantics explicit while preserving the existing field IDs, defaults, and forwarding, so no distinct wire or rolling-upgrade defect remains.
parquet_statistics.cppstill deliberately skips the required safe-prefix fix: an earlier localized deterministic-but-error-preserving conjunct can be ignored by metadata evaluation, allowing a later ZoneMap-capable conjunct to prune a Row Group or page range and suppress the earlier row-level error. Both footer Statistics and native ColumnIndex/OffsetIndex paths are affected, and the changed positive test does not cover this negative case.- A partitioned split whose map is nonempty but whose safe prefix has no partition-only predicate still builds a second complete cloned/re-written conjunct set that
TableReaderdiscards. This is the same location, outcome, and requested eligibility guard already covered bydiscussion_r3698036620, so it is not reposted.
Critical checkpoint conclusions:
- Goal and proof: the production changes do make FileScannerV2 ignore the two legacy gates, and the new unit tests prove the positive plumbing for partition, footer, and page pruning under
false. The goal is not safely complete while the existing error-preservation counterexample remains reachable and untested. - Scope and clarity: the changed surface is small and focused. The acknowledged TODO at the core metadata gate should be resolved in this PR rather than shipping a known semantic hole.
- Concurrency and locking: no new thread, lock, atomic, or shared mutable state is introduced. Scheduler late-RuntimeFilter refresh occurs before scanner work, and split preparation remains serial per scanner; no lock-order or deadlock issue was found.
- Lifecycle and static initialization: partition pruning uses independent per-split clones,
VExprContextdestructor-driven close, and common TableReader split reset/EOF handling. No static-initialization dependency, ownership cycle, leak, or split-state escape was found. - Configuration and compatibility: no new option or wire field is added. Existing Thrift IDs 150 and 224, optional defaults, and FE forwarding are unchanged; V1/older BEs continue honoring
false, while the new V2 behavior is now documented. No symbol, storage-format, persistence, or protocol-layout incompatibility was found. - Parallel paths and conditions: every reviewed V2 TableReader path reaches the common partition safe-prefix logic, which correctly stops at nondeterministic/error-preserving expressions and requires partition-only identities. Parquet footer and page-index pruning both share the changed gate and both still need the corresponding original-order safety fence.
- Tests and expected results: the changed BE tests are deterministic and involve no generated result file. Existing TableReader tests cover real partition pruning, unsafe-prefix retention, refreshed conjunct replacement, and reader reuse. Missing coverage is the already-reported earlier-error/later-ZoneMap case at both metadata levels and the nonempty-but-ineligible clone case. No build or tests were run because this review environment explicitly forbids them.
- Error handling and data correctness: changed
Statusvalues remain propagated, and malformed/unsupported metadata generally retains candidates. The blocking defect is semantic error suppression, which can change a failing query into an empty result. No version, MoW, transaction, EditLog, write, or crash-recovery path is touched. - Memory safety and nullability: no new raw ownership, untracked large allocation, nullable cast, or const/nullable column-shape handling is introduced.
- Observability: existing FileScannerV2 prepare/prune and Parquet Statistics/PageIndex counters and timers cover the changed work; no new log or metric is required.
- Performance: the empty-map fix removes the concrete nonpartitioned per-file cloning regression. Remaining ineligible clone work is already tracked by the existing P2 discussion; no other unbounded I/O, memory, or asymptotic regression was substantiated.
- User focus: no additional review focus was supplied, so the full PR was reviewed.
Two independent normal reviews, one dedicated risk review, and the main-agent sweep found no new valuable issue beyond the existing threads. Please resolve the P1 safe-prefix/error-preservation issue (and its negative test) before approval.
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28977 ms |
16 tasks
Contributor
TPC-DS: Total hot run time: 169582 ms |
Contributor
ClickBench: Total hot run time: 23.87 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
File Scanner V2 inherited the legacy session gates for runtime-filter partition pruning and Parquet expression ZoneMap pruning. Disabling either switch therefore also disabled the corresponding V2 pruning path, even though V2 owns an independent pruning pipeline.
This change keeps both pruning paths enabled for File Scanner V2 regardless of the legacy session switch values. File Scanner V1 behavior is unchanged.
Release note
File Scanner V2 always applies partition predicate pruning and Parquet expression ZoneMap pruning.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)