Repository navigation
Conversation
lucasfang
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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())); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
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.FileReadAheadcontext throughCacheInputStream. 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.Tests
Local Debug build with GCC 10.2.1 and LLD 20.1.7:
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::SetPreBufferRangeCallbackandPreBufferRangeCallback. 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
ReadHintsfields 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)