[fix](be) Preserve condition cache granule base#65496
Merged
yiguolei merged 4 commits intoJul 13, 2026
Merged
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: External file condition-cache entries stored only their survivor bitmap. On a
later cache hit, the physical reader recomputed the bitmap base from the current metadata-pruned
scan plan. If pruning selected a different first row group, bitmap indexes shifted and valid rows
could be silently skipped. Store the bitmap's base granule with the cache value, restore it on V2
cache hits, and only derive it from the V2 reader plan when building a cache entry.
### Release note
Fix incorrect row filtering when a V2 external file condition-cache hit uses a different pruned
scan plan from the cache-producing scan.
### Check List (For Author)
- Test: Unit Test
- `ParquetScanConditionCacheTest.HitKeepsCachedBaseWhenCurrentPlanStartsLater`
- `TableReaderTest.ConditionCacheMissPublishesBitmapAfterReaderEof`
- `NewOrcReaderTest.ConditionCacheHitUsesSplitBaseGranule`
- Behavior changed: Yes. V2 condition-cache hits use the bitmap coordinate base stored with the
cache entry instead of recomputing it from the current scan plan.
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Gabriel39
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
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 two data-correctness blockers in the condition-cache coordinate handling.
Critical checkpoint conclusions: the PR's goal is to preserve the V2 cache bitmap origin, but the implementation still allows valid rows to be skipped in two cases. The code change is small, but it changes the shared condition-cache value contract used by both V1 and V2 scanners. I did not find a new locking/lifecycle issue in the LRU cache path. Compatibility/config is a problem because enable_file_scanner_v2 can switch scanners while both modes share the same ExternalCacheKey. Tests cover basic V2 base restore and publish behavior, but not the persisted guard bit or mixed V1/V2 cache consumption. I fetched the exact base and git diff --check passed; I did not run BE unit tests because this runner is missing thirdparty/installed and thirdparty/installed/bin/protoc.
Subagent conclusions: optimizer-rewrite reported no candidates. tests-session-config reported TSC-1, which I accepted as MAIN-002. MAIN-001 was found and verified by the main review. Convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for this exact two-comment set. User focus: no additional user-provided review focus was supplied.
### What problem does this PR solve? Issue Number: None Related PR: apache#65496 Problem Summary: A condition-cache MISS allocated an extra guard bit that was published as if it covered real rows. A later plan could therefore interpret the false guard as proof that the next real granule had no matches. In addition, legacy and V2 scanners shared external cache keys even though only V2 understands the persisted bitmap base. Track the authoritative granule count and trim allocation-only guards before publication, and version V2 external cache keys to isolate the base-aware coordinate protocol from legacy entries. ### Release note Fix condition-cache filtering across changed metadata-pruning plans and mixed scanner modes. ### Check List (For Author) - Test: Unit Test - `ParquetScanConditionCacheTest.*` - `TableReaderTest.ConditionCacheMissPublishesBitmapAfterReaderEof` - `TableReaderTest.ConditionCacheMissIsDroppedWhenReaderClosesBeforeEof` - `NewOrcReaderTest.ConditionCacheMissMarksSurvivingGranules` - `NewOrcReaderTest.ConditionCacheHitUsesSplitBaseGranule` - Behavior changed: Yes. Guard bits are not published as authoritative cache coverage, and V2 external condition-cache entries cannot be consumed by legacy scanners. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review completed for PR 65496 at head f019e5d.
I found one correctness issue in the ORC V2 condition-cache MISS lifecycle and left it inline. The prior cache-coordinate concerns in existing threads appear addressed by the current head (V2 external key versioning and trim-to-authoritative num_granules), so I did not duplicate them.
Checkpoint conclusions:
- Goal/test: PR aims to store base-aware condition-cache bitmaps for V2 readers and add focused tests; coverage misses the TableReader + ORC all-stripes-pruned condition-cache path.
- Scope: Change is focused, but the ORC zero-range lifecycle needs handling.
- Concurrency/lifecycle: No new shared-lock issue found; lifecycle issue accepted inline for no row reader after ORC pruning.
- Compatibility: V2 external key is versioned separately from legacy FileScanner; OLAP CacheKey is unaffected.
- Parallel paths/performance/observability: Parquet and ORC hit paths keep out-of-bitmap rows conservatively; no additional performance or observability blocker found.
- Tests: Static review only per review prompt; no builds/tests were run. Existing direct ORC EOF coverage does not exercise TableReader condition-cache setup.
User focus: No additional user-provided review focus.
### What problem does this PR solve? Issue Number: None Related PR: apache#65496 Problem Summary: When ORC SARG or min/max pruning removed every stripe, no row reader range was created. The reader still reported the full file row count, so TableReader initialized a condition-cache MISS bitmap and ORC attempted to derive its authoritative granule span from an empty range. Report zero selected rows for this known fully-pruned state so TableReader skips condition-cache setup and the scan completes as EOF. ### Release note Fix ORC scans with condition cache enabled when all stripes are pruned by statistics. ### Check List (For Author) - Test: Unit Test - NewOrcReaderTest.SargConjunctReturnsEofWhenAllStripesArePruned - NewOrcReaderTest.ConditionCacheMissMarksSurvivingGranules - NewOrcReaderTest.ConditionCacheHitHandlesSplitWithoutSelectedStripe - NewOrcReaderTest.ConditionCacheHitUsesSplitBaseGranule - Behavior changed: Yes, fully pruned ORC scans no longer initialize or publish a condition-cache MISS bitmap. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed the condition-cache follow-up across TableReader, ORC, Parquet, ConditionCache, legacy FileScanner, and internal SegmentIterator paths. I found two issues that should be fixed before this lands.
Critical checkpoint conclusions:
- Goal/test coverage: the V2 base/guard fixes are mostly covered, but the PR still misses non-adjacent ORC selected ranges and stop-during-read MISS publication.
- Scope/compatibility: the new external key version separates V2 from legacy FileScanner entries, and internal segment cache behavior is unaffected.
- Concurrency/lifecycle: scanner stop can still be mistaken for complete physical EOF and publish a partial bitmap.
- Parallel paths: Parquet HIT filtering is conservative outside the cached span; ORC MISS publication still only records the first selected range span.
- Validation: static review only, per the review prompt; no builds or tests were run. No additional user focus was provided.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65496 Problem Summary: Extend ORC condition-cache MISS bitmaps across non-adjacent selected stripe ranges, and avoid publishing partial MISS results when a scan is stopped during a read that reports EOF. ### Release note Fix incomplete condition-cache publication for non-adjacent ORC stripe ranges and stopped scans. ### Check List (For Author) - Test: Unit Test\n - TableReader condition-cache tests\n - ORC non-adjacent stripe-range tests\n- Behavior changed: Yes. Condition-cache MISS entries are only published after complete reads and now cover all selected ORC ranges.\n- Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review completed for PR #65496.
I did not find any new non-duplicate correctness issue to raise after the follow-up patch. The previously reported risks around stopped EOF publishing, guard-bit overreach, legacy/V2 cache-coordinate mixing, ORC all-stripe pruning, and ORC non-adjacent selected ranges appear addressed by the current changes.
Checkpoint conclusions:
- Goal/test: the patch now stores base-aware V2 external condition-cache entries, trims MISS bitmaps to the authoritative span, avoids publishing stop/cancel partial MISS state, handles all-pruned ORC as zero rows, and extends ORC MISS spans across non-adjacent selected ranges. Added BE tests cover these cases plus Parquet cached-base HIT conservatism.
- Scope: the change is focused on condition-cache coordinate/lifecycle fixes and related tests.
- Concurrency/lifecycle: the main lifecycle risk is
io_ctx->should_stopracing with physical reader EOF conversion;TableReadernow samples it after reads and only publishes on real EOF. LIMIT/abort/close-before-EOF paths drop MISS state. - Compatibility/parallel paths: V2 external entries use a versioned key, legacy FileScanner remains on version 0, and OLAP segment condition cache uses a separate
CacheKeywith default base 0. I found no remaining cross-mode coordinate collision. - Data correctness/performance: HIT paths stay conservative outside stored bitmap spans, so sparse Parquet/ORC coverage may reduce cache benefit but should not skip valid rows. The added work is small key/base/span bookkeeping and publish-time trimming.
- Tests/validation: review was static only per the runner instruction; no build or tests were run. Subagent convergence completed with S1/S2/S3/RF1/RF2 and round-2 C1/C2 all returning NO_NEW_VALUABLE_FINDINGS.
User focus: no additional user-provided review focus was specified.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29434 ms |
Contributor
TPC-DS: Total hot run time: 180729 ms |
Contributor
ClickBench: Total hot run time: 24.95 s |
yiguolei
approved these changes
Jul 13, 2026
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.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: External file condition-cache entries stored only their survivor bitmap. On a
later cache hit, the physical reader recomputed the bitmap base from the current metadata-pruned
scan plan. If pruning selected a different first row group, bitmap indexes shifted and valid rows
could be silently skipped.
This change stores the bitmap's base granule with the cache value. The V2 table reader restores the
stored base on cache hits, while V2 Parquet and ORC readers derive a base from their current plan
only when producing a new cache entry. V1 reader behavior is unchanged.
Release note
Fix incorrect row filtering when a V2 external file condition-cache hit uses a different pruned
scan plan from the cache-producing scan.
Check List (For Author)
ParquetScanConditionCacheTest.HitKeepsCachedBaseWhenCurrentPlanStartsLaterTableReaderTest.ConditionCacheMissPublishesBitmapAfterReaderEofNewOrcReaderTest.ConditionCacheHitUsesSplitBaseGranulecache entry instead of recomputing it from the current scan plan.