Skip to content

feat(read)!: decouple raw warmup and read-ahead caching from prefetch - #513

Open
lucasfang wants to merge 3 commits into
apache:mainfrom
lucasfang:codex/decouple-warmup-prefetch
Open

lucasfang wants to merge 3 commits into
apache:mainfrom
lucasfang:codex/decouple-warmup-prefetch

Conversation

@lucasfang

@lucasfang lucasfang commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Purpose

Linked issue: Closes #512

With batch prefetch disabled, Parquet/ORC previously bypassed the shared read-ahead cache and warmup. The existing default configuration (Prefetch=false, cache enabled, Warmup=RAW) now caches synchronous reads and prepares raw bytes ahead of consumption. Warmup never decodes batches; actual reads start parallel decoding when prefetch is enabled.

  • Detect synchronous read-ahead support from the existing reader byte-range interface, without a format whitelist. Build each reader once; readers without that interface retain their original object without opening an extra cache stream.
  • Share the internal FileReadAhead context through CacheInputStream. LM prepares payload ranges before its first payload batch, and ORC prepares later stripe ranges as reading advances. Range callbacks, registration, forwarding and cleanup are removed from the public reader interface.
  • Bound synchronous and adaptive-fallback range-buffer residency using the existing pre-buffer limit. Pending I/O and buffers retained by callbacks count against the budget across resets; safe eviction permits rereads. The block cache keeps its separate budget, and parallel prefetch retains its existing retention policy.
  • Validate initial and later byte ranges against known file sizes before coalescing/splitting. Preserve deferred warmup errors, file/range diagnostics and once-only context error counting.
  • Discover ORC ranges one stripe at a time instead of parsing every stripe footer before the first batch. Parquet selects its buffering properties from the actual shared-cache state, with per-build hints and cached footer reads.
  • Stop and join reader tasks before resetting or releasing the context. Keep cache/I/O metrics available independently of prefetch. Add no configuration options; context wiring uses explicit arguments and overloads.

Tests

Local Debug build with GCC 10.2.1 and LLD 20.1.7:

cmake --build build --target paimon-common-test paimon-core-test paimon-parquet-format-test paimon-orc-format-test paimon-pk-compaction-inte-test -j 32
./build/debug/paimon-common-test
./build/debug/paimon-core-test
./build/debug/paimon-parquet-format-test
./build/debug/paimon-orc-format-test
./build/debug/paimon-pk-compaction-inte-test
git diff --check

All five complete executables passed: common 1,875 passed / 2 conditional cache-disabled skips; core 2,622; Parquet 241; ORC 122; PK compaction integration 60. Total: 4,920 passed, 2 skipped, 0 failures. Focused runs passed: 104 common tests plus the same 2 skips, 2 ORC tests and the bitmap64 high-position integration test.

Coverage includes prefetch/cache/NONE/RAW combinations, adaptive fallback, capability detection independent of format names, actual payload cache I/O before the first returned batch, real ORC stripe-footer access, projections/predicates, bitmaps, row IDs, multiple files, schema reset, deferred failures and early close/destruction. A 32 MiB scan with a 2 MiB range-buffer budget stays within a 2 MiB buffer-pool peak and supports rewind. Controlled streams cover pending I/O and callback-retained buffers across resets and mode changes.

Additional local lifecycle/concurrency review reran 80 tests (plus 2 conditional skips) and used a condition-variable probe to check concurrent range discovery, ended-context no-ops, absence of context ownership cycles and callback-retained pool lifetime. This was an author-side review.

Read-only clang-format 20.1.8, cmake-format, whitespace/EOF and applicable pre-commit large-file, spelling, Sphinx and C++ lint checks passed for all changed files. Rewriting hooks were replaced by read-only checks. Repository-wide unittest, other integration executables, sanitizer/TSan builds, remote-latency benchmarks and disabled-format builds were not run locally.

API and Format

Breaking C++ API changes: remove WarmupLevel::DECODED, PrefetchFileBatchReader::SetPreBufferRangeCallback and PreBufferRangeCallback. Callers must choose RAW or NONE; plugins overriding the callback setter must remove that override and rebuild against the changed virtual interface. The shared context is internal; no public replacement setter or configuration is added.

Remaining enum values and configuration defaults are unchanged. NONE disables early fetching while retaining normal-read caching; RAW requires the cache and prepares only bytes. Existing ReadHints fields remain, and Parquet buffering follows the actual shared-cache state. There is no storage-format or protocol change.

Documentation

Update the warmup/prefetch guide, cache and API comments, and I/O metrics guide. Describe independent cache/warmup behavior, synchronous residency limits, on-demand ORC preparation, plugin migration and early-stop costs. RAW overlaps I/O only when the filesystem's asynchronous read is actually asynchronous; NONE avoids early synchronous fetches on local filesystems.

Generative AI tooling

Generated-by: OpenAI Codex (codex-cli 0.166.0)

@lucasfang lucasfang left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Self-review by OpenAI Codex of commit dd993eb. This is an author-side review, not an independent maintainer approval.

No blocking correctness findings found in the final implementation.

Reviewed cache ownership and once-per-generation registration; weak late-range callbacks; thread join/discard ordering on schema reset, Close and destruction; deferred and terminal read errors; adaptive/format fallback; no-executor synchronous reads; Parquet buffer handoff; bitmap/filter and file-row-ID forwarding; preservation of other format plugins' plain read path; and cache/I/O metrics lifetime.

Corrected stale callback-lifetime and default-read comments during this review. These final edits only change comments; the implementation was validated by all four complete common/core/Parquet/ORC executables (4,846 passed, 2 conditional skips), and the final source passed read-only formatting/lint and git diff --check.

Remaining validation limits: no local sanitizer, repository-wide unittest or integration-executable runs. Default synchronous reads now incur the explicitly requested cache/RAW memory and I/O behavior; RAW on synchronous filesystems retains its documented latency limitation.

@SteNicholas SteNicholas left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@lucasfang, the focused Parquet and ORC cases passed. I found failures under allocation pressure and with caching disabled, along with memory growth on synchronous scans. PTAL.

}

Result<BatchReader::ReadBatchWithBitmap> WarmupFileBatchReader::ReadOneBatch() {
PAIMON_RETURN_NOT_OK(read_ahead_->Warmup(reader_.get()));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes the synchronous reader retain consumed byte ranges until close. With a 2 MiB pre-buffer limit, I measured 2 MiB allocated after warmup, 32 MiB after sequentially reading 32 MiB, and zero after releasing the cache. The limit bounds each fetch window, while previously fetched buffers stay resident. This now affects default scans, including WarmupLevel::NONE on the first actual read. Could the single-reader path evict consumed ranges or enforce a resident-byte budget? A scan several times larger than the window would make a good peak-memory regression test.

for (const auto& range : ranges.value()) {
byte_ranges.emplace_back(range.first, range.second);
}
auto added = cache_->AddRanges(std::move(byte_ranges));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please validate the ranges against the known file size before passing them to AddRanges(), both here and in RegisterLateRanges(). A cache for a 1 KiB file accepted a 32 MiB range starting at offset 512 in my test. A corrupt Parquet total_compressed_size can fit in int64_t but cause the coalescer to allocate a large number of entries before any read reports EOF. Checking offset <= file_size and length <= file_size - offset before splitting also gives us a place to report the file and offending range.

auto state = std::shared_ptr<FileReadAhead>(new FileReadAhead(cache, io_metrics));
for (const auto& reader : readers) {
std::weak_ptr<FileReadAhead> weak_state = state;
reader->SetPreBufferRangeCallback(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please install this callback only when the shared cache is enabled. LateMaterializingFileBatchReader uses the presence of the callback to decide whether to call PreBufferRange() for the payload pass. I reproduced a read failure with the cache off and WarmupLevel::NONE by failing only that range query. The callback's later !cache_ check is too late. A test that asserts zero payload range queries with caching disabled would catch this.

return status_;
}
warmed_ = true;
auto ranges = reader->PreBufferRange();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On ORC, this call enumerates every stripe and reads and parses its footer synchronously before dispatching the first cache fetch. With default RAW warmup, a remote file with many stripes can therefore delay the first batch, even for LIMIT 1 on a filesystem with asynchronous reads. Could range discovery start with the first stripe or pre-buffer window and advance on demand? A counting stream could assert which stripe footers were touched before the first batch was returned.

BREAKING CHANGE: WarmupLevel::DECODED is removed. Use RAW for byte warmup or NONE to disable it.
@lucasfang lucasfang changed the title feat(read): decouple warmup and read-ahead caching from batch prefetch feat(read)!: decouple raw warmup and read-ahead caching from prefetch Nov 5, 2026

@lucasfang lucasfang left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Follow-up self-review by OpenAI Codex of commit 4ddc508. This is an author-side review, not an independent maintainer approval.

The final scope is NONE/RAW warmup only. DECODED and the synchronous wrapper's decode thread, pending Arrow batch and join/discard machinery have been removed. Prefetch's background decode loop is still started by actual batch reads, not Warmup().

No blocking correctness findings found. Checked:

  • Raw byte warmup remains independent of prefetch, including format/adaptive fallback.
  • Warmup never calls the batch decoder; synchronous reads stay on the caller thread.
  • Cache generation/reset, weak late-range callbacks, deferred/terminal errors, row IDs, and early close/destruction remain covered.
  • All callers and docs were migrated away from WarmupLevel::DECODED; the public enum removal is explicitly documented as a source API change.
  • The format guard is now named FormatSupportsReadAhead to describe its byte-cache role.

Validation: all four complete common/core/Parquet/ORC executables passed (4,842 passed, 2 conditional cache-disabled skips). Focused checks passed (139 common tests plus 2 skips, 141 context/system/format-table tests). Build, read-only formatting/lint and git diff --check passed.

Repository-wide unittest, integration executables and sanitizer builds were not run locally. RAW retains its documented synchronous-filesystem limitation; NONE disables early fetching.

Detect synchronous byte-range support from reader capabilities and build once.
Share the internal cache context with LM and ORC instead of range callbacks.
Bound synchronous buffer residency, validate ranges before splitting, and
discover ORC stripe ranges on demand while preserving Parquet buffer handoff.
Cover lifecycle, adaptive fallback, payload warmup and high file positions.

BREAKING CHANGE: remove PrefetchFileBatchReader::SetPreBufferRangeCallback
and PreBufferRangeCallback. Plugins must remove overrides and rebuild.

This branch has not been deployed

No deployments
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.

[Feature] Decouple raw warmup and read-ahead caching from batch prefetch

2 participants