[fix](file) Align V2 condition cache and avoid profile race#66056
Open
Gabriel39 wants to merge 2 commits into
Open
[fix](file) Align V2 condition cache and avoid profile race#66056Gabriel39 wants to merge 2 commits into
Gabriel39 wants to merge 2 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FileScannerV2 split every selected Parquet range at each 2048-row condition-cache granule. Unlike the V1 RowRanges implementation, it did not merge adjacent surviving granules, so an all-true cache hit capped physical batches at the cache granule size and inflated reader calls. Merge adjacent intersections while preserving gaps for false granules.
### Release note
Condition-cache hits no longer split adjacent surviving Parquet ranges at cache granule boundaries.
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=*ConditionCache* -j 32
- Behavior changed: Yes. Adjacent true cache granules retain the original read-range batch boundary.
- Does this need documentation: No.
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 result: no blocking issues found.
Critical checkpoints:
- Goal and correctness: The change coalesces only touching half-open Condition Cache survivor ranges. It preserves the exact selected row set, retains false-granule and deferred PageIndex gaps, and restores the V1 RowRanges batching semantics described by the PR.
- Scope and focus: The production change is small and localized to range normalization, with one focused BE unit regression test.
- Concurrency: No concurrent state or lock behavior changes. The scheduler rewrites its local plan before row-group readers are created.
- Lifecycle and memory safety: No ownership or static-lifecycle changes. Native reader cursors, lazy-column lag, retained scratch, row-group reset, and EOF remain row-count driven rather than cache-granule driven.
- Configuration and compatibility: No configuration, protocol, symbol, storage-format, or rolling-upgrade surface changes. V1 already canonicalizes touching ranges, and parallel PageIndex logic preserves semantic gaps.
- Conditions and error handling: The adjacency condition is exact and cannot bridge a false cache granule. No Status or exception path changes.
- Test coverage and results: The added test directly distinguishes the old 2048-row first batch from the intended 4096-row batch. Existing adjacent Condition Cache coverage checks false gaps, survivor values, filtered-row accounting, and follow-up EOF. No repository build or test was run in this review environment because the review contract prohibits builds; the author reports 29 targeted Condition Cache tests passed.
- Observability: Existing Condition Cache filtered-row, raw/selected-row, TotalBatches, reader, profile-flush, and EOF accounting remain consistent. TotalBatches intentionally decreases for adjacent survivors.
- Transactions, persistence, data writes, and FE-BE variable propagation: Not applicable to this reader-side range normalization.
- Performance: The patch removes artificial 2048-row fragmentation on all-true adjacent granules without adding per-row work or extra allocations.
- Other issues: No actionable correctness, compatibility, lifecycle, performance, observability, or coverage defect remained after two review rounds.
User focus: No additional user-provided focus points were supplied.
Overall opinion: From the code-review perspective, this focused fix is safe to merge once the normal project CI gates complete.
Contributor
TPC-H: Total hot run time: 29686 ms |
Contributor
TPC-DS: Total hot run time: 178180 ms |
Contributor
ClickBench: Total hot run time: 25.2 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
marked this pull request as ready for review
July 26, 2026 10:17
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Gabriel39
changed the title
Contributor
There was a problem hiding this comment.
Automated review result: no blocking issues found.
Critical checkpoints:
- Goal and correctness: The patch restores V1-style coalescing for touching Parquet Condition Cache survivor ranges without changing the selected row set, and atomically reuses the nested Iceberg position-delete profile across parallel scanners.
- Scope and focus: The production changes are small and localized to range normalization plus one reusable RuntimeProfile primitive and its Iceberg call site.
- Concurrency: FileScannerV2 instances share the local-state scanner profile.
get_or_create_child()holds_children_lockacross lookup, allocation, and insertion; counter registration is locked and counter updates are atomic. No reverse lock order or per-block lock work was introduced. - Lifecycle and memory safety: The parent profile owns the shared child; each outer reader uniquely owns and closes its inner reader; scanner-profile lifetime outlives scanner readers. No static-initialization, ownership-cycle, cache-handle, or untracked large-allocation issue is introduced.
- Configuration, compatibility, and parallel paths: No configuration, FE/BE protocol, persisted/storage format, symbol removal, rolling-upgrade, transaction, or data-write behavior changes. Parquet Page Index, partial-cache, false-gap, lazy-read, row-position, ORC position-delete, and mixed Parquet/ORC profile paths remain consistent.
- Conditions and errors: Exact adjacency cannot bridge a false granule, uncovered cache coordinates remain conservative, and Status/error propagation is unchanged.
- Tests and expected results: The new Parquet test distinguishes the old 2048-row first batch from the intended 4096-row batch; the new latch-based 16-thread test proves one shared child. Existing neighboring tests cover false gaps, survivor values, cache accounting/EOF, and distinct Iceberg inner profiles. The review environment prohibited builds/tests; the author reports the targeted BE tests passed. Current formatting, CheckStyle, license, dependency, title, and secret checks pass, while broader BE/compile/external/performance checks are still pending.
- Observability and performance: Existing filtered/raw/selected row and reader/profile counters keep their meanings. Adjacent true granules now avoid artificial batch fragmentation, and profile locking occurs only during child lookup/creation rather than the scan hot path.
- Other issues: No actionable correctness, compatibility, lifecycle, performance, observability, or coverage defect remained after the main review plus normal Parquet, normal RuntimeProfile/Iceberg, and risk-focused convergence passes.
User focus: No additional user-provided focus points were supplied.
Overall opinion: From the code-review perspective, this focused fix is safe to merge once the normal project CI gates complete.
Contributor
TPC-H: Total hot run time: 29299 ms |
Contributor
TPC-DS: Total hot run time: 176986 ms |
Contributor
ClickBench: Total hot run time: 24.98 s |
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
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:
RowRangesimplementation, it did not merge adjacent surviving granules. An all-true cache hit therefore capped physical batches at the cache granule size and inflated reader calls. This change merges adjacent intersections while preserving gaps for false granules, aligning V2 range semantics with V1.The condition-cache regression test configures a 4096-row batch over two adjacent true granules. It returned only 2048 rows before the fix and returns 4096 rows after the fix. The profile regression test starts 16 callers together and verifies that concurrent initialization creates exactly one shared child.
Release note
Condition-cache hits no longer split adjacent surviving Parquet ranges at cache granule boundaries. Parallel Iceberg position-delete scans no longer race while creating their nested runtime profile.
Check List (For Author)
./run-be-ut.sh --run --filter='*ConditionCache*' -j 32(29 tests passed)./run-be-ut.sh --run --filter='RuntimeProfileTest.ConcurrentGetOrCreateChildReturnsSameProfile:IcebergPositionDeleteSysTableV2ProfileTest.*' -j 32(2 tests passed)build-support/check-format.shgit diff --checkrun-clang-tidy.shwas attempted; the local analyzer could not find the systemstddef.hheader. The actionable changed-line diagnostic was fixed, and the unit-test build completed with warnings treated as errors.Check List (For Reviewer who merge this PR)