[improvement](be) Implement native Parquet decoding for FileScannerV2#65674
Open
Gabriel39 wants to merge 14 commits into
Open
[improvement](be) Implement native Parquet decoding for FileScannerV2#65674Gabriel39 wants to merge 14 commits into
Gabriel39 wants to merge 14 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review completed. I found one issue in the FileScannerV2 Parquet reader changes: persistent scratch now extends Arrow binary chunk lifetimes after materialization/discard.\n\nValidation: static review only. git diff --check c8b81647776bcd482b99665530f9fd5ba41f0d03...0826614aff1d8bf5e21b78f8c8c5a0acfcb465de passed. I did not run build or unit/regression tests in this review-only checkout.
Contributor
TPC-H: Total hot run time: 29482 ms |
Contributor
TPC-DS: Total hot run time: 176989 ms |
Contributor
ClickBench: Total hot run time: 25.08 s |
Gabriel39
marked this pull request as ready for review
July 16, 2026 00:14
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 16, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: FileScannerV2 retained Arrow binary and dictionary chunks in persistent Parquet reader scratch after synchronous Doris materialization. Large binary batches could therefore stay alive until a later read and overlap the next Arrow builder allocation. Release batch payload ownership on every success and error path while retaining vector capacity, clear borrowed StringRef scratch before its owners are released, and add unit coverage for ownership release and capacity reuse. Also remove the previous decoder changes under the legacy format directory so the v1 implementation remains unchanged; future native decoder work is constrained to format_v2. This lifecycle fix addresses the review finding but does not claim to eliminate the separate complex-column temporary materialization overhead. That v2-only decoder/materializer redesign remains distinct from COUNT(nullable_col) pushdown. ### Release note None ### Check List (For Author) - Test: Unit Test - Remote ASAN BE build - 16 focused Parquet v2 BE unit tests - Remote clang-format check - Remote clang-tidy attempted; blocked by existing diagnostics and the remote toolchain missing stddef.h during analysis - Behavior changed: No; internal buffer ownership is released earlier - Does this need documentation: Yes; updated the FileScannerV2 Parquet design document and format_v2 review guide
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary
Changes requested for one P1 memory-lifecycle regression. The allocation-reuse work is otherwise coherent, and the earlier Arrow-chunk ownership issue is fixed at the current head, but persistent nested scratch still lacks a bounded high-water release policy. See the inline comment.
Checkpoint conclusions
- Goal and proof: The PR reduces FileScannerV2/Parquet allocation and materialization overhead and hardens several reader boundaries. The changed tests cover logical reset, object/capacity reuse, Arrow-owner release, dictionary validation, column mapping, and split Row Group accounting, but they do not prove that oversized nested scratch is released; that missing property is the finding.
- Scope: The scanner, mapper, reader, statistics, Iceberg, tests, and design/guidance changes are focused on the stated optimization/hardening work.
- Concurrency: Row Group reader trees are uniquely owned and invoked serially by the scanner; no new shared globals, locks, atomics, lock ordering, or reentrant access was found.
- Lifecycle: Arrow chunks and borrowed
StringRefs now unwind safely on success/error paths, resolving the existing thread. However, Doris-owned nested value/null/conversion/level/index backing allocations remain until Row Group teardown; the inline P1 covers this distinct defect. - Configuration: No configuration items or dynamic-reload behavior are added.
- Compatibility: No storage-format, wire-protocol, symbol, or rolling-upgrade contract changes were found. Required-slot mapping is populated from the scan tuple for finalized and late-filter expressions, so removing the invalid ordinal fallback is safe.
- Parallel paths: Parquet, ORC, text, JSON/native layout consumers continue to use stable
local_positions; Scanner retains residual evaluation after lazy materialization. Dictionary, selection/page-skip, nested values/levels-only, List/Map/Struct, and Iceberg delete paths were also traced. - Conditions and errors: New slot/dictionary/selection validation returns checked errors; malformed partial reader state is terminal rather than retried. No speculative defensive continuation was introduced.
- Test coverage: Unit coverage is broad for the changed local behaviors, including negative dictionary/selection cases, but needs the oversized repeated-value retained-allocation regression test requested inline.
- Test results: No generated expected-result file changed. This review was static-only because the review contract prohibits builds in this runner.
- Observability: Existing Status context and profile counters are sufficient for these local changes; Row Group candidate totals are corrected per Split and no new critical metric/log path is required.
- Transactions and persistence: Not applicable; no transaction, EditLog, replay, or persistent metadata behavior changes.
- Data writes: Not applicable; scan-side decoding and equality-delete filtering do not alter write atomicity or crash recovery.
- FE/BE variables: No new transmitted variable or protocol field is introduced.
- Performance: Normal-size scratch reuse removes hot-path allocations, but child-count-unbounded nested buffers can accumulate outlier high-water capacity and cause a new memory-limit failure. Other reviewed CPU/I/O/allocation paths showed no substantiated regression.
- Other issues / user focus: No additional review focus was supplied. After three normal plus risk-focused convergence rounds, no second valuable finding or unresolved suspicious point remains.
Validation state
The live head is 913123e6cfeaa5712fd9571006a92e87f177f5a7. The author reports a remote ASAN build and focused unit tests, but I did not independently run them. Current formatter, license, FE UT, and Cloud UT checks pass. COMPILE, BE UT, and performance fail during CI setup because this head conflicts with current master in be/test/format_v2/parquet/parquet_reader_control_test.cpp; those jobs did not reach compilation/tests, and the PR is currently marked conflicting.
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FileScannerV2 currently materializes Parquet values through Arrow and repeatedly allocates conversion and nested-column scratch. Introduce an Arrow-independent physical column schema and selection-aware flat decode contract in the existing Doris Parquet kernel, batch string materialization, retain scalar and complex-reader scratch across batches, and document the target shared-level-plan interface for native complex decoding. The legacy v1 production call path remains unchanged.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- Remote ASAN BE build
- 120 targeted Parquet BE unit tests
- Behavior changed: No; this adds migration interfaces and internal allocation optimizations
- Does this need documentation: Yes; updated the FileScannerV2 Parquet design document and format_v2 review guide
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: FileScannerV2 retained Arrow binary and dictionary chunks in persistent Parquet reader scratch after synchronous Doris materialization. Large binary batches could therefore stay alive until a later read and overlap the next Arrow builder allocation. Release batch payload ownership on every success and error path while retaining vector capacity, clear borrowed StringRef scratch before its owners are released, and add unit coverage for ownership release and capacity reuse. Also remove the previous decoder changes under the legacy format directory so the v1 implementation remains unchanged; future native decoder work is constrained to format_v2. This lifecycle fix addresses the review finding but does not claim to eliminate the separate complex-column temporary materialization overhead. That v2-only decoder/materializer redesign remains distinct from COUNT(nullable_col) pushdown. ### Release note None ### Check List (For Author) - Test: Unit Test - Remote ASAN BE build - 16 focused Parquet v2 BE unit tests - Remote clang-format check - Remote clang-tidy attempted; blocked by existing diagnostics and the remote toolchain missing stddef.h during analysis - Behavior changed: No; internal buffer ownership is released earlier - Does this need documentation: Yes; updated the FileScannerV2 Parquet design document and format_v2 review guide
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: FileScannerV2 materialized ordinary Parquet values through Arrow arrays and temporary nested batches, which added allocation, conversion, and peak-memory overhead compared with the v1 reader. Route ordinary predicate and output columns through a persistent native reader that decodes directly into Doris columns, preserves selection and page-index coordinates, shares v1-compatible footer/page caches and MergeRange I/O, and keeps Arrow only for metadata planning, dictionary probing, and the existing COUNT complex levels-only path. Add detailed native decode profiles, adaptive-batch fragmentation profiles, complex/string/decimal/fixed-binary coverage, and update the Parquet design and review guidance. No FE or COUNT pushdown behavior is changed, and no be/src/format decoder code is modified. ### Release note Improve FileScannerV2 Parquet scan memory use and native decode performance for scalar and complex columns. ### Check List (For Author) - Test: Unit Test - 192 format v2 Parquet BE unit tests on the designated remote host - Behavior changed: Yes (ordinary FileScannerV2 Parquet values use native direct materialization; query results and pushdown semantics are unchanged) - Does this need documentation: Yes (design and review guide updated in this change)
### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#65674 Problem Summary: The Parquet v2 native scan path no longer uses the prototype Arrow value-reader hierarchy, but its decoded leaf batch, nested load/build/consume protocol, factories, and tests remained reachable from a shape-only aggregate path. Remove that intermediate layer and isolate the unchanged COUNT(nullable_col) level-reading compatibility path behind a narrow API. Ordinary scans now expose only the persistent native read/skip/select contract. ### Release note None ### Check List (For Author) - Test: Unit Test (remote validation pending) - Behavior changed: No - Does this need documentation: Yes (updated in this commit)
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: FileScannerV2 still paid for an Arrow-decoded value layer or decoder-owned logical conversion before constructing Doris columns. This duplicated buffers for strings and nested values, amplified adaptive-batch memory peaks, and made dictionary, page-cache, and decode profiles difficult to compare with the v1 reader. Introduce an independent Parquet page/encoding reader under format_v2 whose decoders only parse encoded streams and own cursors. DataTypeSerDe now interprets Parquet physical and logical annotations and materializes selected values directly into persistent Doris columns. The native reader supports the existing scalar, decimal, date/time, timestamp, UUID, dictionary, delta, byte-stream-split, Page V1/V2, nested level, selection, page-index, footer/page-cache, and MergeRange paths. COUNT complex columns use a levels-only reader with persistent scratch, and page/decode/cache profiles expose cumulative deltas without duplicate counting. Arrow remains only in metadata/index planning and test fixture generation. No FE code or v1 format decoder is modified. ### Release note Improve FileScannerV2 Parquet scan memory use and native decode performance, especially for strings, decimals, nested columns, dictionary selection, and adaptive batch sizes. ### Check List (For Author) - Test: Unit Test - 14 Parquet decoder and DataTypeSerDe tests on the designated remote host - 76 Parquet v2 reader, selection, page-index, complex-column, and scan tests on the designated remote host - 6 focused COUNT/profile tests on the designated remote host - Behavior changed: Yes (FileScannerV2 Parquet data pages materialize through the native v2 decoder and DataTypeSerDe; query semantics are unchanged) - Does this need documentation: Yes (updated the FileScannerV2 Parquet design and review guide)
Gabriel39
force-pushed
the
codex/fix-file-scanner-v2-native-decode
branch
from
July 16, 2026 11:36
913123e to
78678d9
Compare
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: Split the Parquet v2 decoder factory by encoding family and replace raw allocations with std::make_unique. This keeps decoder construction behavior unchanged while satisfying function-size and ownership static-analysis rules. ### Release note None ### Check List (For Author) - Test: Unit Test - Remote ASAN BE build and 90 Parquet v2/SerDe unit tests - Remote clang-tidy on the changed decoder factory - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: Persistent Parquet v2 leaf readers reused level, null, selection, conversion, and dictionary-index scratch across batches. A legal repeated column can expand one logical batch into an exceptional number of leaf values, so staggered outliers could retain large allocations in multiple leaves until Row Group teardown. Keep normal-size capacity reusable, recursively release individual scratch buffers above a 4 MiB high-water mark after the top-level complex reader consumes the level plan, and cover the retained-capacity drop with a unit test. ### Release note None ### Check List (For Author) - Test: Unit Test - Remote ASAN ./build.sh --be - Remote Parquet v2 and SerDe unit-test suite - Behavior changed: No - Does this need documentation: Yes. Updated the FileScannerV2 Parquet design and review guide.
Gabriel39
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review summary
Changes requested for 15 new, non-duplicate issues in the native Parquet reader. They cluster in three boundaries: shared footer/page-cache identity and immutability; exact decoder/page/level validation; and ARRAY/MAP/STRUCT shape preservation. The earlier Arrow-chunk ownership issue is fixed, and the existing nested-scratch high-water P1 remains active without being duplicated here.
Checkpoint conclusions
- Goal and proof: The PR substantially implements native decoding and removes the old intermediate value path, but the current tests do not prove safe behavior for valid Page V1 BIT_PACKED levels, cache-order/version changes, decoder reuse, or several malformed-page/nested-shape cases covered by the inline findings.
- Scope: The production, test, and design changes are focused on the stated FileScannerV2 Parquet migration, though the migration is broad enough that cache, decoder, selection, COUNT, and complex-column boundaries all needed end-to-end tracing.
- Concurrency: Concurrent scans can share a cached
FileMetaData; post-publicationassign_ids()mutates it without synchronization. No other new lock-order or thread-entry issue was substantiated. - Lifecycle: Footer metadata is both option-dependent and mutated after publication; native S3 page keys lose the split's stable version; the reused delta-length decoder keeps stale/uninitialized state on empty pages. The existing nested-scratch retention thread remains separate.
- Configuration: No new configuration item is added, but existing VARBINARY/TIMESTAMPTZ mapping options affect parsed schema and are missing from the shared footer-cache identity.
- Compatibility: No storage or wire format is changed, but valid Page V1 BIT_PACKED definition/repetition levels are not supported by the non-bulk cursor paths, so existing files can fail or execute undefined behavior.
- Parallel paths: Page V1/V2, compressed/uncompressed and cache-hit reads, selected/filtered/skipped values, normal nested scans and COUNT, v1/v2 footer-cache producers, and MAP/STRUCT sibling paths were checked. The findings identify the parallel instances that need the same exact validation.
- Conditions and errors: Several decoder return values/counts are discarded, signed external sizes reach unsafe arithmetic/buffers, a malformed BYTE_STREAM_SPLIT payload reaches
DORIS_CHECK, and release-onlyDCHECKs stand in for required corruption statuses. - Test coverage: Happy-path coverage is broad, but negative tests are missing for truncated BOOLEAN/delta/level streams, page-size and V2-level layout corruption, empty reused delta-length pages, null/misaligned MAPs, short siblings, leading continuations, and cache-order/version behavior. A concurrent same-footer cache test and valid BIT_PACKED Page V1 tests are also needed.
- Test results: No generated regression
.outfile changed. The author reports remote ASAN build/focused-unit/format validation in the PR history; I did not independently run builds or tests under this review contract. - Observability: No new metric is required for these local failures; checked corruption statuses with file/column/page context are preferable to silent success, undefined behavior, or process termination.
- Transactions and persistence: Not applicable; this is scan-side parsing/decoding and cache state, with no EditLog, replay, or transaction change.
- Data writes: Not applicable; no table-data write, publish, or crash-recovery path is modified.
- FE/BE variables: No new FE-to-BE transmitted variable or protocol field is introduced.
- Performance: The native/materialization design removes intended allocations and copies, but correctness and bounded lifecycle must hold first. The existing P1 already covers retained outlier scratch; no additional non-duplicate performance regression was found.
- Other issues / user focus: No extra user focus was supplied. After three normal/risk convergence rounds, the 15 inline findings below are the complete current non-duplicate set.
Validation state
The reviewed live head is d257ca06abff17c3a43f6f287594dc8dcf52ef80. This was static review only: the supplied contract prohibits builds, and this checkout has no .worktree_initialized, thirdparty/installed, or thirdparty/installed/bin/protoc.
Contributor
TPC-H: Total hot run time: 29697 ms |
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: Highly selective Parquet v2 scans entered DataTypeSerDe and the encoding decoder once per selection run. Fragmented selections therefore amplified virtual dispatch, consumer construction, and cursor-management overhead. Add a page-fragment hybrid selection plan that enters SerDe once and lets each native encoding batch gather or compact selected values while preserving and validating the full physical cursor. Add profile counters, decoder coverage, scratch retention bounds, and design/review documentation. ### Release note None ### Check List (For Author) - Test: No test run (per user request) - Behavior changed: Yes (sparse Parquet v2 materialization is batched per page fragment) - Does this need documentation: Yes (included in this PR)
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requested changes: six new, deduplicated correctness and compatibility issues remain in the native Parquet-reader replacement.
Checkpoint conclusions:
- Goal, scope, and tests: the native page/encoding/SerDe/scheduler/COUNT/cache architecture is broadly implemented and extensively tested, but behavioral parity is incomplete at the six inline boundaries below. Per the review contract, this was a static review only; I did not build or run tests. Missing focused coverage maps directly to INDEX_PAGE, PARQUET-816, pre-Arrow-3 V2 compression, non-strict nullable overflow, malformed nonempty OffsetIndexes, and invalid signed chunk ranges.
- Concurrency, lifecycle, configuration: reader/decoder/scheduler state remains scan-local; shared cache/profile facilities keep established synchronization and teardown. No new thread, lock-order, static-initialization, lifecycle, or dynamic-config issue survived deduplication.
- Compatibility and parallel paths: flat/nested, indexed/sequential, scalar/COUNT, dictionary/plain, V1/V2, cold/warm cache, direct/fallback conversion, predicate/lazy-output, and selected-range paths were checked. Findings 1-4 regress prior valid-file or scan semantics; findings 5-6 regress corruption boundaries and can return wrong data or cross chunk boundaries.
- Error, memory, data correctness, and nullable handling: other inspected Status and bounds paths fail loudly. The unvalidated OffsetIndex can silently replay rows, the wrapped chunk range widens IO ownership, and direct materialization loses non-strict failure-to-NULL semantics. Existing MAP/STRUCT/decoder comments were not duplicated.
- Observability and performance: the new profile hierarchy and counters cover the critical pipeline. Existing comments already own the substantiated allocation, scratch, cache, and complexity issues; no distinct additional metric or performance finding remained.
- Persistence, writes, and FE/BE: this is a read-only external-file path with no Doris transaction, EditLog, persisted-format, data-write, or new FE-BE protocol change. The relevant existing BE session state, strict mode, becomes write-only as noted inline.
- User focus: no additional focus was supplied; the whole PR was reviewed.
All 130 changed paths and all 37 existing inline comments were re-swept. Two normal full-review agents and the risk-focused agent converged on this exact six-comment state with NO_NEW_VALUABLE_FINDINGS.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Found three blocking correctness gaps in the native Parquet path:
- INT96 direct materialization uses unchecked Julian-day/nanos arithmetic and can append an invalid timestamp while returning success.
- TIMESTAMPTZ direct conversion neither checks millisecond scaling nor the 0001-9999 target range, including the decoded/statistics path.
- OffsetIndex validation does not bind location zero to the metadata's first data page, allowing a shifted but monotonic index to skip and relabel page values.
Checkpoint summary:
- Goal/scope and compatibility: the migration is coherently scoped, and legacy-writer/page-cache fixes plus PLAIN/dictionary/selection/count/nested paths were reviewed and deduplicated against existing threads; the three issues above remain.
- Concurrency/lifecycle/configuration: footer-cache publication, reader ownership/reset, cancellation/profile flushing, MergeRange/FileCache routing, and session options showed no additional issue.
- Parallel paths and tests: flat/nested, cold/warm cache, dictionary/plain, selected/skip, COUNT, alternate formats, and integration tests were covered. The inline comments identify missing boundary/corruption tests.
- Persistence/write/FE-BE boundaries: this is a read-only BE change; no transactional, persistence, or FE-variable issue applies.
- Performance/observability: adaptive scheduling, retained scratch, counters, and profile ownership showed no additional blocker.
Validation was static only, as the review task explicitly prohibited builds and test execution. No additional user-provided review focus was supplied.
### What problem does this PR solve? Issue Number: None Related PR: apache#65674 Problem Summary: Parquet INT96 conversion could overflow signed arithmetic or accept an invalid nanos-of-day, TIMESTAMPTZ millisecond scaling could overflow or materialize values outside years 0001-9999, and a uniformly shifted OffsetIndex could still pass monotonic range validation. Use a shared checked timestamp conversion for direct, dictionary, and decoded/statistics paths, preserve strict rollback and non-strict NULL propagation, and anchor the first OffsetIndex location to the owning data_page_offset. ### Release note None ### Check List (For Author) - Test: Unit Test - DataTypeSerDeParquetTest.* and ParquetV2NativeDecoderTest.* (60 tests) - Behavior changed: Yes (invalid Parquet timestamps are rejected or converted to NULL according to load strictness, and incoherent OffsetIndex metadata falls back to sequential traversal) - Does this need documentation: Yes (updated the review guide and Parquet scan design in apache#65674)
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review completed after three full-diff and exact-set convergence rounds. Requesting changes for five P1 issues: two direct-decimal correctness/compatibility regressions, missing level-domain validation, an unbounded nested-level allocation from page metadata, and unbounded retained lazy-skip bitmaps.
Checkpoint conclusions:
- Goal, scope, and tests: The native V2 decode replacement and associated cache/profile work broadly implement the intended goal and add extensive BE coverage, but the negative schema-evolution, malformed-level/page, and long-prefix resource cases in the inline comments are missing. The patch is not yet safe to merge.
- Concurrency and lifecycle: Persistent row-group readers, cache handles, MergeRange ownership, profile snapshot publication, and normal/error close paths were traced; no new lock, deadlock, use-after-free, or double-publication issue remains.
- Compatibility, parallel paths, and conditions: Plain and dictionary encodings, strict and nullable non-strict conversion, V1/V2 pages, flat/nested readers, scalar/COUNT paths, and cache hit/miss behavior were checked. No protocol, config, or storage-format change was introduced; the blocking compatibility gaps are the decimal direct paths called out inline.
- Tests, results, and observability: Added unit tests and profile counters are generally coherent, but the five negative/resource cases are absent. No generated-result issue was found. No build or tests were run because the review-runner contract requires static review only.
- Persistence, writes, and FE/BE variables: Not applicable; this is a BE read-path change.
- Performance and memory: The two allocation findings are blocking because valid selective scans or tiny malformed pages can cause memory use far beyond the requested batch.
- User focus: No additional focus points were supplied.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: I found three P1 correctness/nontermination issues and four P2 compatibility/profile-accounting issues in the native Parquet and FileScannerV2 integration. Existing review threads and all raw inline comments were deduplicated; the previously reported shifted MAP repetition issue is intentionally not repeated.
Critical checkpoint conclusions:
- Goal and tests: the native decoder/direct-materialization goal is implemented broadly, but the accepted page-cardinality, nested-progress, and projection failures show the current head does not yet preserve correctness for all supported inputs. Focused negative tests are missing for each accepted case.
- Scope and focus: this is a large but coherent BE reader replacement plus cross-format profile refactor, not a local change. No additional user focus was supplied, so the complete 125-file change set was reviewed.
- Concurrency: cache metadata/page-range publication and close/profile publication use mutex or at-most-once/delta protection; no new distinct race or lock-order issue survived review. No new thread entry is introduced.
- Lifecycle and static initialization: three P2 timer defects are reported below. No cross-TU static-initialization dependency or ownership cycle was found.
- Configuration: no configuration item is added, so dynamic-update behavior is not applicable.
- Compatibility: no FE/BE protocol or persisted-format change is introduced. One P2 comment covers rejection of a format-valid large V1 BIT_PACKED page; rolling-upgrade handling is otherwise not implicated.
- Parallel paths and conditions: V1/V2, indexed/sequential, selected/skip, plain/dictionary, nested/scalar, JNI/native, Iceberg delete, abort, EOF, and parallel format/profile paths were traced. Existing related issues were suppressed as duplicates.
- Error, memory, null, and data correctness: malformed cardinalities must fail instead of shifting rows or spinning; full nested projection must not default or fatal. Nullable/conversion, allocation, encoding, timestamp, and cache concerns already present in review context were not duplicated.
- Test coverage and results: the changed tests are extensive but do not cover the seven reported failures. This review was static-only by contract; no local build or tests were run. The PR body reports remote ASAN/focused 91/91 validation through d257ca0 and explicitly says the later hybrid commit was not built/unit-tested, so that earlier result does not validate the full current head.
- Observability: the hierarchy is useful, but the reported late, omitted, and overlapping timers make key values unreliable until fixed. Other profile publication paths use snapshot/delta or at-most-once guards.
- Transactions, persistence, and data writes: not involved; no EditLog, transactional mutation, or crash-atomicity path changes.
- FE/BE variables: none added or transmitted.
- Performance: native/direct decoding and bounded scratch target the intended CPU/memory improvements; no additional substantiated performance defect survived beyond existing threads.
- Other issues: all suspicious points have a submitted, duplicate, or evidence-backed dismissed conclusion, and three reviewers converged on this exact seven-comment set.
Validation: static review only; no local build or tests were run.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29840 ms |
Contributor
TPC-DS: Total hot run time: 178306 ms |
Contributor
ClickBench: Total hot run time: 25.17 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29480 ms |
Contributor
TPC-DS: Total hot run time: 177428 ms |
Contributor
ClickBench: Total hot run time: 25.12 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes. I found three new issues in the native Parquet path: two P1 optional-index boundary failures (an unbounded serialized-index allocation and false-negative IS NULL pruning from contradictory null metadata) plus one P2 cache-disabled V2 level-copy regression.
Checkpoint summary:
- Goal, scope, and tests: the Arrow-to-native materialization goal is coherent and the PR adds broad SerDe, decoder, nested-reader, scheduling, cache, compatibility, and profile coverage. The three inline cases still need focused fallback/correctness/performance tests.
- Correctness and compatibility: I checked flat/nested, plain/dictionary, indexed/sequential, selected/skipped, page-crossing, strict/non-strict, old-writer, and compressed/uncompressed V2 paths. Existing cardinality, decoder, decimal, timestamp, and projection findings were deduplicated rather than repeated.
- Concurrency, lifecycle, and observability: shared metadata/cache publication and cleanup/profile paths were reviewed. No additional distinct concurrency issue survived; the current lifecycle/profile threads remain applicable.
- Configuration and performance: cache enable/disable and mapping/compatibility options were traced. The cache-disabled V2 copy and optional-index memory exposure are the new surviving performance/resource issues.
- Transactions, writes, and FE/BE variable propagation: not applicable to this read-path change.
- User focus: no additional focus was provided.
This was a static review as required by the runner contract; I did not build or run tests. All 134 authoritative changed paths were swept, and the independent full/risk reviews converged on this exact comment set with no further valuable findings.
|
|
||
| const int64_t span_offset = (*ranges)[range_begin].offset; | ||
| const int64_t span_length = span_end - span_offset; | ||
| std::vector<uint8_t> serialized(static_cast<size_t>(span_length)); |
Contributor
There was a problem hiding this comment.
[P1] Bound optional index reads before allocating them
Each individual footer range is accepted up to the integer limit when it merely fits inside the file, and adjacent ranges across projected leaves are merged here without an aggregate budget. A large object can therefore advertise one roughly-2-GiB index (or several adjacent ones) and make this optional pruning path allocate a multi-gigabyte std::vector before Thrift parsing can reject it; the OffsetIndex-only loader has the same whole-range allocation at line 793. Please cap individual and coalesced serialized-index bytes (using tracked storage where applicable) and conservatively skip the optional index above that budget, with large-length/adjacent-range fallback tests.
| return false; | ||
| } | ||
| page_statistics->has_null_count = true; | ||
| page_statistics->has_null = column_index.null_counts[page_idx] > 0; |
Contributor
There was a problem hiding this comment.
[P1] Reject contradictory ColumnIndex null metadata before pruning
The native loader checks the null_pages vector length but not these counts or their consistency. For a positive-row page with null_pages[i] = true and null_counts[i] = 0 (or a negative count), this produces a zone map with both has_null and has_not_null false. IS NULL then evaluates to kNoMatch, so an all-null page can be removed without evaluating its data. Please invalidate/fall back from the optional ColumnIndex on negative or contradictory null metadata, and add native PageIndex tests for IS NULL/IS NOT NULL with those cases.
| if (_block_compress_codec != nullptr) { | ||
| Slice compressed_data; | ||
| RETURN_IF_ERROR(_page_reader->get_page_data(compressed_data)); | ||
| std::vector<uint8_t> level_bytes; |
Contributor
There was a problem hiding this comment.
[P2] Avoid copying V2 levels when the page cannot be cached
This allocates and copies every repetition/definition byte before checking whether the session cache is enabled, storage caching is globally disabled, or a cache instance exists. The live decoders use _v2_rep_levels/_v2_def_levels slices into the page buffer; level_bytes is consumed only by _insert_page_into_cache. Thus cache-disabled nested V2 scans still pay one allocation, memcpy, and free per page. Please build this buffer only inside cache admission (or reuse bounded scratch only for an actual insertion), with a cache-disabled coverage hook/benchmark.
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
2 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?
Issue Number: None
Related PR: None
Problem Summary:
FileScannerV2 previously used Arrow RecordReader/arrays/builders for Parquet value materialization and reconstructed complex columns through intermediate decoded objects. That path duplicated decode/materialization work, retained fragmented per-batch scratch, amplified string and nested-column memory usage, and made adaptive batches and cache/profile behavior diverge from the legacy v1 reader.
This PR implements an independent native Parquet read path under
format_v2:HybridSelectionBatches,HybridSelectionRanges, andHybridSelectionNullFallbackBatchesfor diagnosis.be/src/formatv1 implementation unchanged.Arrow is now limited to the existing metadata/index/probe adapter boundary; it is not used to materialize scan values.
Release note
FileScannerV2 Parquet scans now use native Doris page/encoding decoding and direct DataTypeSerDe materialization instead of Arrow value readers.
Check List (For Author)
d257ca06abf: remote ASAN BE build passed; targeted BE UT passed 91/91; LLVM 16 format check passed.099c111c12fadds hybrid sparse decoding and decoder UT coverage. Build/UT validation was not run at the user's request; CI is requested below.docs/file-scanner-v2-parquet-scan-design.mdanddocs/file-scanner-v2-code-review-guide.md.