branch-4.1: [Enhancement](file scanner) Support row-id fetch in FileScannerV2 #67906 - #67922
branch-4.1: [Enhancement](file scanner) Support row-id fetch in FileScannerV2 #67906#67922Gabriel39 wants to merge 3 commits into
Conversation
… branch-4.1 (apache#67906) Adapt scanner row-id reads and lazy column categories to branch-4.1, preserving Lance uint64 reads, physical splits, and condition-cache state. Use the existing Iceberg scan node and row-lineage regression suite. (cherry picked from commit bda646e)
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
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.
Request changes: the new FileScannerV2 TopN row-ID route has one unsafe cardinality boundary and two Parquet performance/resource regressions.
Findings:
- The V2 exact-row API can accept a short Parquet result and pass dense requested positions to an unchecked reorder consumer. V1 has an analogous omission, so this is scoped to the safety contract of the newly added V2 entry point rather than claimed as a parity regression.
- Disjoint Parquet row IDs are returned one selected range/block at a time instead of filling the requested batch across ranges.
- The row-ID base-reader path retains the full per-leaf buffered read-ahead, allowing a flat wide projection to exceed the configured aggregate row-group budget even for one fetched row.
Architecture and interface review: I traced the FE full-schema category map through Thrift, IdFileMap, phase-two parameter reconstruction, standalone Scanner/TableReader/FileReader setup, exact-row format dispatch, and result reordering. Source identity, absolute row coordinates, deduplication, duplicate restoration, and output ordering are consistent for stable inputs.
Schema/materialization and table formats: regular, generated, partition, synthesized, missing/default, positional, and virtual columns retain their intended physical-index consumption. The copied Iceberg/Hive range suppresses already-applied delete state while preserving file identity, original path, format/version, partition data, and v3 row-lineage metadata. ORC's split validation, one-row seek/read, EOF behavior, and virtual positions are aligned.
Compatibility and lifecycle: the phase-two route reuses the phase-one presence-sensitive V2 rollout gate; old payloads and unsupported/transactional formats remain on V1 or their existing paths. I found no distinct surviving resource, cleanup, or observable-profile defect beyond the inline issues.
Performance and tests: Parquet exact-range/cache accounting is otherwise preserved, but the batching and per-leaf read-ahead issues are not covered by the added tests. The needed coverage is a many-disjoint-ID batch-count case and a production-default flat-wide cold-cache byte/peak-buffer case, plus an explicit short-cardinality failure case. This was a static review only; per the review contract, I did not run builds or tests or modify source files.
User focus: no additional review focus was provided; the full 23-file diff and its relevant callers/consumers were reviewed.
Reject short V2 fetches before indexed reordering, fill sparse Parquet batches across ranges, and disable per-leaf read-ahead for exact-row requests. Cover cardinality, batching, nested output, and wide-projection buffer usage; use supported virtual names in the backport fixture. Validation: 274 related BE ASAN tests and clang-format 16 passed.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of exact head a4abc561e153da04960ea10697a07ba6854cde51. I am requesting changes for one P2 performance regression in the new FileScannerV2 row-ID path.
I traced the FE-to-BE lazy-materialization contract, scanner rollout and V1 fallback, row-ID grouping/deduplication/reordering, Hive/Iceberg projection and category reconstruction, standalone TableReader lifecycle and exact-cardinality handling, Parquet range/page/cursor/I/O composition, ORC seek/split/virtual-position behavior, optional-Thrift compatibility, and the changed tests. The existing threads for short reads, disjoint-range batching, and per-leaf read-ahead were treated as duplicate fences and rechecked against the current head.
The remaining issue is that sparse nested Data Page V1 reads have no usable physical page-navigation path. Combined with the new zero-read-ahead policy, a late requested row can require page-by-page demand I/O and decoding through the entire preceding portion of every projected repeated leaf.
No additional user-provided focus was present. This was a static-only review: no builds or tests were run under the review contract; author/CI test claims were not independently executed here.
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: one distinct P2 performance issue remains in the new FileScannerV2 Parquet row-ID path.
Finding: predicate-free phase-two requests never load available validated OffsetIndexes for flat Page V1/V2 or all-V2 nested chunks. A late row therefore traverses every preceding page header at the PageCache/demand-reader layer for every projected leaf, despite these modes already supporting indexed navigation. The inline comment is deliberately separate from the existing nested-Page-V1 thread, whose level/value replay requires a different solution.
Architecture and interface contracts: I traced the FE full-schema category map through Thrift, IdFileMap, phase-two parameter rebuilding, standalone Scanner/TableReader/FileReader setup, physical-reader dispatch, exact-count enforcement, and final duplicate restoration/reordering. Global, table, file-local, physical-leaf, row-group, and absolute-row identities remain separated; I found no distinct mapping or materialization defect.
Lifecycle, rollout, and compatibility: successful and short reads close before reordering; failed standalone reads immediately destroy their owned reader graph, so no actionable retained-resource issue was substantiated. Presence-sensitive rollout gates retain V1 for absent/disabled options and unsupported or transactional modes. Hive positional mapping and Iceberg field-id, partition, delete-state, file-identity, and row-lineage reconstruction remain consistent.
Format correctness: ORC validates absolute IDs against the split and preserves seek, EOF, and virtual-row positions. Parquet row-group-local range conversion, disjoint/cross-row-group accumulation, scalar/nested cursor alignment, missing-ID cardinality checks, MergeRange/prefetch fencing, and retained-buffer bounds were rechecked. The existing short-read, range-batching, and wide-read-ahead threads have fixes in this head; the existing nested Page V1 concern remains separately tracked.
Tests and observability: the changed tests cover empty/short IDs, disjoint batches, nested appends, cached-remote bytes, FE category propagation, ORC row fetch, and Iceberg lineage, but do not bound a late indexed seek for flat V1/V2 or nested V2. Please add that header/read-call bound with the fix. This was a static-only review under the runner contract: no builds or tests were run independently. Author-reported test results were not independently verified.
User focus: no additional user-provided focus was supplied; the complete 25-file authoritative diff and relevant callers/consumers were reviewed.
| const size_t max_buffer_size = std::min(max_group_buffer, max_column_buffer); | ||
| // Sparse exact-row fetches need demand pages, not one read-ahead buffer per physical leaf. | ||
| // Passing zero through the native tree also prevents wide nested projections multiplying it. | ||
| const size_t max_buffer_size = |
There was a problem hiding this comment.
[P2] Retain indexed navigation when disabling read-ahead
Zero configured read-ahead is appropriate for exact-row payloads, but predicate-free row-ID requests never populate row_group_plan.offset_indexes: the only current path is load_native_page_indexes(), gated on a zonemap predicate, while the validated standalone load_native_offset_indexes() has no caller. For a late ID, flat Page V1/V2 and all-V2 nested readers therefore parse every preceding page header to discover row bounds. Each page incurs a PageCache lookup and, on an ordinary cold miss, a demand FileReader operation per projected leaf (lower file-cache layers may still coalesce network traffic). This is distinct from the existing nested-Page-V1 thread: these modes already support indexed navigation, so loading their validated OffsetIndexes is sufficient. Please load them for row-ID projections with the existing malformed/absent-index fallback, and test a late row with a header/read-call bound.
2 participants
What problem does this PR solve?
Backport #67906 to branch-4.1.
TopN two-phase materialization can fetch selected Parquet and ORC file rows through FileScannerV2. The second phase follows the scanner rollout policy, avoids whole-chunk Parquet prefetch for sparse row selections, and preserves partition/generated/synthesized column categories and Iceberg file metadata. Exact-row fetches reject short results before reordering, fill output batches across sparse ranges, and use demand-page reads for Parquet projections.
Compatibility adjustments for branch-4.1:
_file/_posfeature and connector framework are not prerequisites for this backport.Release note
Support row-id fetch for Parquet and ORC in FileScannerV2.
Check List (For Author)
FileQueryScanNodeTestpassed (14 tests, zero failures/errors); Maven validate passed with zero Checkstyle violations.