Repository navigation
[Enhancement](file scanner) Support row-id fetch in FileScannerV2 - #67906
Merged
Gabriel39 merged 4 commits intoSep 14, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 16571 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of exact head f80bae195f7ed0db89c9beb6950475440750209c: 2 blocking findings, both attached inline.
Checkpoint conclusions:
- Goal / proof: The format-neutral row-ID plumbing and the Parquet/ORC absolute-row selection are coherent by static inspection, but the phase-two implementation does not preserve the scanner rollout choice and does not deliver selective remote Parquet I/O in a reachable cache configuration.
- Scope / reuse: The change is focused and reuses
TableReader, native readers, and existing selected-range/seek abstractions. - Concurrency / lifecycle: Per-task scanner and reader state, completion publication, EOF/close, and failure ownership were traced; no separate race, deadlock, leak, use-after-free, or initialization defect was substantiated.
- Configuration / compatibility / parallel paths:
enable_file_scanner_v2=falseis retained in the phase-two runtime state but ignored by the new dispatch, allowing a V1 phase one to cross into V2. The older absent-option case has the same BE behavior. Unsupported connector/table-format paths still fall back to V1. - Data correctness / special cases: Strict ID validation, Split/Row-Group/stripe coordinates, duplicate restoration, schema/default/partition materialization, and ORC one-row seek parity were checked; no additional wrong-row issue was confirmed. This is a read-only path with no transaction, persistence, storage-format, or data-write change.
- Performance / observability: A sparse Parquet request can start full projected Column-Chunk FileCache prefetch, while that dry-run traffic bypasses query reader/cache counters.
- Tests: The two added direct-reader tests are statically coherent but do not cover end-to-end dispatch, false/unset rollout behavior, cached-remote byte selectivity, partial Splits, or richer projections. Per the review contract I did not run builds or tests. Current CI shows compile/style checks passing while BE UT, external/regression, and performance jobs are still pending; those are CI evidence, not independent execution.
- Focus / completeness: No additional user focus was supplied. Two complete review rounds converged with every Round 2 normal and risk-focused reviewer returning
NO_NEW_VALUABLE_FINDINGS; every candidate was accepted, deduplicated, or dismissed before submission.
Contributor
TPC-DS: Total hot run time: 80928 ms |
Contributor
ClickBench: Total hot run time: 14.43 s |
…ication ### What problem does this PR solve? Related PR: apache#67906 Problem Summary: External row-id fetches ignored the scanner rollout option, and sparse Parquet requests could prefetch complete projected chunks. Reuse the phase-one selection policy and bypass whole-chunk prefetch and range merging for explicit row selections. ### Release note Honor enable_file_scanner_v2 during external row-id fetches and avoid whole-chunk Parquet cache prefetch for sparse row selections. ### Check List (For Author) - Test: 280 focused BE unit tests passed under ASAN using run-be-ut.sh; clang-format 16 and build hygiene passed. Clang-tidy was blocked by pre-existing NOLINT marker errors. - Behavior changed: Yes, disabled or absent V2 options retain V1 and row-id reads avoid whole-chunk warm-up. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
### What problem does this PR solve? Related PR: apache#67906 Problem Summary: Hive ORC TopN queries failed when second-phase projections contained partition columns. Rebuilt is_file_slot values lacked Thrift presence bits, so Hive V2 treated partition columns as physical columns and exhausted column_idxs. Build the fetch projection with Thrift setters and cover its interaction with the Hive positional mapper. ### Release note Fix Hive TopN row-id fetches with partition columns and positional column mapping. ### Check List (For Author) - Test: New unit test reproduced the failure before the fix; 281 focused BE unit tests passed under ASAN after it. Clang-format 16 and build hygiene passed. Clang-tidy is blocked by pre-existing NOLINT marker errors. External SQL validation is pending CI. - Behavior changed: Yes, partition columns no longer consume physical file indexes in second-phase scans. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
There was a problem hiding this comment.
Static review of exact head df828379c4d947648202b01ef563a1b48227dd70: 1 blocking finding, attached inline.
Critical checkpoint conclusions:
- Goal / proof: The PR adds FileScannerV2 phase-two row-ID fetching for supported Parquet/ORC paths, but the rebuilt projection loses connector metadata semantics. The direct reader, rollout, and cached-remote tests do not prove lazy synthesized-column behavior.
- Scope / reuse: The change is focused and reuses
TableReader, native readers, selected-range, and seek abstractions; the gap is at the cross-phase projection boundary. - Concurrency: Remote-scheduler tasks operate per file. Shared status/completion publication is synchronized, scan blocks/statistics are disjoint by index, heavy I/O stays outside the completion lock, there is no multi-lock order, and every accepted task is awaited. No distinct race or deadlock was found.
- Lifecycle: Task memory-tracker attachment, RuntimeState ownership, error exits, and scanner/reader RAII were traced. No leak, cycle, use-after-free, or cross-TU static-initialization issue was substantiated.
- Configuration: No configuration is added. The existing
enable_file_scanner_v2presence/value policy is now reused for phase two, including false/absent fallback. - Compatibility: There is no storage-format, persistence, or public-symbol change. Unsupported table/format paths retain V1 fallback; no separate rolling-upgrade issue was found.
- Parallel paths: V1/V2, Parquet/ORC, and Hive/Iceberg/other external-table hooks were compared. The existing rollout and Parquet-prefetch threads are hard duplicate fences; no other distinct parity defect survived review.
- Special conditions: Row-ID order/range/format validation and duplicate scatter are coherent. The synthesized-category condition is not, which is the inline finding.
- Test coverage: Added unit coverage exercises direct Parquet/ORC reads, helper reconstruction, rollout, and cached-remote sparse Parquet I/O. It lacks an end-to-end TopN lazy Iceberg
_file/_postest and corresponding negative coverage. - Test results: Per the review contract, I ran no builds or tests. Visible CI/style results and the author's reported focused ASAN tests are external evidence only, not independent execution.
- Observability: Existing status context and profile counters are adequate for this read path; no new log or metric requirement was identified.
- Transactions / persistence: Not applicable; this is a read-only scan path with no EditLog or failover state.
- Data writes: Not applicable; there is no write, atomicity, or crash-consistency change.
- FE-BE transport: The phase-two slot projection is reconstructed across the RPC boundary, but it does not retain
TColumnCategory; this is the blocking defect. - Performance: The sparse Parquet prefetch issue already raised inline is fixed on this head, and ORC's remaining merge behavior matches V1. No additional CPU, memory, or I/O regression was established.
- Other correctness: Status propagation, strict row-count checks, memory ownership, and nullable handling were inspected. One distinct P1 remains after all other candidates were deduplicated or dismissed.
Two complete review rounds converged with all Round 2 tracks returning NO_NEW_VALUABLE_FINDINGS.
| const auto& slot = scan_slots[slot_idx]; | ||
| const auto column_idx = scan_column_idxs[slot_idx]; | ||
| TFileScanSlotInfo slot_info; | ||
| slot_info.__set_slot_id(slot.id()); |
Contributor
There was a problem hiding this comment.
[P1] Preserve synthesized slot categories in the phase-two request
An Iceberg metadata column such as _file or _pos can be selected as a TopN lazy slot (reduced path inferred from the code: Materialize(lazy=[_file]) -> TopN -> IcebergFileScan(id, GLOBAL_ROWID)). This rebuild creates every non-partition slot with only is_file_slot=true. On the V2 path, the missing SYNTHESIZED category makes _build_projected_columns() mark the slot as physical; TableColumnMapper then skips the virtual _file/_pos mapping and materializes a missing-column default instead of the metadata value. Please carry the original category through the phase-two request/rebuild, including slots pruned from phase one, derive column_idxs from it, and add a lazy metadata-column regression test.
Contributor
Author
There was a problem hiding this comment.
Fixed in 32d20cb. FE now retains the pinned full schema's non-regular column categories in the scan parameters, including lazy columns absent from phase one's tuple. BE restores those categories when rebuilding the fetch projection and includes only REGULAR/GENERATED slots in column_idxs. An explicitly empty category map preserves ordinary physical columns with metadata-like names.
The fetch also preserves original Iceberg file paths and row-lineage metadata when removing delete files, using a local range copy so shared FileMapping entries are not modified.
Both new BE regressions failed before their respective fixes. Validation: 283 focused ASAN tests and 16 targeted FE tests passed; the FE run required a temporary exclusion of an unrelated pre-existing IVM test compilation error. Added eager-versus-lazy _file/_pos comparisons for Parquet and ORC, with a materialization-plan assertion. External SQL validation is pending the newly requested CI run.
Contributor
TPC-H: Total hot run time: 16816 ms |
Contributor
TPC-DS: Total hot run time: 81513 ms |
Contributor
ClickBench: Total hot run time: 14.81 s |
### What problem does this PR solve? Related PR: apache#67906 Problem Summary: Rebuilding a second-phase projection discarded synthesized slot categories, so lazy Iceberg metadata columns were treated as missing physical fields. Preserve non-regular categories from the pinned full schema, including slots pruned from phase one, and derive physical column indexes from those categories. Retain original file paths and row-lineage metadata when removing delete files, using a local range copy to avoid modifying shared file mappings. ### Release note Preserve Iceberg metadata values in TopN lazy row-id fetches. ### Check List (For Author) - Test: 283 focused BE ASAN tests and 16 targeted FE tests passed. FE validation required a local exclusion of an unrelated pre-existing IVM test compilation error; the exclusion was restored. Added Parquet/ORC lazy metadata regression coverage; external SQL validation awaits CI. Clang-format 16, FE Checkstyle, and build hygiene passed. Clang-tidy is blocked by pre-existing NOLINT marker errors. - Behavior changed: Yes, lazy metadata columns retain their virtual values and original file metadata. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
One blocking correctness issue remains.
Critical checkpoint conclusions:
- Goal and scope: The PR adds standalone FileScannerV2 row-id fetch for external Top-N lazy materialization. The FE/BE/format/test changes are appropriately focused, but the rolling-upgrade gap below prevents the feature from being correct across supported mixed versions.
- Concurrency and lifecycle: The scheduled fetch tasks, semaphore, mutex/condition/status coordination, accepted-task wait, scanner state initialization, cleanup, and error propagation were traced; no additional actionable issue was found.
- Configuration and compatibility: No new configuration is introduced. Reusing the existing V2 option is unsafe when the new optional field 38 is absent, as described inline.
- Parallel paths and conditions: V1 fallback, Parquet/ORC, reachable Iceberg/Hive paths, and Paimon/Hudi/system-table capability gates were checked. Current sparse-prefetch and synthesized-column fixes address their same-version paths; the missing field-presence condition remains.
- Tests and results: The new unit/FE/regression coverage exercises current-version behavior and deterministic eager-versus-lazy equality, but it does not cover an old-FE payload without field 38 on a new BE. Per the review contract, I did not build or run tests. The PR discussion reports 283 focused BE ASAN tests and 16 FE tests passing, with the external SQL regression pending.
- Observability and performance: Phase-two I/O/time counters and sparse-read attribution are present; no further actionable issue was found.
- Transactions, persistence, and writes: Not applicable; this is a read-only scan path with no EditLog, storage-format, or write-path change.
- FE/BE contract: The current FE producer and BE consumer for field 38 are wired consistently, but absence of the optional field is not negotiated safely.
Focus: No additional user-provided review focus was supplied.
Completion: Static review converged in two rounds. All normal full-review and dedicated risk-review passes returned NO_NEW_VALUABLE_FINDINGS in the final round. Existing inline threads were treated as duplicate fences.
Contributor
TPC-H: Total hot run time: 16723 ms |
Contributor
TPC-DS: Total hot run time: 81952 ms |
Contributor
ClickBench: Total hot run time: 16.49 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
yiguolei
approved these changes
Sep 14, 2026
924060929
approved these changes
Sep 14, 2026
yiguolei
pushed a commit
that referenced
this pull request
Sep 15, 2026
…cannerV2 #67906 (#67922) ### 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: - Preserve the existing Lance dataset-level uint64 row-ID fetch path, physical split scheduling, condition-cache state, and Variant projections. - Apply name-based column classification to the existing IcebergScanNode and keep relation-snapshot schema categories separate from physical column positions. - Use the existing Iceberg v3 row-lineage regression suite for eager/lazy comparisons; the master-only `_file`/`_pos` feature 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) - FE: `FileQueryScanNodeTest` passed (14 tests, zero failures/errors); Maven validate passed with zero Checkstyle violations. - BE: 274 related ASAN unit tests passed across RowIdStorageReader, FileScanner, FileScannerV2, Parquet, and ORC. clang-format 16 passed for all 20 affected C++ files. - Regression evidence: all five focused cases failed before the fixes and passed afterward. A 1,024-row sparse selection now needs 8 output batches at a 128-row cap; a one-row fetch over 20 flat columns retains about 1.53 MiB of stream buffers instead of 160 MiB with default settings. - Validation limitation: the external Iceberg regression suite was not run locally. - Behavior changed: Yes. Supported TopN second-phase fetches use FileScannerV2 when enabled. - Does this need documentation: No.
4 of 16 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
FileScannerV2 did not support selective reads by absolute file row position. As a result, TopN two-phase materialization still had to use the legacy FileScanner for its second-phase Parquet/ORC fetch.
This change carries the selected row positions through TableReader and FileScanRequest, builds selective Parquet row ranges, seeks requested ORC rows, and routes supported second-phase fetches through FileScannerV2. Table-format variants that V2 does not support retain the existing V1 fallback. Phase two reuses the phase-one scanner selection policy, including the option presence bit, so disabling or omitting
enable_file_scanner_v2retains V1. Explicit row-ID requests bypass whole-chunk Parquet cache prefetch and range merging. Rebuilt fetch projections preserve Thrift slot presence bits and the pinned full schema's non-regular column categories, including slots pruned from phase one. Hive partition columns and synthesized metadata columns do not consume physical column indexes. Fetches retain original Iceberg file paths and row lineage while removing delete files, without mutating shared file mappings.Release note
Support row-id fetch for Parquet and ORC in FileScannerV2.
Check List (For Author)
ParquetScanTest.ReadsOnlyRequestedAbsoluteFileRowsAcrossRowGroupsNewOrcReaderTest.ReadsOnlyRequestedAbsoluteFileRowsRowIdStorageReaderTest.ExternalScannerSelectionRespectsRolloutOptionRowIdStorageReaderTest.ExternalScannerSelectionKeepsUnsupportedFormatsOnV1ParquetScanTest.SparseRowIdsAvoidCachedRemoteChunkPrefetchRowIdStorageReaderTest.ExternalFetchPartitionSlotsPreserveHivePositionMappingRowIdStorageReaderTest.ExternalFetchPreservesPrunedMetadataCategoriesRowIdStorageReaderTest.ExternalFetchPreservesIcebergFileMetadataFileQueryScanNodeTest.testRowIdFetchRetainsCategoriesOfPrunedColumnstest_iceberg_file_metadata_columnsfor Parquet and ORC.run-be-ut.sh; 16 targeted FE tests passed after temporarily excluding an unrelated pre-existing IVM test compilation error. Clang-format 16, FE Checkstyle, and build hygiene passed. Clang-tidy was blocked by pre-existing unmatchedNOLINTENDdiagnostics. Full SQL integration tests were not run locally.Check List (For Reviewer who merge this PR)