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.
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.
There was a problem hiding this comment.
@lucasfang, I have reproduced the three issues below against b755d585. PTAL.
Forward synchronous batch reads through the matching underlying API so sparse bitmap compaction keeps the format reader's file-row mapping and bounds checks. Avoid opening a cache stream when LM wraps a reader without byte-range support. Prepare the target ORC stripe before seekToRow can read payload bytes. Add sparse-bitmap, LM capability and physical seek I/O regression coverage, and route on-demand ORC stripe tests through CacheInputStream. Validation: 5,430 tests passed with 2 conditional skips across seven complete test executables; changed-file pre-commit checks and git diff --check passed.
There was a problem hiding this comment.
@lucasfang, thanks for updates. I left only comment for updates. PTAL.
There was a problem hiding this comment.
@lucasfang, thanks for update. I left only one comment for the remaining issue reproduced below: predicate-driven ORC stripe skipping bypasses range preparation and causes duplicate payload I/O. PTAL.
| } | ||
|
|
||
| Result<BatchReader::ReadBatch> OrcFileBatchReader::NextBatch() { | ||
| PAIMON_RETURN_NOT_OK(PrepareRangesForRow(reader_->GetNextRowToRead())); |
There was a problem hiding this comment.
Predicate-driven stripe skipping can read the target payload twice. GetNextRowToRead() still denotes the pre-filter position here, but ORC's startNextStripe() can advance to a matching stripe inside reader_->Next() without calling our SeekToRow(). In a three-stripe file containing rows 0–11, with a >= 8, batch size 2 and the block cache disabled, I reproduced the first NextBatch() reading all 7 bytes of stripe 2's payload directly (zero payload bytes from the range cache). The second NextBatch() then fetched those same 7 bytes through PrepareRangesForRow(10), after ORC had already consumed them. The explicit-seek fix does not cover this path. For larger stripes this adds redundant storage I/O and misses preparation before the first matching batch. Please coordinate preparation with the stripe actually selected by ORC before its payload read, or otherwise prevent fetching already-consumed payload again. Extend the predicate portion of DiscoverStripeRangesOnDemand to assert physical reads/cache hits as well as returned rows.
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)