perf(read): support warmup between data files - #286
Merged
Conversation
lucasfang
marked this pull request as draft
September 6, 2026 15:28
lucasfang
marked this pull request as ready for review
September 7, 2026 02:58
lucasfang
force-pushed
the
dev14
branch
2 times, most recently
from
September 9, 2026 01:52
598f880 to
5a1966b
Compare
lxy-9602
reviewed
Sep 9, 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
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.
Purpose
Linked issue: close #289
Reading a primary-key table in merge-on-read mode serializes the first read of every data file behind the consumption of the previous one, because
PrefetchFileBatchReaderImplstarts its background thread lazily, inside the firstNextBatchWithBitmap(). Within a sorted run the next file's first read starts only once the current file is exhausted, and across a section'skrunsLoserTree::InitializeIfNeededblocks on each leaf's first read in turn. On remote storage that is one round trip per file instead of overlapping them.This PR adds one optional
Warmup()verb to the two reader abstractions and calls it where the consumer already knows what it is going to read next, so that reader's first read is issued while the current one is still being consumed. Warming changes when a file is opened, never what it reads: ordering, merging, filtering, deletion vectors and metrics are untouched.How far it prepares the next file is a public option, because it trades memory for latency and a scan that stops early - a
LIMIT, or a predicate selective enough to empty a split - pays for whatever it already warmed.WarmupLevelsits ininclude/paimon/utils/prefetch_cache_config.hnext to theCacheConfigit interacts with, set throughReadContextBuilder::SetWarmupLeveland read back throughReadContext::GetWarmupLevel. Each level takes the next file one step further along the read pipeline, so a higher one hides more latency and commits more memory:NONE- the behavior from before warmup existed: no extra memory, no background thread.RAW- fetch only the next file's raw, still-compressed bytes, which overlaps the remote fetch without materializing a decoded batch ahead of the read.DECODED(default) - also start the background decode loop, so decoded batches are ready before the file is read.Changes:
virtual void Warmup()with an empty default body onBatchReader(public) and on the internalKeyValueRecordReader, so no existing implementation has to change. It returnsvoidon purpose: the caller may warm a reader it never ends up reading, and a hint about a file nobody reads must not be able to fail the read in progress.PrefetchFileBatchReaderImpl::Warmup()is the only real implementation and interprets the level:NONEreturns at once,RAWcalls the newWarmCacheOnce(),DECODEDcallsEnsureBackgroundThread(), into which the lazy thread start moved so the read path and the warmup path share one start. All three return early when the read ranges are not fresh, a stateNextBatchWithBitmap()still rejects, so an unprepared read is reported there and not swallowed here.ReadAheadCache::Init()is not idempotent, soWarmCacheOnce()guards oncache_warmed_, whichCleanUp()clears for the next read-range generation.FileBatchReaderwrappers forward the call -FieldMappingReader,CompleteRowTrackingFieldsBatchReader,ApplyDeletionVectorBatchReader,ApplyBitmapIndexBatchReader,ShreddingFileReader,VectorFileBatchReaderandDelegatingPrefetchReader- plusKeyValueDataFileRecordReaderon the KeyValue side.DelegatingPrefetchReaderskips the call whenNeedPrefetch()is false, becauseGetReader()then bypasses the prefetch layer and forwarding would decode the reader the consumer is itself reading.ConcatKeyValueRecordReaderwarms the reader it is consuming plus one file ahead (kWarmupLookahead = 1) on everyNextBatch(), andLoserTree::InitializeIfNeeded()calls the newWarmupLeaves()before its advance loop, in consumption order, so a section ofkruns haskfiles in flight at once - one per leaf, not two.MergeFileSplitReadnow builds a section's runs with oneCreateRawFileReaderscall over the flattened file list and hands the readers back by position, the merge path and the raw path both going through it;CreateReaderForRunis removed. Because the matching is positional, the existingasserton the two counts becomes an explicitStatus::Invalid: a release build would otherwise shift files into the wrong run instead of failing.DataFileReadOptions::warmup_level, filled byAbstractSplitReadandFormatTableReadfrom theReadContextand forwarded byDataFileReaderFactory::Open(), the one place a production read builds the prefetching reader.Create()no longer defaults the parameter, so a dropped link is a compile error instead of a silent fallback toDECODED.ReadContextfor the data table underneath them and copied every prefetch and cache setting but not this one, so a caller'sNONEorRAWcame back asDECODEDon the table that does the actual reading; each chain is now a namedCreateDataReadContext(), which is also what makes it testable.Two known limits, left as follow-ups: warming does not cross a section boundary, because sections sit inside a
SortMergeKeyValueRecordReaderholding aSortMergeReaderwith no warmup verb to forward to; and themin-heapengine gets the intra-run warmup but not the cross-run one, since onlyLoserTreewarms its leaves -loser-treeis the default.One build fix rides along, plus the CI gap that hid it; both can be split into their own PR if preferred.
PAIMON_RETURN_NOT_OK_FROM_ARROWandPAIMON_ASSIGN_OR_RAISE_IMPL_FROM_ARROWcalledToPaimonStatusby unqualified name, which does not resolve at global scope, wherebenchmark/parquet_format_benchmark.cppkeeps its helpers, so every expansion site there failed to compile; both now spell::paimon::ToPaimonStatus. CI never saw it because it builds nothing underbenchmark/, the top-levelCMakeLists.txthaving added it only underPAIMON_BUILD_BENCHMARKS; it is now added underPAIMON_BUILD_TESTS OR PAIMON_BUILD_BENCHMARKS, so a tests-only build registers the two gtest targets already gated there (ctest count 28 to 30). Building them exposed two dormant defects, fixed here too: the benchmark test linked only the Parquet format library, so theCoreOptionsit constructs could not resolvemanifest.format- defaultavro- and all 8 cases failed, so it now links the benchmark's ownPAIMON_BENCHMARK_STATIC_LINK_LIBS; and its negative codec assertion wrote a real"lz4"file, which Parquet rejects only mid-write through a Thrift conversion whosedefaultbranch isDCHECK(false), an abort in the six Debug and sanitizer CI jobs, so it now pins the same fact at name resolution with no write.Tests
18 new cases: 17 appended to test files that already build in an existing target, plus a new
data_file_reader_factory_test.cppregistered insrc/paimon/CMakeLists.txt.MockFileBatchReadergained aWarmup()override and aGetWarmupCount()accessor, which is what makes the forwarding cases possible without a new mock per wrapper.ConcatKeyValueRecordReaderTest.TestWarmupLooksOneReaderAhead, one case per forwarding wrapper, andKeyValueDataFileRecordReaderTest.TestWarmupForwardsToInnerReader. The Concat case also pins that nothing is warmed before a read asks for it, and that a warmed Concat read to the end yields the same KeyValues as the unwarmed one.SortMergeReaderTest.TestLoserTreeWarmsAllLeavesBeforeAdvancingAnyasserts a three-leaf tree records exactlywarm2, warm1, warm0, read2, read1, read0, and that a secondInitializeIfNeeded()records nothing.PrefetchFileBatchReaderImplTest.TestWarmupLevelNone,.TestWarmupLevelRawand.TestWarmupLevelDecoded, each over{parquet, orc} x {read-ahead cache on, off}, assert whether the background decode loop started and then read every row against the unwarmed expectation; two more cover both branches of theNeedPrefetch()guard;.TestWarmupCacheRearmsForNewReadRangeGenerationcovers thecache_warmed_lifecycle across aRefreshReadRanges().ReadContextTest.TestSetWarmupLevel,SystemTableTest.TestNewReadPropagatesWarmupLevel- for$ro,$audit_logand$binlog, asEXPECT_EQso one broken chain does not hide another - andDataFileReaderFactoryTest.OpenForwardsWarmupLevelToPrefetchReader.The assertions are shaped by mutation testing: seven deletions that all compile - the
WarmupLeaves()call, all seven wrapper forwards,DECODEDdegraded toRAW,cache_warmed_.store(false), theNeedPrefetch()guard, andSetWarmupLevelfrom each system-table chain - each left the full core and common suites green before, and each is now killed by the case that pins it. The changed paths also stay covered by the existingConcatKeyValueRecordReaderTest,SortMergeReaderTest(both engines),MergeFileSplitReadTestandPrefetchFileBatchReaderImplTest.Validation, local Release build with
PAIMON_BUILD_TESTS=ONandPAIMON_BUILD_BENCHMARKS=ON, all passed:30 of 30 targets under the
unittestlabel, including the two theCMakeLists.txtchange brings into a tests-only build;paimon-core-test2053;paimon-common-test1572 ran, 1570 passed and 2 skipped - the cache-disabled parameters ofTestWarmupCacheRearmsForNewReadRangeGeneration, and the only skips in any run;paimon-read-inte-test294;paimon-write-and-read-inte-test198;paimon-pk-compaction-inte-test38. Not run locally, left to CI: the sanitizer jobs andci/scripts/build_paimon.sh. The sanitizers matter more than usual here, becauseDECODEDwarmup and theNeedPrefetch()guard both start work on a background thread ahead of the reader's own thread.API and Format
Three public API changes under
include/paimon/, all additive except one constructor. No storage format and no protocol change: nothing written to disk, to a manifest, to a file footer or over the wire is touched, and warming changes when a file is opened, never what is read.reader/batch_reader.h:virtual void Warmup()with an empty default body on the exportedBatchReader. Source compatible, but not binary compatible - a new virtual changes the vtable layout and shifts the slot of every virtual after it - so everything must be recompiled together.utils/prefetch_cache_config.h: newenum class PAIMON_EXPORT WarmupLevel { NONE, RAW, DECODED }. Purely additive and binary compatible; a scoped enum carries no data and no functions.read_context.h:SetWarmupLevel()andGetWarmupLevel()are new non-virtual members and the builder is a pimpl, sosizeof(ReadContextBuilder)is unchanged.ReadContext's public constructor gained a trailingWarmupLevel, source-breaking for a caller that constructs it directly - the class documents going throughReadContextBuilder, and nothing here does - and it gained a private member, sosizeof(ReadContext)changes; no virtuals, so layout only.Everything else is under
src/paimon/and not exported:KeyValueRecordReader::Warmup(),LoserTree::WarmupLeaves(),WarmCacheOnce(),EnsureBackgroundThread(), theMergeFileSplitReadsignature changes,DataFileReadOptions::warmup_level, the twoCreateDataReadContext()methods, andPrefetchFileBatchReaderImpl::Create()'s new trailingWarmupLevelparameter, which has no default.Documentation
The three new methods need no
.rstedit of their own:docs/source/api/read.rstalready pullspaimon::ReadContextBuilder,paimon::ReadContextandpaimon::BatchReaderin throughdoxygenclasswith:members: :undoc-members:, andapi/file_format.rstdoes the same forpaimon::FileBatchReader, so they render automatically with the doc comments this PR adds.One documentation line is added:
.. doxygenenum:: paimon::WarmupLevelinapi/read.rst. Without it no.rstreferences the new public enum, so the@see WarmupLevelinSetWarmupLevel's doc comment is a dangling reference and the meaning of the three levels appears nowhere in the rendered docs even though the option is public. The repository already usesdoxygenenumforpaimon::FieldType,paimon::ByteOrderandpaimon::SeekOrigin. No user-guide page is affected:user_guide/prefetch.rstdocuments no user-facing option at all.Generative AI tooling
Generated-by: Qoder