[fix](scan) Backport FileScannerV2 predicate ownership fixes - #66356
[fix](scan) Backport FileScannerV2 predicate ownership fixes#66356Gabriel39 wants to merge 10 commits into
Conversation
Keep identity selections implicit, refresh late predicates at row-group boundaries, and reuse dictionary predicate bitmaps within a query.
### 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
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.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
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. |
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review found three correctness regressions and two cooperative-progress gaps in the predicate ownership transfer:
- residual contexts lose thread-local function state;
- predicate-only dictionary materialization can corrupt later residual/delete predicates;
- removing the unsafe-prefix barrier can suppress ordered errors;
- all-reject late filters can bypass Scanner progress in JNI and refreshed Parquet paths.
No additional review focus was provided, so I reviewed the full authoritative 44-file diff. I did not run builds or tests under the static-review constraint.
| } | ||
| return status; | ||
| size_t FileScannerV2::_last_block_rows_read(const Block& block) const { | ||
| const auto& stats = _table_reader->last_materialized_block_stats(); |
There was a problem hiding this comment.
[P2] Account for JNI batches before falling back to survivor rows. JniTableReader::get_block() applies the now-owned (including late) conjuncts and loops again whenever a Java batch is fully rejected, but it never records MaterializedBlockStats. Since Scanner filtering is now a no-op, a late selective RF can drain the rest of a large JNI split in one scheduler turn, while these hooks see only zero/survivor rows and cannot enforce the row/byte budget or learn the pre-filter width. Please record the pre-filter JNI rows/bytes and yield after one rejected materialized batch, as the base TableReader path now does.
| // `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 predicate-only values needed by later consumers. This mapper decision only proves that query filters localized into FileReader; it does not prove Parquet can discard the value while staging them. On dictionary encoding, the direct path checks only same-column dictionary residuals, emits a default placeholder, and marks the position materialized even when residual_predicate_positions says a multi-column residual or Iceberg equality-delete conjunct still needs the real value. Those later predicates then evaluate defaults and can return wrong rows. Please gate dictionary predicate-only mode on the residual/delete position set (as the fixed/runtime direct path does) and cover both overlaps.
| Status TableReader::_prepare_conjunct(const VExprContextSPtr& source, VExprContextSPtr* prepared) { | ||
| DORIS_CHECK(prepared != nullptr); | ||
| VExprContextSPtr conjunct; | ||
| RETURN_IF_ERROR(_clone_conjunct(source, &conjunct)); |
There was a problem hiding this comment.
[P1] Open rebuilt predicate contexts with per-scanner function state. _clone_conjunct() creates a fresh VExprContext, so this open() uses FRAGMENT_LOCAL; however residual functions such as LIKE/REGEXP initialize their LikeState only for THREAD_LOCAL and then dereference it during execution (Java/Python UDFs have the same contract). Because LIKE is unsafe to localize, WHERE s LIKE 'x%' now reaches TableReader's residual filter after Scanner relinquishes the predicate and fails with missing/null function state. Please preserve the original fragment state and clone/open the rewritten context with thread-local state, and add a residual LIKE regression.
| // Unsafe or non-deterministic predicates remain at TableReader. Safe predicates are | ||
| // independent pruning candidates even when an earlier predicate cannot be pre-executed. | ||
| if (!is_safe_to_pre_execute(conjunct)) { | ||
| continue; |
There was a problem hiding this comment.
[P1] Preserve the unsafe conjunct as an ordering barrier. With a partition value p = BIGINT_MIN and ordered predicates mod(p, -1) > 0 AND p > 0, the first predicate is unsafe because it must raise INVALID_ARGUMENT, while the second is safely false. Continuing here lets the second predicate prune the split before the modulo executes, suppressing that required error; V1 and the pre-change V2 path stop at the first unsafe conjunct for exactly this reason. Please restore the safe-prefix barrier for partition/constant pruning and later localization, and cover this error-preserving order.
| }; | ||
| while (true) { | ||
| if (!_has_current_row_group) { | ||
| activate_pending_scan_request_at_row_group_boundary(); |
There was a problem hiding this comment.
[P2] Bound all-reject work after activating the refreshed request. If a late RF rejects the remaining row groups, read_next_batch() keeps growing and retrying zero-survivor probes until a survivor or EOF; Parquet publishes its raw-row delta only after that call returns, so Scanner cannot charge its row budget or reach cancellation checkpoints during the drain. This PR newly moves active-split late RFs into that loop. Please return a bounded empty progress unit (with consumed raw rows/bytes) and add an all-reject late-RF regression.
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
2 participants
What problem does this PR solve?
Issue Number: N/A
Backport the selected FileScannerV2 predicate ownership and localization fixes from #66261 to
branch-4.1.The selected commits require the earlier predicate-refresh and residual-ownership infrastructure from the same PR. This PR therefore includes the complete dependency prefix through the four selected commits, preserving their original order and cleanup pairing.
The changes keep residual predicate ownership ordered across splits, localize each safe predicate independently, recompute localization for every split, and avoid retaining predicate-column payloads after exact file-level filtering when those values are no longer needed.
Release note
None
Check List (For Author)
TableReaderTest.*tests passed.FileQueryScanNodeTest: 7 tests passed.