[fix](be) Restore FileScannerV2 residual predicate ownership - #66035
[fix](be) Restore FileScannerV2 residual predicate ownership#66035Gabriel39 wants to merge 2 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review outcome: changes requested for 3 issues (1 P1 correctness issue and 2 P2 lifecycle/performance issues).
The split-local ownership design is substantially restored and the native mapper/FileReader enforcement, materialized-work accounting, cache/delete/aggregate behavior, hybrid-reader forwarding, and synthetic Iceberg paths were traced without another reachable defect. The remaining blockers are: next-split partition pruning does not use the new append-identity ordering; TableReader opens Scanner-owned analysis-only predicates; and the base Scanner path clones/retains V2-only late-RF deltas for every scanner type.
Critical checkpoint conclusions:
- Goal and proof: The patch largely restores exact split-local residual ownership and adds broad focused unit coverage, but F3 leaves the predicate-order guarantee incomplete. The added tests do not combine cost sorting, late RF arrival, and next-split partition pruning, nor do they verify unsafe-function open/close counts for F1.
- Scope and focus: The production changes are concentrated in Scanner/FileScannerV2/TableReader/ColumnMapper and their tests. The structure is generally focused, but the new ownership boundary is applied inconsistently in F1 and the parallel pruning consumer is missed in F3.
- Concurrency and thread safety: Arrival generations and shared conjunct snapshots remain synchronized, with no missed/duplicated RF or lock-order/deadlock issue found. F2 performs clone/open work while holding
_conjuncts_lockand retains unused delta contexts in non-V2 scanners. - Lifecycle and static initialization: No cross-TU static-initialization or reference-cycle problem was found. F1 creates a second FRAGMENT_LOCAL/THREAD_LOCAL function lifecycle for Scanner-owned suffix expressions before row execution.
- Configuration: No configuration item is added. F3 is production-reachable because
enable_adjust_conjunct_order_by_costandenable_runtime_filter_partition_pruneboth default to enabled. - Compatibility: No wire, persisted metadata, storage-format, or rolling-upgrade compatibility change was identified.
- Parallel paths: Native Parquet, ORC, materialized readers, Hudi/Paimon native children, and Iceberg synthetic readers were traced. Current scan-level JNI selection keeps the identified JNI ownership/progress gaps on FileScannerV1, so they were not reported as current V2 findings. Next-split partition pruning remains inconsistent with the new row path (F3).
- Special conditions: The safe-prefix/unsafe-suffix checks are documented and mostly preserve ordering, but F3 feeds the pruning walker a cost-sorted view that bypasses that intended barrier.
- Tests and results: The changed unit tests cover localization, late-RF identity, hybrid state, pruning barriers, materialized progress, and adaptive sizing. Missing focused cases are called out in F1 and F3. Per the review-run instructions, no build or test command was run; CI currently shows formatter, CheckStyle, license, title, dependency, and secret checks passing, while BE UT (macOS) and automated code review are still pending.
- Observability: The added profile counters and Scanner residual predicate info are appropriate; no separate logging/metrics blocker was found.
- Transactions and persistence: Not applicable; this is a read-path execution change with no EditLog or persisted state.
- Data writes and atomicity: Not applicable; no data-write or transaction path is modified.
- FE/BE variable propagation: No new FE/BE session variable or thrift contract requiring propagation was added.
- Performance and memory: F2 adds redundant clone/open work under a query-shared lock and retains contexts until teardown; F1 also acquires duplicate function resources. No other substantiated CPU, memory-safety, allocator, or nullable/const-column issue was found.
- Other issues: Exact native predicate enforcement, CHAR/VARCHAR conversion guards, missing/default/partition/virtual columns, deletes, ConditionCache publication, aggregate eligibility, EOF/cancellation, and split reset/close behavior were rechecked with no further accepted finding.
User focus: no additional user-provided focus was supplied; the full 27-file patch was reviewed.
| } | ||
|
|
||
| Status FileScannerV2::_build_table_conjuncts(VExprContextSPtrs* conjuncts) const { | ||
| return _build_table_conjuncts(_conjuncts, conjuncts); |
There was a problem hiding this comment.
[P1] Preserve append order for next-split partition pruning
This default overload still clones the cost-sorted _conjuncts, whereas the new _late_arrival_rf_conjuncts identity path protects only row execution. With the default cost sort enabled, a cheap late partition RF can move ahead of an older unsafe/error-preserving predicate; the next split passes that order to the safe-prefix partition-prune walker, which can reject the split before ever reaching the older predicate and thereby suppress its error/stateful semantics. Please build pruning predicates from an append-identity-ordered view (the same owned-prefix/ordered-suffix state used for row execution) and add a next-split test covering an unsafe partition predicate followed by a late rejecting RF.
| prepared.reserve(conjuncts.size()); | ||
| for (const auto& source : conjuncts) { | ||
| VExprContextSPtr conjunct; | ||
| RETURN_IF_ERROR(_prepare_conjunct(source, &conjunct)); |
There was a problem hiding this comment.
[P2] Keep Scanner-owned suffix contexts analysis-only
_replace_conjuncts() prepares and opens every expression even when _table_reader_owned_conjunct_count stops before the unsafe suffix; append_conjuncts() likewise calls _prepare_conjunct() before applying owned_count. That suffix is executed by Scanner, not the native TableReader, so stateful/external functions get an extra FRAGMENT_LOCAL/THREAD_LOCAL lifecycle (and any open-time resources or errors) for every scanner. Please prepare/open only the owned prefix and keep the suffix as unopened analysis metadata.
| } | ||
| for (const auto& conjunct : batch) { | ||
| VExprContextSPtr cloned; | ||
| RETURN_IF_ERROR(conjunct->clone(state, cloned)); |
There was a problem hiding this comment.
[P2] Avoid cloning RF deltas for scanners that cannot consume them
This runs under _conjuncts_lock and clones every unseen RF batch for every Scanner subclass. The base Scanner then clones the complete conjunct snapshot as well and stores these delta contexts in _late_arrival_rf_conjuncts, but only FileScannerV2 ever consumes or clears that vector; FileScanner V1, OlapScanner, JdbcScanner, and MetaScanner retain the opened clones until teardown. Please make delta delivery a FileScannerV2 capability/override (or otherwise optional) and avoid doing unused clone/open work under the shared lock.
### 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
|
run buildall |
|
/review |
Gabriel39
changed the title
There was a problem hiding this comment.
Requesting changes for two JNI gaps in the otherwise coherent ownership/progress refactor.
Critical checkpoint conclusions:
- Goal and proof: Native FileScannerV2/TableReader residual ownership, exact localization fallback, and materialized-work accounting are largely coherent, but the goal is incomplete on JNI paths. The changed tests cover native and fake hybrid children; they do not exercise a real JNI late RF or a fully filtered Java batch.
- Scope and focus: The 27-file BE scanner/format-v2/test patch is broad but focused on the stated refactor. Both blockers are in parallel JNI implementations that the shared abstractions now expect to honor.
- Concurrency and thread safety: Shared late-RF history, per-scanner applied counts, lock coverage, and append-order identity were traced without a new missed/duplicated RF, lock-order, or deadlock issue. The existing non-V2 clone-under-lock concern was not duplicated.
- Lifecycle, errors, and memory safety: MAIN-001 violates the expression prepare/open ownership boundary and can execute an unopened runtime-filter clone. Other reviewed native/synthetic lifecycles close normally; no additional allocator, ownership-cycle, silent-Status, or memory-safety issue was substantiated.
- Configuration: No configuration item is added or changes dynamic-update semantics.
- Compatibility: No wire, persisted metadata, storage-format, symbol, or rolling-upgrade incompatibility was identified.
- Parallel paths: Parquet, ORC, generic materialized readers, remote/native readers, Hudi/Paimon native children, and Iceberg synthetic readers were traced. Direct JNI and Hudi/Paimon JNI children are the two missed parallel paths called out inline.
- Special conditions and data correctness: The safe-prefix/unsafe-suffix barrier, split resets, defaults/missing/partition/virtual columns, bounded strings, delete handling, EOF-with-rows, and condition-cache publication were checked. Apart from MAIN-001, the ownership/count conditions preserve row semantics.
- Tests and results: Changed unit tests were reviewed statically and their expected logic is consistent, but real-JNI negative coverage is missing for both findings. Per the review-task contract, no build or test command was run.
- Observability: New residual/profile and adaptive counters are useful on native paths; MAIN-002 leaves JNI progress and adaptive samples inaccurate. No additional logging/metrics blocker was found.
- Transactions, persistence, and data writes: Not applicable; this is a read-path change with no EditLog, transaction, persisted state, or data-write atomicity impact.
- FE/BE propagation: No new FE/BE session variable or thrift field requires propagation.
- Performance: MAIN-002 permits a reject-all JNI split to drain in one Scanner task and trains adaptive sizing from survivors. No other material CPU or memory regression was substantiated; the bounded redundant hybrid-wrapper contexts were reviewed and dismissed as cost-only.
- Other issues: Null/nullable shapes, aggregate eligibility, cancellation, hybrid child switching, and all changed test paths were rechecked without another accepted finding.
Review completion: Complete after two rounds; both normal reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS against this exact two-comment set. Existing review threads were deduplicated before submission.
User focus: No additional user-provided review focus was supplied; the full patch was reviewed.
| RETURN_IF_ERROR(_prepare_conjunct(source, &conjunct)); | ||
| } else { | ||
| // Preserve Scanner as the sole owner of runtime state for appended residuals. | ||
| RETURN_IF_ERROR(_clone_conjunct(source, &conjunct)); |
There was a problem hiding this comment.
[P1] Keep analysis-only late conjuncts out of JNI filtering
When a Scanner-owned suffix already exists, _sync_table_reader_conjuncts() passes owned_count=0, so this branch deliberately creates an unprepared/unopened analysis clone. The JNI path still prepares and filters the complete _conjuncts vector (JniTableReader plus the JDBC/Hudi/MaxCompute finalizers), so the next JNI batch executes this clone; RuntimeFilterExpr::execute_filter() DCHECKs _open_finished, and Scanner would evaluate its separately owned RF again. Please make every JNI finalizer evaluate only the TableReader-owned prefix (without re-opening analysis-only suffixes) and add an active-split late-RF JNI test.
| return Status::OK(); | ||
| } | ||
|
|
||
| size_t FileScannerV2::_last_block_rows_read(const Block& block) const { |
There was a problem hiding this comment.
[P1] Preserve the Scanner progress bound for JNI batches
This fallback is useful only when the reader returns once per materialized batch. JniTableReader::get_block() neither records MaterializedBlockStats nor returns when a Java batch is fully filtered; it keeps reading until a survivor or EOF. Therefore direct JNI and Hudi/Paimon JNI splits can drain an entire reject-all split in one Scanner task, while adaptive sizing learns from survivors instead of the pre-filter Java block. Please reset/record JNI materialization stats before predicate filtering and return an empty progress batch after one rejected Java batch, with a JNI coverage case.
TPC-H: Total hot run time: 29686 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-DS: Total hot run time: 176940 ms |
ClickBench: Total hot run time: 25.05 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
What problem does this PR solve?
Issue Number: N/A
Related PR: #65998
Problem Summary:
FileScannerV2 needs a stable ownership boundary for predicates across split transitions. Only the safe prefix may execute inside TableReader; the first unsafe predicate and every later predicate must remain Scanner-owned so stateful and error-preserving evaluation order is unchanged.
The previous implementation could also rebuild next-split partition-pruning predicates from a cost-sorted Scanner snapshot. A cheap late runtime filter could therefore cross an older unsafe ordering barrier. In addition, TableReader prepared and opened Scanner-owned suffixes even though it retained them only for pruning analysis, creating duplicate expression state.
This change:
Release note
None
Check List (For Author)
FileScannerV2Test.*TableReaderTest.*ScannerLateArrivalRfTest.*