[improvement](parquet) Make V2 column initialization lazy#66011
Open
Gabriel39 wants to merge 4 commits into
Open
[improvement](parquet) Make V2 column initialization lazy#66011Gabriel39 wants to merge 4 commits into
Gabriel39 wants to merge 4 commits into
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: 22889 ms |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-DS: Total hot run time: 117342 ms |
Contributor
ClickBench: Total hot run time: 18.48 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 23440 ms |
Contributor
TPC-DS: Total hot run time: 120912 ms |
Contributor
ClickBench: Total hot run time: 19.07 s |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
yiguolei
previously approved these changes
Jul 24, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
approved
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 24, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66011 Problem Summary: Lazy dictionary discovery could return before inspecting the first data page. For nested V1 chunks, this allowed an unverified OffsetIndex to skip earlier physical pages and desynchronize logical progress. Continue through the first data-page header before indexed seeking, and add a unit test covering dictionary-prefixed nested V1 pages. ### Release note None ### Check List (For Author) - Test: Unit Test - ParquetV2NativeDecoderTest.* - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
github-actions
Bot
removed
the
approved
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29036 ms |
Contributor
TPC-DS: Total hot run time: 177174 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
I found one blocking data-correctness issue and am requesting changes.
Critical checkpoint conclusions:
- Goal and proof: The PR is intended to make native Parquet column-chunk initialization perform zero page I/O and defer dictionary/page setup. The new read-count tests demonstrate lazy initialization, but the implementation no longer establishes page zero before OffsetIndex row bounds are consumed, so the end-to-end goal is not yet correct.
- Scope and design: The patch is small and focused, and lazy setup remains inside the native reader boundary. MAIN-1 is a local state-contract regression rather than unrelated scope growth.
- Concurrency: These reader and cursor fields are single-reader, single-owner scan state; no new thread, lock, atomic, static initialization, or deadlock concern was found.
- Lifecycle and error handling: Dictionary, auxiliary-page, cache-hit, sequential, EOF, and error propagation paths remain coherent. The blocking lifecycle defect is the first indexed page: reconciliation and chunk-level validation can occur after a caller has already cached or acted on its old row range.
- Configuration and dynamic behavior: No configuration item is added or changed.
- Compatibility: No storage format or function-symbol change is introduced. However, malformed but structurally accepted external Parquet OffsetIndexes can now yield shifted rows instead of the previous corruption/fallback behavior, so compatibility-safe error semantics regress.
- Parallel paths and special conditions: Sequential scalar readers and the levels-only COUNT path parse before use; nested V1 explicitly discards the unsafe index. Flat indexed reads, direct fixed-width filtering, and nested Data Page V2 are covered by MAIN-1 and need the same lazy first-page establishment invariant.
- Tests and results: The added BE tests cover zero-I/O initialization, dictionary probing, indexed dictionary skipping, and nested-V1 behavior. Existing cardinality tests were changed to call
parse_page_header()explicitly, so they do not exercise a first operation that skips page zero or a scalar fallback after index reconciliation. Add those negative cases. I did not run builds or tests in this review-only runner; live PR checks currently report BE UT, compile, formatter, and the other completed suites green. - Observability and resources: Existing native-reader statistics, destructor flushing, page-cache accounting, ownership, and retained-scratch behavior remain intact; no separate observability or memory-accounting issue was found.
- Persistence, transactions, writes, and FE/BE propagation: Not applicable; this patch changes only BE Parquet read-side initialization and tests.
- Performance: Avoiding page I/O for unused readers is a valid optimization, and the added idempotent check is negligible. Correctness must be restored without giving up zero-I/O
init()by lazily establishing the first page before its indexed range is used. - Other issues: No additional actionable defect remained after two review rounds and a final full changed-file sweep.
User focus: no additional focus was supplied.
| _offset_index = nullptr; | ||
| } | ||
| } | ||
| _dict_checked = true; |
Contributor
There was a problem hiding this comment.
[P1] Establish page zero before consuming its indexed range
With lazy init, a flat reader can cache page_end_row() and decide to skip page 0 before this helper parses its header. That parse may either leave the index active without running ColumnChunkReader::parse_page_header()'s num_values/num_rows checks, or invalidate a physically inconsistent index after the caller has already cached its old row bound.
Both paths can shift rows. Indexed spans [0,2), [2,4), [4,5), [5,6) with physical page counts 1,2,1,2 let a scan selecting [2,4) return physical rows 1-2 as logical rows 2-3. Similarly, spans [0,1), [1,3) with physical counts 2,1 plus a first-rectangle size mismatch switch to sequential end 2, but the caller can still advance using cached end 1 and skip requested row 1. Before this PR, init() established page 0 before any page_end_row() use. Please preserve zero-I/O init(), but lazily reconcile and chunk-validate the first data header before consuming its indexed range, with flat and nested-V2 skip/fallback tests.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
No new actionable findings at head 94e2d85e5ab65638623aa134fdd74e9eb987e140. The previously reported page-zero OffsetIndex issue is addressed by establishing and validating the first physical data-page header before any indexed range is consumed, while preserving zero-I/O init().
Critical checkpoint conclusions:
- Goal and proof: The patch makes native Parquet column-chunk initialization perform no page I/O, defers dictionary/header discovery until first use, and preserves page-zero reconciliation before indexed reads, direct fixed-width filtering, nested seeks, and page advances. The added read-count, dictionary, flat fallback/cardinality, and nested V1/V2 tests exercise the intended changed behavior.
- Scope and design: The change is small and localized to the native Parquet reader state machine and its focused BE tests. Dictionary discovery, chunk-level cardinality validation, and page navigation remain in their owning reader layers.
- Concurrency: Reader and cursor state remains single-owner scan state. No new threads, locks, atomics, shared mutable globals, static initialization dependency, or deadlock risk is introduced.
- Lifecycle and error handling: No-dictionary, dictionary-first, auxiliary-page, page-cache, ordinary scalar, direct-filter, nested, levels-only COUNT, EOF, and failed-advance paths were traced. New status-returning calls are checked, successful transitions are idempotent, and wrapper state is committed only after page advance succeeds.
- Configuration and dynamic behavior: No configuration item is added or changed.
- Compatibility: No storage format, protocol, function symbol, FE/BE variable, or v1 reader behavior is changed. Active OffsetIndex cardinality checks and conservative sequential fallback remain explicit.
- Parallel paths and special conditions: Flat required/optional reads, direct fixed-width predicates, nested Page V1/V2, dictionary probing/filtering, auxiliary pages, cached headers, zero-value pages, fully skipped ranges, and levels-only COUNT were reviewed. No changed-path inconsistency remains.
- Tests and results: The changed BE tests were reviewed for coordinate domains, negative cases, multi-page state, and masking. I did not run builds or tests because this is a review-only runner. Current live checks show formatter, checkstyle, license, dependency, and secret checks passing; BE UT, compile, macOS BE UT, and performance were still pending at the final check.
- Observability and resources: Existing page/cache/profile counters, buffered I/O ownership, decoder state, and decompression scratch lifetime remain on the same paths; no new memory-accounting or observability gap was found.
- Persistence, transactions, and writes: Not applicable; this patch changes only BE Parquet read-side initialization and tests.
- Performance: Delaying the first header/cache/prefetch operation avoids I/O for unused readers, and the one-time lazy checks add negligible work once page state is established.
- Other issues: A later-page sequential-fallback concern was investigated and dismissed from this review because the authoritative removed code proves the identical behavior existed at the base and this PR neither introduces nor worsens it.
User focus: no additional focus was supplied; the full PR was reviewed and no additional PR-scoped issue was found.
Review status: complete after two convergence rounds, final changed-file and unresolved-candidate sweeps, and live duplicate-state verification.
Contributor
TPC-H: Total hot run time: 29284 ms |
Contributor
TPC-DS: Total hot run time: 177346 ms |
Contributor
ClickBench: Total hot run time: 25.13 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#66011 Problem Summary: Lazy dictionary discovery could return before inspecting the first data page. For nested V1 chunks, this allowed an unverified OffsetIndex to skip earlier physical pages and desynchronize logical progress. Continue through the first data-page header before indexed seeking, and add a unit test covering dictionary-prefixed nested V1 pages. ### Release note None ### Check List (For Author) - Test: Unit Test - ParquetV2NativeDecoderTest.* - Behavior changed: No - Does this need documentation: No
Lazy column-chunk initialization left OffsetIndex row bounds observable before the first physical page was reconciled and cardinality-checked. Establish page zero on first indexed use and cover flat reads, fixed-width filters, sequential fallback, and nested V2 seeks.
Gabriel39
force-pushed
the
agent/parquet-v2-lazy-dictionary-init
branch
from
July 26, 2026 03:21
94e2d85 to
6afebd6
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29399 ms |
Contributor
There was a problem hiding this comment.
Review conclusion: no new actionable findings at head 6afebd6eabf3c2f2353ec24bd32c5258dce9ef8f; approval opinion.
Critical checkpoint conclusions:
- Goal and proof: The change makes native Parquet column-chunk initialization perform no page I/O and defers dictionary/page setup until first use. Read-count tests cover the direct chunk and scalar dictionary-probe boundaries, while the added flat/nested tests exercise first-page validation and OffsetIndex fallback.
- Scope and design: The patch is small and localized to the native Parquet reader state machine and focused BE tests. Dictionary discovery, physical page validation, and typed dictionary ownership remain in their existing reader layers.
- Concurrency: Reader/page/decoder state is single-owner Row Group scan state. No new threads, locks, atomics, shared mutable globals, static initialization dependencies, or deadlock risks are introduced.
- Lifecycle and error handling: Dictionary/no-dictionary, auxiliary-page, page-cache-hit, ordinary, fixed-width, dictionary-ID, nested Page V1/V2, levels-only COUNT, EOF, and failed-advance paths were traced. PageReader reconciles physical rectangles before their row end is observed; ColumnChunkReader validates page cardinality before production decode/skip and before indexed advance. New Status results are propagated.
- Configuration and dynamic behavior: No configuration item is added or changed.
- Compatibility: No externally consumed ABI/protocol, storage-format contract, FE/BE variable, or v1 reader behavior is changed; the internal native-reader API change has all in-tree callers updated. Optional malformed OffsetIndexes either fall back sequentially or fail on inconsistent cardinality before cursor movement.
- Parallel paths and special conditions: The previously reported page-zero concern is addressed for ordinary reads, direct fixed-width filtering, indexed advances, nested seeks, and dictionary filtering's two-layer probe/consume path. No parallel-path omission remains.
- Tests and results: The changed tests cover zero-I/O initialization, lazy dictionary materialization, flat cardinality and rectangle mismatch handling, nested V1/V2 seeking/fallback, auxiliary pages, and malformed page extents. I did not run builds or tests in this review-only runner. The PR body reports 106 native-decoder tests from an earlier patch state; current-head BE UT/compile/performance checks were still pending during review.
- Observability and resources: Existing page/cache/decompression/dictionary counters and batch/Row Group/destructor flush paths continue to account for the deferred work. Decoder/dictionary/scratch ownership remains reader-scoped; no new memory-accounting issue was found.
- Persistence, transactions, writes, and FE/BE propagation: Not applicable; this is a BE Parquet read-side initialization change.
- Performance: Removing first-page I/O from unused reader initialization is a valid optimization; the one-time lazy guards add negligible work and preserve cache/prefetch ownership.
- Other issues: The only candidate raised during review was dismissed with current-head code evidence after separating lower PageReader rectangle reconciliation from higher ColumnChunkReader cardinality validation.
User focus: no additional focus was supplied; the full PR was reviewed.
Review analysis status: complete after two convergence rounds, final changed-file/unresolved-candidate sweeps, and live head/comment deduplication. The submitted review object will be verified through the PR API immediately after submission.
Contributor
TPC-DS: Total hot run time: 177587 ms |
Contributor
ClickBench: Total hot run time: 24.89 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
3 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.
Proposed changes
Test
./run-be-ut.sh --run --filter=ParquetV2NativeDecoderTest.* -j 24(106 tests passed)build-support/check-format.shclang-tidy