[fix](external) Schedule Parquet files as scanner parent tasks - #66602
[fix](external) Schedule Parquet files as scanner parent tasks#66602Gabriel39 wants to merge 16 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
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.
Requesting changes because the new parent-task/cache paths still have correctness and resource regressions.
Critical checkpoints:
- Split ownership and lifecycle: PARQUET-816 padding can duplicate Row Groups; parent open errors bypass the NOT_FOUND policy; local one-file scans stay single-scanner; small HTTP files are staged N+1 times.
- Cache correctness and memory: reader-local sharing lacks a stable filesystem/version identity, and the strong registry is outside its capacity/tracker.
- MergeRange and observability: lazy-output staged activation is unreachable, and specialized index/footer RPC counters undercount disjoint misses. Exact-cache fallback, overlap normalization, and general byte accounting otherwise remained sound.
- Table-format/compatibility: explicit split-size overrides, COUNT shortcuts, delete/schema propagation, and rolling FE/BE fallbacks remained sound.
- Decoder/benchmark/tests: decoder heuristics and counts were consistent, but the benchmark guide advertises unregistered 5% Selection/Reader cases. Existing tests miss the production scanner cap, old-writer ownership, HTTP staging, and lazy MergeRange reachability.
Review workflow: three complete normal plus risk-focused rounds converged with nine distinct inline findings. No builds were run, per the review-runner instructions.
User focus: no additional user-provided focus was present; the full PR was reviewed.
| } | ||
| if (is_file_parent) { | ||
| std::vector<FileScanSplitTask> children; | ||
| const auto build_status = _table_reader->build_file_split_tasks(&children); |
There was a problem hiding this comment.
A parent is deliberately not opened by prepare_split(), so a missing file is first observed here. This branch releases the parent and returns build_status directly, bypassing the _should_skip_not_found() policy above; the same stale listing is skipped as an ordinary split when ignore_not_found_file_in_external_table is enabled but now fails the query when it is marked as a parent. Please route parent-build failures through the same NOT_FOUND handling, then continue to the next split after finishing the parent.
| } | ||
|
|
||
| targetSplitSize = determineTargetFileSplitSize(fileScanTaskList); | ||
| if (sessionVariable.enableFileScannerV2) { |
There was a problem hiding this comment.
In the ordinary non-batch path, preserving this Parquet task leaves one FE range for a backend. FileScanLocalState::set_scan_ranges() then clamps _max_scanners to LocalSplitSourceConnector::num_scan_ranges(), so a one-file scan creates exactly one FileScannerV2; its generated Row Group children are all consumed serially and the former multi-split scan can collapse to one core. Batch mode avoids this clamp, but local mode does not. Please make the scanner-count calculation expansion-aware (and cover one local parent through production scanner construction); the native Paimon parent path needs the same treatment.
| // Path, version, and size form a stable physical-file identity across the row-group | ||
| // readers created from one parent task without extending the aggregate reader options. | ||
| _reader_local_file_cache = _reader_local_cache->get_or_create_file_cache( | ||
| fmt::format("{}:{}:{}", path().native(), opts.mtime, opts.file_size)); |
There was a problem hiding this comment.
This shared map needs a stable physical-file identity, but the key drops fs_name/storage identity and accepts the unknown sentinels mtime == 0 and file_size == -1. In particular, HdfsFileReader normalizes away the nameservice, while TFileRangeDesc permits different fs_name values in one scan, so equal paths/mtime/size on two filesystems alias here; the second reader can return the first file's promoted bytes before touching its own storage. Please use the canonical filesystem/resource plus path, reliable version, and actual-size fallback, and disable sharing for a mutable file whose version is unknown (as build_native_file_cache_key() already does).
| try { | ||
| auto file_cache = std::shared_ptr<FileScannerV2ReaderLocalFileCache>( | ||
| new FileScannerV2ReaderLocalFileCache(shared_from_this())); | ||
| _file_cache_by_key.emplace(file_key, file_cache); |
There was a problem hiding this comment.
The configured capacity only accounts promoted vector bytes, while this strong registry entry, its key, mutex/map object, and later entry metadata are retained outside _memory_bytes and the cache tracker. _file_cache_by_key is pruned only from payload-reservation pressure, so a many-file scan whose files stay cold or whose MergeRange reads bypass reader-local promotion can grow one empty retained object per file without entering that path. Please bound and track the registry itself (or use an eviction/weak-retention scheme independent of payload pressure) and add a no-promotions many-file test.
| // Independent predicate/output readers may revisit the same physical leaf at different | ||
| // cursors. MergeRangeFileReader has one consumptive cache per range, so use the random | ||
| // access reader for this layout instead of sharing one sequential range cache. | ||
| _current_merge_range_active = file_context.set_native_random_access_ranges( |
There was a problem hiding this comment.
This is the predicate-plus-lazy-output layout that needs staged activation, but passing {} makes should_use_merge_range_reader() return false. Consequently _current_merge_range_reader remains null, the per-column ranges below are never recorded, and every predicate/lazy activation hook later in the scheduler is a no-op; remote scans with projected payload still fall back to small direct reads. Please make the staged path reachable with a design that preserves the independent predicate/output cursor contract (the current single consumptive cache cannot simply be enabled for revisited leaves), and add an integration test proving predicate activation plus survivor-only lazy activation.
| statis->num_remote_io_total++; | ||
| if (source_read_breakdown.remote_bytes != 0 || source_read_breakdown.remote_requests != 0) { | ||
| statis->num_remote_io_total += | ||
| std::max<int64_t>(source_read_breakdown.remote_requests, 1); |
There was a problem hiding this comment.
_read_from_indirect_cache() can now issue one remote/peer request per disjoint miss run, and the general profile correctly adds source_read_breakdown.*_requests. This specialized aggregation still increments only once when any source bytes exist, so a [miss][hit][miss] index/footer read reports two physical RPCs generally but one in its index/footer subprofile. Please use the request counts here as well and extend the split-miss test to cover both specialized profiles.
| row_group_idx, column_idx); | ||
| } | ||
| native::ColumnChunkRange chunk_range; | ||
| RETURN_IF_ERROR(native::compute_column_chunk_range( |
There was a problem hiding this comment.
The compatibility padding is a decoder read extent, not a disjoint ownership boundary. For an affected file with adjacent one-column chunks (offset=100,size=20) and (150,20), this call pads them to child ranges [100,220) and [150,270). Selection later recomputes padded Row Group midpoints 160 and 210, both of which lie in both children, so each child reads both Row Groups and duplicates the rows. Please keep padding out of task ownership (for example, carry the exact Row Group ordinal in the child context) and add an old parquet-mr fixture that asserts one distinct group per child.
| `ParquetSelection` contains 25 cases that isolate the selection-vector work used by Parquet | ||
| predicate evaluation. It measures identity initialization, one raw-row filter, and two successive | ||
| filters. The filter matrix covers 0%, 1%, 10%, 50%, 90%, and 100% selectivity with clustered and | ||
| filters. The filter matrix covers 0%, 1%, 5%, 10%, 50%, 90%, and 100% selectivity with clustered and |
There was a problem hiding this comment.
The guide now names 5% Selection and Reader scenarios, but their generators still use {0,1,10,50,90,100}; only the Decoder matrix gained 5%. ParquetSelection still has 25 cases, which is also incompatible with the seven selectivities listed here. Please either keep these two descriptions aligned with the registered matrices or add the missing scenarios and update their counts/invariants.
| group_start = std::min(group_start, chunk_range.offset); | ||
| group_end = std::max(group_end, chunk_range.offset + chunk_range.length); | ||
| } | ||
| TFileRangeDesc child = parent; |
There was a problem hiding this comment.
Each generated child opens a fresh ParquetFileContext, but small HTTP(S) files are wrapped in a fresh InMemoryFileReader before the shared-footer check. The parent therefore stages the complete object to parse its footer, and each of N Row Group children stages the complete object again on first data access—N+1 full downloads without FileCache, or N+1 full cache reads and copies with it. Please share the staged immutable buffer across children or skip Row Group expansion for this whole-object HTTP path, and add a multi-Row-Group HTTP test that asserts one full-object load.
|
run buildall |
|
/review |
|
run buildall |
|
/review |
|
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. |
7499add to
46ecd98
Compare
|
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. |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Follow-up review of current head 948cf698297f96a735ebae158d918acaea8b69af found no new distinct inline issues.
Critical checkpoints:
- Parent lifecycle and scheduling: parent claims, child publication, EOS/wakeup, cancellation/close, FE eligibility, and expansion-aware scanner capacity are sound on the reviewed paths. The existing thread at
be/src/exec/scan/file_scanner_v2.cpp:673still applies: a parent that first observesNOT_FOUNDinbuild_file_split_tasks()returns the error directly instead of honoringignore_not_found_file_in_external_table, so that remains blocking. - Row Group and table semantics: raw task ownership, shared footer/staged HTTP lifetime, file-global row positions, descriptor materialization, and Iceberg/Paimon delete handling retain one intended child and the parent table semantics.
- MergeRange and cache correctness: staged predicate/lazy activation, independent readers for revisited nested roots, overlap normalization, exact-cache fallback, per-stream reader-local isolation, weak registry ownership, reservation/pinning/single-flight cleanup, and bypass paths preserve bytes, cursor state, wakeups, and the configured budget.
- Observability and decoder behavior: disjoint physical request counts, reader-local/exact-probe counters, sibling-scanner realtime/final deltas, the sparse dictionary heuristic, and the 266-case decoder benchmark matrix are internally consistent.
- Compatibility and coverage: explicit split-size overrides, COUNT/JNI/ORC/load fallbacks, rolling FE/BE behavior, and the changed regression tests were inspected. No builds or tests were run, per the review-runner instructions.
The current head contains the requested follow-ups for scanner capacity, Row Group ownership, small-HTTP staging, reader-local isolation/lifetime, staged MergeRange reachability, specialized request accounting, and benchmark documentation. I am not duplicating those existing inline threads, and I am not duplicating the still-applicable parent NOT_FOUND thread.
Review workflow: one complete round with two normal full-scope reviews and separate risk-focused reviews for all six main risk areas converged with NO_NEW_VALUABLE_FINDINGS; the main-agent final sweep found no unresolved new candidate. This is therefore a comment-only follow-up with zero new inline comments, while the prior changes-requested state remains warranted by the existing blocker above.
User focus: no additional user-provided focus was present; the full PR was reviewed.
|
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. |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
2 participants
Proposed changes
Validation
This PR is based on and intended to follow #66548.