[fix](io) Harden Parquet reader-local cache for File Scanner V2 - #66548
[fix](io) Harden Parquet reader-local cache for File Scanner V2#66548Gabriel39 wants to merge 11 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Gabriel39
changed the title
|
run buildall |
|
/review |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
20f2409 to
cecb633
Compare
Gabriel39
changed the title
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
changed the title
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
|
run buildall |
|
/review |
Gabriel39
changed the title
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
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.
Requesting changes. I found 10 actionable issues: 2 P1 and 8 P2.
Findings, ordered by severity
- P1: zero-survivor staged Parquet batches physically skip later predicates before their deferred MergeRange ranges are activated, reintroducing unmerged remote reads.
- P1: the reader-local cache destructor allocates from an implicitly noexcept teardown path and can terminate the BE under memory pressure.
- P2: the decoder benchmark registers 266 cases while the mandatory benchmark guide still requires 228 and omits the 5% axis.
- P2: a short reader-local entry permanently blocks promotion of a later wider FileCache extent at the same offset.
- P2: ReaderLocalCacheAdmissionRejects is dead because reservation rejection never sets its source flag.
- P2: split miss runs collapse multiple physical peer/S3 calls into one profile request and can lose mixed-source attribution.
- P2: overlapping eager Parquet ranges can double-count useful bytes and underflow MergedGapBytes.
- P2: REMOTE_ONLY_ON_MISS full FileCache hits bypass reader-local promotion for unwrapped external Parquet readers.
- P2: shared FileReadCalls and FileReadTime counters are still overwritten by sibling scanners even though bytes now use additive deltas.
- P2: FuturePredicatePrefetchBytes has no production-reachable greater-stage range and remains zero; its nonzero test uses a scheduler-unreachable sequence.
Critical checkpoints
- Correctness and lifecycle: exact-cache coverage/fallback, row-group reset, pending batches, and single-flight loader publication were traced and are otherwise sound. The deferred physical-skip hole and noexcept teardown allocation remain blocking.
- Concurrency and memory: reservation commit/cancel/release, waiter notification, pinning, and eviction balance on the reviewed exits. The short/wide replacement gap and teardown snapshot still violate the cache's best-effort contract.
- Compatibility and decode: external-Parquet opt-in and the decoder threshold/Q28 behavior are sound. Pre-PARQUET-816 padded overlaps expose the merged-accounting defect.
- Performance and observability: the inline findings cover the remaining promotion, request/source accounting, shared-profile, dead-counter, and staged-range visibility gaps.
- Tests and validation: the changed tests cover many helper paths but miss the production scheduler/policy/multi-scanner cases above; the benchmark validation guide is also stale. Per the review task instructions, no builds were run.
- User focus: no additional user-provided focus was supplied, so the complete PR was reviewed without a narrower focus override.
Review completeness
The authoritative 30-file diff at dde77c6 was reviewed end to end. Three fresh normal/risk rounds completed; the final round returned NO_NEW_VALUABLE_FINDINGS from all reviewers. Existing inline-thread context was empty, all candidates were deduplicated, and the 10 accepted findings are attached inline.
| const auto& col = request.predicate_columns[idx]; | ||
| const auto fid = col.column_id(); | ||
| if (_current_merge_range_reader != nullptr) { | ||
| RETURN_IF_ERROR(activate_merge_ranges_for_columns({fid})); |
There was a problem hiding this comment.
[P1] Activate deferred ranges before skipping later predicates
When an early staged predicate reduces selected_rows to zero, this activation path is never reached for the remaining predicate columns. skip_unmaterialized_predicate_columns() still calls NativeColumnReader::skip(batch_rows) on each of them; inside the current selected range that executes read_with_filter(), but the deferred MergeRangeFileReader still has no registered range and therefore falls through to direct underlying reads. Highly selective cold remote scans can regress to one set of unmerged page/header reads for every later predicate column. Please activate each unmaterialized column before its physical skip (or retain a logical pending skip until activation), and cover the zero-survivor staged-predicate path with an underlying-request-count test.
| _query_mem_tracker(std::move(query_mem_tracker)), | ||
| _memory_tracker(std::make_shared<doris::MemTracker>("FileScannerV2ReaderLocalCache")) {} | ||
|
|
||
| FileScannerV2ReaderLocalCache::~FileScannerV2ReaderLocalCache() { |
There was a problem hiding this comment.
[P1] Keep the cache destructor allocation-free
This destructor is implicitly noexcept, but _file_caches() allocates a new vector and calls reserve(_files.size()) before any of the exception-safe drain logic runs. Under the same query/global memory pressure this cache is designed to tolerate, std::bad_alloc here escapes the destructor and terminates the BE. The registry also retains expired weak entries, so the snapshot can allocate according to the scanner's file-count high-water mark even when no live file cache remains. Please make teardown walk/drain the registry without allocating, or contain allocation failure while still releasing the budget, and add a teardown-failure regression test.
| inline bool register_decoder_benchmarks() { | ||
| for (const auto& scenario : decoder_scenarios()) { | ||
| for (const int selectivity : {0, 1, 10, 50, 90, 100}) { | ||
| for (const int selectivity : {0, 1, 5, 10, 50, 90, 100}) { |
There was a problem hiding this comment.
[P2] Keep the benchmark validation guide in sync
This loop now registers 19 x 7 x 2 = 266 decoder cases, and the changed invariant test expects 266, but be/benchmark/parquet/AGENTS.md still tells reviewers to expect 228 in three places and still lists only six selectivities. That guide is the mandatory validation contract for this directory, so its prescribed registration check will reject the new matrix. Please update the count and add the 5% axis there as part of this change.
| lookup->wait_time = wait_watch.elapsed_time(); | ||
| } | ||
| RETURN_IF_ERROR(entry->load_status); | ||
| if (entry->data == nullptr || entry->data->size() < block_size) { |
There was a problem hiding this comment.
[P2] Let a wider extent replace a short cache entry
This safely refuses to reuse a short promotion, but it leaves that entry in _entries and _lru. If FileCache later recreates a wider cell with the same left boundary, every wider read hits this same mismatch and falls back to disk forever; the wider extent can never be promoted until unrelated capacity eviction happens. Please retire/replace an unpinned undersized entry (or include the extent in the key) while preserving single-flight behavior, and cover short-cell-then-wider-cell repeated reads.
| std::shared_ptr<std::vector<char>> data; | ||
| std::optional<SwitchThreadMemTrackerLimiter> switch_query_tracker; | ||
| try { | ||
| if (!owner->_reserve(block_size, this, &lookup->evicted)) { |
There was a problem hiding this comment.
[P2] Publish reservation failures as admission rejects
LookupResult::admission_rejected defaults to false and is never set anywhere, including this _reserve() failure path. _read_local_block() consequently adds zero to ReaderLocalCacheAdmissionRejects for every capacity, query-limit, and global-soft-limit rejection, making the new counter permanently dead. Please set the flag for policy rejection (and define whether allocation failure is included), then assert it in the existing pressure tests.
| } | ||
| // A cache hit is a hard merge boundary. Reading across it would redownload resident data | ||
| // and violate the cache-aware miss coalescing invariant used by StarRocks. | ||
| std::vector<FileBlockSPtr> contiguous_misses(empty_blocks.begin() + run_start, |
There was a problem hiding this comment.
[P2] Account for every split source request
This loop can now issue one peer/S3 call per contiguous miss run (the new miss-hit-miss test observes two), but the shared stats retain only copied byte totals and one last-writer-wins from_peer_cache bit. _update_stats() therefore increments NumRemoteIOTotal/NumPeerIOTotal at most once, and a later run can erase the source of a backward-aligned fill that copied no caller bytes. That hides the request amplification this change is meant to control and can omit mixed peer/S3 work. Please accumulate per-run peer/remote call counts (and used-source state) and assert both counts in split and mixed-source tests.
| continue; | ||
| } | ||
| const size_t overlap = overlap_end - overlap_start; | ||
| useful_bytes += overlap; |
There was a problem hiding this comment.
[P2] Count the union of eager-range overlaps
Eager native ranges are only sorted before this reader is constructed; unlike deferred ranges, they are not coalesced. For pre-PARQUET-816 files, the 100-byte chunk padding can make adjacent ranges overlap, so this loop counts those bytes twice and bytes_read - useful_bytes underflows. MergedGapBytes can then become negative or enormous, breaking the new profile invariant. Please normalize constructor ranges too, or compute the union of intersections and guard the subtraction; add an eager-overlap accounting test.
| const bool bypass_reader_local_cache = io_ctx->bypass_reader_local_cache; | ||
| MonotonicStopWatch reader_local_probe_watch; | ||
| reader_local_probe_watch.start(); | ||
| const bool reader_local_hit = |
There was a problem hiding this comment.
[P2] Promote full hits in the remote-only policy
The hot probe above cannot help until some path has populated the reader-local map, but _read_remote_only_on_cache_miss() still reads fully covered blocks via FileBlock::read() directly. After the scanner's write limiter switches to this policy, an unwrapped/large-chunk Parquet reader can repeatedly hit FileCache on disk without ever creating the 256 KiB promotion, so the default-enabled cache is ineffective for exactly those revisits. Please route full covered hits through _read_local_block() while retaining remote-only miss behavior, and test this with an external reader (the current Doris-table tests disable the cache).
| COUNTER_SET(_file_read_bytes_counter, bytes_read); | ||
| // Scanner instances share the profile counter, so publishing an absolute value would erase | ||
| // bytes already reported by sibling scanners. | ||
| COUNTER_UPDATE(_file_read_bytes_counter, |
There was a problem hiding this comment.
[P2] Aggregate calls and time across sibling scanners too
The sharing rule in this new comment applies to all three adjacent counters, but only bytes was converted to a per-scanner additive delta. FileReadCalls and FileReadTime still use scanner-local cumulative COUNTER_SET values here and in _collect_profile_before_close(), so each realtime/final publication overwrites work already reported by sibling scanners and the result depends on scheduling/close order. Please track reported calls/time and COUNTER_UPDATE their deltas as well, then add an interleaved two-scanner profile test that converges to the sum.
| } | ||
| const size_t overlap = overlap_end - overlap_start; | ||
| useful_bytes += overlap; | ||
| if (_range_stages[index] > _range_stages[range_index]) { |
There was a problem hiding this comment.
[P2] Make future-stage accounting reachable from production
This comparison has no production path with a greater-stage range already present. Eager readers construct the whole plan with every stage set to 0 and never call add_random_access_ranges(); deferred readers insert only the current predicate immediately before reading it, and lazy outputs are inserted together at one stage. FuturePredicatePrefetchBytes therefore stays zero, while the changed nonzero test manually pre-registers stage 1 in a sequence the scheduler never performs. Please instrument stage metadata/visibility from the real scheduler (or remove/rename this counter if zero is the intended invariant) and cover it with a scheduler-level profile test.
2 participants
What problem does this PR solve?
File Scanner V2 Parquet scans repeatedly access small page headers and payload fragments from downloaded FileCache blocks.
What is changed?
This PR is based on and depends on #66504.
Validation