Skip to content

[core] Reuse sidecar block cache for unfiltered scans - #9924

Merged
JingsongLi merged 2 commits into
apache:masterfrom
JingsongLi:codex/manifest-sidecar-block-cache
Sep 17, 2026
Merged

JingsongLi merged 2 commits into
apache:masterfrom
JingsongLi:codex/manifest-sidecar-block-cache

Conversation

@JingsongLi

Copy link
Copy Markdown
Contributor

Purpose

Follow up on the manifest sidecar cache review in #9897.

  • Keep filtered scans on the existing sidecar block-selection path.
  • When a manifest cache and an explicit sidecar reference are both available, let unfiltered scans select all sidecar blocks so prefetch and later filtered reads share the block cache.
  • Keep unfiltered scans without a manifest cache on the existing whole-manifest read path.
  • Fall back to the whole-manifest cache when no usable sidecar is referenced.
  • Update the manifest specification for the refined read behavior.

Tests

JAVA_HOME=/Users/lijingsong/Library/Java/JavaVirtualMachines/corretto-17.0.13/Contents/Home mvn -pl paimon-core -Pfast-build -DwildcardSuites=none -Dtest=ManifestFileTest#testUnfilteredReadWithoutCacheSkipsSidecar+testUnfilteredReadWithCacheAndWithoutSidecarUsesWholeManifestCache+testUnfilteredReadWarmsBlockCacheForFilteredRead test
JAVA_HOME=/Users/lijingsong/Library/Java/JavaVirtualMachines/corretto-17.0.13/Contents/Home mvn -pl paimon-core -DskipTests compile

@leaves12138 leaves12138 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the current head. I found one cache-metrics regression in the new unfiltered path; details and the verified reproducer are in the inline comment. The existing targeted tests and the additional fallback/configuration checks pass. I have not run the full CI suite.

return !options.manifestSidecarEnabled()
|| (query == null && partitionFilter == null && bucketFilter == null)
boolean hasFilter = query != null || partitionFilter != null || bucketFilter != null;
return !options.manifestSidecarEnabled() || (!hasFilter && cache == null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we preserve the cache hit/miss metrics when routing unfiltered scans through the block cache?

Previously, an unfiltered scan used ObjectsCache.read(), which updates the CacheMetrics attached by withCacheMetrics(). With this change, a usable sidecar produces a non-null selection, so ManifestFile.read() bypasses that method. SelectedBlockInput accesses SegmentsCache directly and never updates these counters.

I reproduced this with one sidecar-backed manifest and an enabled manifest cache: perform one cold unfiltered read and then one warm unfiltered read. The second read performs no file I/O, but both manifestHitCache and manifestMissedCache remain 0, rather than recording one hit and one miss. The regression test fails on this commit and passes when only the previous selectBlocks() condition is restored.

This does not change query results, but it makes the existing cache observability silently stop working for unfiltered sidecar-backed scans as well. Please carry the metrics into the block-cache path and preserve the per-manifest accounting, with a regression test covering cold and warm reads.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d3e178e. The selected-block reader now tracks actual block-cache accesses and reports exactly one manifest-level hit or miss after each read: any uncached selected block is a miss, while an all-block cache hit is a hit. The regression test covers one cold unfiltered read followed by one warm unfiltered read and verifies both I/O and the hit/miss counters. Five focused ManifestFile tests and the paimon-core compile with Checkstyle/Spotless pass.

@leaves12138 leaves12138 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed d3e178e. The cache-metrics issue from my previous review is fixed: a selected-block read reports one manifest-level hit when all accessed blocks are cached, or one miss when any accessed block is uncached.

Validated cold and warm reads, unfiltered-to-filtered cache reuse, and additional local regression cases for partially warm reads and blocks exceeding the cache element-size limit. All 236 targeted tests passed. The three focused unfiltered-read tests also passed again with the standard Checkstyle, Spotless and Enforcer checks enabled. No new blocking findings; LGTM.

I did not run the full CI suite.

@JingsongLi
JingsongLi merged commit 9b52878 into apache:master Sep 17, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants