From 875a580512ed5d6327d64dd21f9b34997dc7c523 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Mon, 14 Sep 2026 16:57:34 +0800 Subject: [PATCH 01/15] perf(prefetch): prefetch the late-materialization payload ranges mid-read --- .../reader/prefetch_file_batch_reader.h | 14 + include/paimon/utils/prefetch_cache_config.h | 19 ++ .../late_materializing_file_batch_reader.cpp | 20 ++ .../late_materializing_file_batch_reader.h | 20 +- ...e_materializing_file_batch_reader_test.cpp | 141 +++++++++ .../prefetch_file_batch_reader_impl.cpp | 58 ++++ .../reader/prefetch_file_batch_reader_impl.h | 22 ++ src/paimon/common/utils/read_ahead_cache.cpp | 277 +++++++++++++----- src/paimon/common/utils/read_ahead_cache.h | 57 +++- .../common/utils/read_ahead_cache_test.cpp | 260 +++++++++++++++- .../testing/mock/mock_file_batch_reader.h | 26 ++ src/paimon/testing/mock/mock_file_system.h | 7 +- .../testing/mock/mock_format_reader_builder.h | 21 +- test/inte/read_inte_test.cpp | 11 + 14 files changed, 872 insertions(+), 81 deletions(-) diff --git a/include/paimon/reader/prefetch_file_batch_reader.h b/include/paimon/reader/prefetch_file_batch_reader.h index 842279956..77f4aa467 100644 --- a/include/paimon/reader/prefetch_file_batch_reader.h +++ b/include/paimon/reader/prefetch_file_batch_reader.h @@ -19,6 +19,7 @@ #pragma once +#include #include #include #include @@ -135,6 +136,19 @@ class PAIMON_EXPORT PrefetchFileBatchReader : public FileBatchReader { virtual Result>> PreBufferRange() { return std::vector>{}; } + + /// Sink a reader reports byte ranges to when they only become known after reading has + /// started, so the prefetch layer can register them with its shared read-ahead cache. + /// + /// PreBufferRange() covers what is known up front; a reader whose ranges depend on data it + /// has already read - the late-materialization payload pass only knows which pages hold the + /// matched rows once the probe pass has run - reports them here instead. + using PreBufferSink = std::function>&&)>; + + /// Installs the sink above, or clears it when `sink` is empty. By default a reader has no + /// late byte ranges to report and ignores the sink. + /// @param sink The sink to report late byte ranges to. + virtual void SetPreBufferSink(PreBufferSink /*sink*/) {} }; } // namespace paimon diff --git a/include/paimon/utils/prefetch_cache_config.h b/include/paimon/utils/prefetch_cache_config.h index 9f20fbf2b..122c2a497 100644 --- a/include/paimon/utils/prefetch_cache_config.h +++ b/include/paimon/utils/prefetch_cache_config.h @@ -45,6 +45,19 @@ class PAIMON_EXPORT CacheConfig { range_size_limit_ = range_size_limit; } + /// Returns the maximum allowed size (in bytes) for a single range registered after the cache + /// was initialized, i.e. for the ranges that only become known while reading. + /// Defaults to 8 MiB. + uint64_t GetLateRangeSizeLimit() const { + return late_range_size_limit_; + } + + /// Sets the maximum allowed size (in bytes) for a single range registered after the cache was + /// initialized. + void SetLateRangeSizeLimit(uint64_t late_range_size_limit) { + late_range_size_limit_ = late_range_size_limit; + } + /// Returns the maximum gap size (in bytes) considered mergeable between /// adjacent ranges. Defaults to 8 KiB. uint64_t GetHoleSizeLimit() const { @@ -100,7 +113,13 @@ class PAIMON_EXPORT CacheConfig { // (coalesced column-chunk reads of ~128 MiB were observed): fetches are // only dispatched up to this window, so a request reaching past it can // never be served and falls back to a second fetch of the same bytes. + // - late_range_size_limit bounds the ranges registered mid-read instead of + // at Init: those are fetched only just before they are read, and one range + // is one request, so they are cut smaller than range_size_limit to be + // fetched concurrently rather than in one long request. A read spanning + // several of them is still served, as they are adjacent. uint64_t range_size_limit_ = 32 * 1024 * 1024; + uint64_t late_range_size_limit_ = 8 * 1024 * 1024; uint64_t hole_size_limit_ = 8 * 1024; uint64_t pre_buffer_limit_ = 256 * 1024 * 1024; // Blocks are aligned to the END of the file, so a block never reaches past diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader.cpp b/src/paimon/common/reader/late_materializing_file_batch_reader.cpp index 5554b92b2..e0ef74ac1 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader.cpp +++ b/src/paimon/common/reader/late_materializing_file_batch_reader.cpp @@ -73,6 +73,7 @@ Result LateMaterializingFileBatchReader::NextBatch() PAIMON_RETURN_NOT_OK( SetInnerReadSchema(payload_schema_, /*predicate=*/nullptr, matched_bitmap_)); state_ = kRunning; + PAIMON_RETURN_NOT_OK(ReportPayloadPreBufferRanges()); } } @@ -262,6 +263,25 @@ Status LateMaterializingFileBatchReader::SetInnerReadSchema( return Status::OK(); } +Status LateMaterializingFileBatchReader::ReportPayloadPreBufferRanges() { + // The payload byte ranges only exist now: setting the payload schema refined the inner + // reader's target row groups down to the pages holding the matched rows. Reporting them + // here lets the shared read-ahead cache fetch them while this pass is still assembling + // its first batch, instead of every payload read waiting for its own IO. + if (!pre_buffer_sink_ || prefetch_inner_ == nullptr) { + return Status::OK(); + } + // The error is propagated rather than swallowed: the ranges come from the file metadata, so a + // failure here means the reads about to follow would fail too. + using ByteRanges = std::vector>; + PAIMON_ASSIGN_OR_RAISE(ByteRanges ranges, prefetch_inner_->PreBufferRange()); + if (ranges.empty()) { + return Status::OK(); + } + pre_buffer_sink_(std::move(ranges)); + return Status::OK(); +} + Status LateMaterializingFileBatchReader::SetReadSchema( ::ArrowSchema* read_schema, const std::shared_ptr& predicate, const std::optional& selection_bitmap) { diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader.h b/src/paimon/common/reader/late_materializing_file_batch_reader.h index 7a225bca7..b9f826df0 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader.h +++ b/src/paimon/common/reader/late_materializing_file_batch_reader.h @@ -93,14 +93,22 @@ class LateMaterializingFileBatchReader : public PrefetchFileBatchReader { Status SetReadRanges(const std::vector>& read_ranges) override; Result>> PreBufferRange() override { - // TODO(zhouhongfeng.zhf): PrebufferRange (called by PrefetchFileBatchReader) only read the - // probe data, consider read the payload data as well. + // Deliberately only the probe ranges: this is asked once before any read, when the payload + // pass still has no idea which pages hold the matched rows. The payload ranges are + // reported through pre_buffer_sink_ once the probe pass has run, see + // ReportPayloadPreBufferRanges(). if (prefetch_inner_ == nullptr) { return std::vector>{}; } return prefetch_inner_->PreBufferRange(); } + void SetPreBufferSink(PreBufferSink sink) override { + // Not forwarded to prefetch_inner_: a format reader has no late ranges of its own, and + // forwarding would report the same ranges twice. + pre_buffer_sink_ = std::move(sink); + } + private: LateMaterializingFileBatchReader(std::unique_ptr inner, PrefetchFileBatchReader* prefetch_inner, @@ -141,6 +149,11 @@ class LateMaterializingFileBatchReader : public PrefetchFileBatchReader { const std::shared_ptr& predicate, const std::optional& selection); + /// Report the byte ranges the payload pass is about to read to pre_buffer_sink_, so that the + /// prefetch layer can fetch them ahead of the pass. A no-op without a sink or without a + /// prefetch-capable inner reader. + Status ReportPayloadPreBufferRanges(); + /// Returns the inner reader's prefetch interface, or an error when the format reader does not /// implement it (avro and blob do not). Result GetPrefetchReaderOrRaise( @@ -163,6 +176,9 @@ class LateMaterializingFileBatchReader : public PrefetchFileBatchReader { /// inner_ is never reassigned, so the cast is resolved once in Create(). PrefetchFileBatchReader* prefetch_inner_ = nullptr; std::shared_ptr arrow_pool_; + /// Where the payload byte ranges are reported. Part of how this reader is wired into the + /// prefetch layer rather than of a read generation, so Reset() leaves it alone. + PreBufferSink pre_buffer_sink_; LatMatState state_ = kInit; std::shared_ptr full_schema_; // projection holding only the predicate fields; nullptr when probing is not applicable diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp index 0849836ca..36c751a2a 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp +++ b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp @@ -765,4 +765,145 @@ TEST_F(LateMaterializingFileBatchReaderTest, FailsOnPredicateTypeMismatch) { "mismatches"); } +// The payload byte ranges are only known once the probe pass has run, so they are reported through +// the sink rather than through PreBufferRange(), and early enough to still be prefetched: before +// the first payload batch leaves the reader. +TEST_F(LateMaterializingFileBatchReaderTest, PayloadPreBufferRangesReportedToSink) { + auto data = BuildData({0, 1, 2, 3, 4, 5, 6, 7, 8, 9}); + auto mock = std::make_unique(data, full_type_, /*batch_size=*/3); + // Aliased because a type carrying a comma cannot be passed to the ASSERT_OK_AND_ASSIGN macro. + using ByteRanges = std::vector>; + const ByteRanges probe_ranges = {{0, 1024}}; + const ByteRanges payload_ranges = {{100000, 2048}, {200000, 4096}}; + mock->SetPreBufferRangesPerSchema({probe_ranges, payload_ranges}); + ASSERT_OK_AND_ASSIGN(auto reader, LateMaterializingFileBatchReader::Create( + std::move(mock), GetArrowPool(pool_))); + int32_t batches = 0; + std::vector reported; + std::vector batches_when_reported; + reader->SetPreBufferSink([&](ByteRanges&& ranges) { + reported.push_back(std::move(ranges)); + batches_when_reported.push_back(batches); + }); + auto predicate = + PredicateBuilder::GreaterOrEqual(/*field_index=*/0, "k", FieldType::BIGINT, Literal(4l)); + ASSERT_OK(SetReadSchema(reader.get(), arrow::schema(full_fields_), predicate, std::nullopt)); + // What is known up front is the probe projection's ranges, not the payload's. + ASSERT_OK_AND_ASSIGN(ByteRanges up_front, reader->PreBufferRange()); + EXPECT_EQ(up_front, probe_ranges); + + while (true) { + ASSERT_OK_AND_ASSIGN(FileBatchReader::ReadBatch batch, reader->NextBatch()); + if (BatchReader::IsEofBatch(batch)) { + break; + } + // Import the batch to run its release callback, as a real consumer would. + auto& [c_array, c_schema] = batch; + ASSERT_TRUE(arrow::ImportArray(c_array.get(), c_schema.get()).ok()); + batches++; + } + EXPECT_GT(batches, 0); // the payload pass did run + ASSERT_EQ(reported.size(), 1u); + EXPECT_EQ(reported[0], payload_ranges); + // Reported while the payload pass was still assembling its first batch, which is the whole + // point: reporting it later would leave the first payload reads waiting for their own IO. + EXPECT_EQ(batches_when_reported[0], 0); +} + +// Without a predicate there is no payload pass, so there is no late range to report. +TEST_F(LateMaterializingFileBatchReaderTest, NoSinkReportWithoutLateMaterialization) { + auto data = BuildData({0, 1, 2, 3, 4}); + auto mock = std::make_unique(data, full_type_, /*batch_size=*/2); + mock->SetPreBufferRangesPerSchema({{{0, 1024}}, {{100000, 2048}}}); + ASSERT_OK_AND_ASSIGN(auto reader, LateMaterializingFileBatchReader::Create( + std::move(mock), GetArrowPool(pool_))); + int32_t calls = 0; + reader->SetPreBufferSink([&calls](std::vector>&&) { calls++; }); + ASSERT_OK(SetReadSchema(reader.get(), arrow::schema(full_fields_), /*predicate=*/nullptr, + std::nullopt)); + + ASSERT_OK_AND_ASSIGN(std::vector rows, Collect(reader.get())); + ASSERT_EQ(rows.size(), 5u); + EXPECT_EQ(calls, 0); +} + +// The probe pass matched nothing, so the payload pass never runs and reports no range. +TEST_F(LateMaterializingFileBatchReaderTest, EmptyMatchDoesNotReportPayloadRanges) { + auto data = BuildData({0, 1, 2, 3, 4}); + auto mock = std::make_unique(data, full_type_, /*batch_size=*/2); + mock->SetPreBufferRangesPerSchema({{{0, 1024}}, {{100000, 2048}}}); + ASSERT_OK_AND_ASSIGN(auto reader, LateMaterializingFileBatchReader::Create( + std::move(mock), GetArrowPool(pool_))); + int32_t calls = 0; + reader->SetPreBufferSink([&calls](std::vector>&&) { calls++; }); + auto predicate = + PredicateBuilder::GreaterThan(/*field_index=*/0, "k", FieldType::BIGINT, Literal(100l)); + ASSERT_OK(SetReadSchema(reader.get(), arrow::schema(full_fields_), predicate, std::nullopt)); + + ASSERT_OK_AND_ASSIGN(std::vector rows, Collect(reader.get())); + EXPECT_TRUE(rows.empty()); + EXPECT_EQ(calls, 0); +} + +// The payload ranges come from the file metadata, so failing to compute them means the payload +// reads would fail too: the error surfaces from the read instead of being swallowed. +TEST_F(LateMaterializingFileBatchReaderTest, PayloadPreBufferRangeErrorFailsRead) { + auto data = BuildData({0, 1, 2, 3, 4}); + auto mock = std::make_unique(data, full_type_, /*batch_size=*/2); + mock->SetPreBufferRangeStatus(Status::IOError("pre-buffer range unavailable")); + ASSERT_OK_AND_ASSIGN(auto reader, LateMaterializingFileBatchReader::Create( + std::move(mock), GetArrowPool(pool_))); + int32_t calls = 0; + reader->SetPreBufferSink([&calls](std::vector>&&) { calls++; }); + auto predicate = + PredicateBuilder::GreaterOrEqual(/*field_index=*/0, "k", FieldType::BIGINT, Literal(2l)); + ASSERT_OK(SetReadSchema(reader.get(), arrow::schema(full_fields_), predicate, std::nullopt)); + + ASSERT_NOK_WITH_MSG(reader->NextBatch(), "pre-buffer range unavailable"); + EXPECT_EQ(calls, 0); +} + +// End to end below the prefetch layer: the ranges the inner LM reader reports mid-read must reach +// the shared read-ahead cache and be fetched, on top of the probe ranges registered up front. +TEST_F(LateMaterializingFileBatchReaderTest, PrefetchInnerRegistersPayloadPreBufferRanges) { + auto data = BuildData({0, 1, 2, 3, 4, 5, 6, 7, 8, 9}); + auto format_builder = + std::make_unique(data, full_type_, /*batch_size=*/3); + // Far enough apart that the cache's hole-size limit keeps them as two separate ranges. + format_builder->SetPreBufferRangesPerSchema({{{0, 1024}}, {{100000, 2048}}}); + LateMaterializingReaderBuilder builder(std::move(format_builder), GetArrowPool(pool_)); + auto mock_fs = std::make_shared(); + ASSERT_OK_AND_ASSIGN(std::shared_ptr executor, CreateDefaultExecutor(2)); + ASSERT_OK_AND_ASSIGN( + std::unique_ptr impl, + PrefetchFileBatchReaderImpl::Create( + /*data_file_path=*/"", /*data_file_size=*/0, &builder, mock_fs, + /*prefetch_max_parallel_num=*/1, /*batch_size=*/3, /*prefetch_batch_count=*/2, + /*enable_adaptive_prefetch_strategy=*/false, executor, + /*initialize_read_ranges=*/false, /*read_ahead_cache_enabled=*/true, CacheConfig(), + /*enable_io_metrics=*/false, WarmupLevel::DECODED, pool_, GetArrowPool(pool_))); + auto predicate = + PredicateBuilder::GreaterOrEqual(/*field_index=*/0, "k", FieldType::BIGINT, Literal(4l)); + ::ArrowSchema c_schema; + ASSERT_TRUE(arrow::ExportSchema(*arrow::schema(full_fields_), &c_schema).ok()); + ASSERT_OK(impl->SetReadSchema(&c_schema, predicate, std::nullopt)); + + ASSERT_OK_AND_ASSIGN(std::shared_ptr result, CollectStruct(impl.get())); + ASSERT_TRUE(result); + ASSERT_EQ(result->length(), 6); // k = 4..9 + std::shared_ptr metrics = impl->GetReaderMetrics(); + ASSERT_TRUE(metrics); + ASSERT_OK_AND_ASSIGN(uint64_t io_bytes, metrics->GetCounter(ReadAheadCacheMetrics::IO_BYTES)); + // Both the probe ranges registered before the read and the payload ranges registered during it. + EXPECT_EQ(io_bytes, 1024u + 2048u); + // The payload ranges are the ones that arrived mid-read, and none of them was dropped. + ASSERT_OK_AND_ASSIGN(uint64_t late_registered_bytes, + metrics->GetCounter(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES)); + EXPECT_EQ(late_registered_bytes, 2048u); + ASSERT_OK_AND_ASSIGN(uint64_t late_dropped_bytes, + metrics->GetCounter(ReadAheadCacheMetrics::LATE_DROPPED_BYTES)); + EXPECT_EQ(late_dropped_bytes, 0u); + impl->Close(); +} + } // namespace paimon::test diff --git a/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp b/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp index 2b2c7ec8b..8668d9a47 100644 --- a/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp +++ b/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp @@ -322,6 +322,16 @@ Result> PrefetchFileBatchReaderImpl auto reader = std::unique_ptr(new PrefetchFileBatchReaderImpl( readers, batch_size, prefetch_queue_capacity, enable_adaptive_prefetch_strategy, executor, cache, io_metrics, warmup_level, arrow_pool)); + for (const auto& sub_reader : reader->readers_) { + // The sub-readers whose byte ranges only become known once they have read some data (the + // late-materialization payload pass) report them here instead of through PreBufferRange(), + // which is asked once per read-range generation before any read. Capturing the impl raw is + // safe: it owns readers_, and CleanUp() joins the background thread before it goes away. + sub_reader->SetPreBufferSink( + [impl = reader.get()](std::vector>&& ranges) { + impl->RegisterLatePreBufferRanges(std::move(ranges)); + }); + } if (initialize_read_ranges) { // normally initialize read ranges should be false, as set read schema will refresh read // ranges, and set read schema will always be called before read. @@ -356,6 +366,7 @@ PrefetchFileBatchReaderImpl::PrefetchFileBatchReaderImpl( PrefetchFileBatchReaderImpl::~PrefetchFileBatchReaderImpl() { (void)CleanUp(); + ClearPreBufferSinks(); } Status PrefetchFileBatchReaderImpl::SetReadSchema( @@ -512,6 +523,9 @@ Status PrefetchFileBatchReaderImpl::CleanUp() { // SetReadSchema()/RefreshReadRanges() call cache_->Reset() right after CleanUp(), clearing the // cache's initialized state, so the next read-range generation must be allowed to Init again. cache_warmed_.store(false); + // The registration round of this generation ends here, before the cache is reset: a sub-reader + // reporting ranges from now on registers nothing until the next round is opened. + cache_round_.store(kNoCacheRound); clean_prefetch_queue(); prefetch_metrics_->queue_depth.store(0, kMetricsMemoryOrder); for (size_t i = 0; i < readers_pos_.size(); i++) { @@ -790,6 +804,49 @@ void PrefetchFileBatchReaderImpl::WarmCacheOnce() { // Init() only registers the ranges, so without this the first cache fetch races the readers' // first reads instead of running ahead of them. cache_->Warmup(); + // The round the ranges of this generation belong to, for the sub-readers that report theirs + // mid-read. Stored after Init() opened it, cleared by CleanUp() when it ends. + cache_round_.store(cache_->RegistrationRound()); +} + +void PrefetchFileBatchReaderImpl::RegisterLatePreBufferRanges( + std::vector>&& read_ranges) { + if (!cache_ || read_ranges.empty()) { + return; + } + const uint64_t round = cache_round_.load(); + if (round == kNoCacheRound) { + // The read-range generation these ranges belong to has ended, so nothing is going to read + // them. WarmCacheOnce() opens the next round. + return; + } + std::vector ranges; + ranges.reserve(read_ranges.size()); + for (const auto& read_range : read_ranges) { + ranges.emplace_back(read_range.first, read_range.second); + } + Result> first_added = cache_->AddRanges(std::move(ranges), round); + if (!first_added.ok()) { + SetReadStatus(first_added.status()); + return; + } + if (!first_added.value().has_value()) { + // Every range was already registered, or the registration round has ended (the cache was + // reset for a new read-range generation, or released by Close()). Nothing new to fetch + // either way. + return; + } + // Fetch from the first new range rather than from the start: the ranges registered before it + // belong to the pass that has already run. + cache_->Warmup(first_added.value().value()); +} + +void PrefetchFileBatchReaderImpl::ClearPreBufferSinks() { + // Belt and braces: the sinks capture this, and by now CleanUp() has joined the background + // thread, so no sub-reader can report a range any more. + for (const auto& sub_reader : readers_) { + sub_reader->SetPreBufferSink(nullptr); + } } void PrefetchFileBatchReaderImpl::Warmup() { @@ -1032,6 +1089,7 @@ void PrefetchFileBatchReaderImpl::Close() { for (const auto& reader : readers_) { reader->Close(); } + ClearPreBufferSinks(); } } // namespace paimon diff --git a/src/paimon/common/reader/prefetch_file_batch_reader_impl.h b/src/paimon/common/reader/prefetch_file_batch_reader_impl.h index b0b0c6f71..5dd2e8735 100644 --- a/src/paimon/common/reader/prefetch_file_batch_reader_impl.h +++ b/src/paimon/common/reader/prefetch_file_batch_reader_impl.h @@ -60,6 +60,11 @@ class Metrics; struct PrefetchMetricsState; struct PrefetchIoMetricsState; +// No read-ahead cache registration round is open, so the ranges a sub-reader reports belong to a +// read-range generation that is over. Matches ReadAheadCache::RegistrationRound() when the cache +// has no open round. +constexpr uint64_t kNoCacheRound = 0; + class PrefetchFileBatchReaderImpl : public PrefetchFileBatchReader { public: static Result> Create( @@ -141,6 +146,19 @@ class PrefetchFileBatchReaderImpl : public PrefetchFileBatchReader { /// SetReadStatus() rather than returned: a warmup hint for a file that may never be read must /// not fail an in-flight read. void WarmCacheOnce(); + + /// Registers byte ranges a sub-reader discovered mid-read (the late-materialization payload + /// pass) with the shared cache and starts fetching them from there. A no-op without a cache, + /// or when the read-range generation the ranges belong to has ended: the ranges are tagged with + /// the cache's registration round, so a report that outlives its generation - a cache reset by + /// SetReadSchema()/RefreshReadRanges(), or released by Close() - registers nothing instead of + /// prefetching bytes nobody reads. Errors are recorded via SetReadStatus() for the same reason + /// as in WarmCacheOnce(). + void RegisterLatePreBufferRanges(std::vector>&& read_ranges); + + /// Detaches the sinks installed on the sub-readers, so that a sub-reader outliving this reader + /// cannot call back into it. Called once the background thread has been joined. + void ClearPreBufferSinks(); void SetReadStatus(const Status& status); Status GetReadStatus() const; Result IsEofRange(const std::pair& read_range) const; @@ -196,6 +214,10 @@ class PrefetchFileBatchReaderImpl : public PrefetchFileBatchReader { // Guards the one-shot read-ahead cache Init/Warmup so it runs at most once per read-range // generation. Reset by CleanUp() alongside read_ranges_freshed_. std::atomic cache_warmed_{false}; + // The cache registration round WarmCacheOnce() opened for the current read-range generation, + // or kNoCacheRound when no round is open. Passed to ReadAheadCache::AddRanges() so that the + // ranges of a generation that has ended are not registered into the next one. + std::atomic cache_round_{kNoCacheRound}; const uint32_t prefetch_queue_capacity_; const bool enable_adaptive_prefetch_strategy_; const WarmupLevel warmup_level_; diff --git a/src/paimon/common/utils/read_ahead_cache.cpp b/src/paimon/common/utils/read_ahead_cache.cpp index 6e517aaaf..8aa7bfdab 100644 --- a/src/paimon/common/utils/read_ahead_cache.cpp +++ b/src/paimon/common/utils/read_ahead_cache.cpp @@ -27,7 +27,9 @@ #include #include #include +#include #include +#include #include "paimon/common/memory/bytes_utils.h" #include "paimon/common/metrics/atomic_counter_pair.h" @@ -82,6 +84,9 @@ void CopyRangeFromEntries(const std::vector& covering, const By } // namespace +// No registration round is open: Init() never ran, or the round it opened has ended. +constexpr uint64_t kNoRegistrationRound = 0; + class ReadAheadCache::Impl { public: Impl(const std::shared_ptr& stream, const CacheConfig& config, uint64_t file_size, @@ -89,10 +94,14 @@ class ReadAheadCache::Impl { ~Impl(); Status Init(std::vector&& ranges); + uint64_t RegistrationRound() const; + Result> AddRanges(std::vector&& ranges, + uint64_t expected_round); Result Read(const ByteRange& range, char* dest); void Reset(); void ReleaseBuffers(); void Warmup(); + void Warmup(uint64_t from_offset); void CollectMetrics(std::shared_ptr* metrics) const; private: @@ -103,15 +112,16 @@ class ReadAheadCache::Impl { /// Returns an empty vector on miss. Entries are copied (shared buffers) /// so the caller may use them after releasing the lock. std::vector FindCoveringEntries(const ByteRange& range); - void PreBuffer(uint64_t offset); - /// Mark, publish and fetch the pending ranges at the given indices. + /// Mark, publish and fetch the pending ranges from the given offset forward, bounded by the + /// pre-buffer limit. /// - /// Marking is_cached_ and publishing the promise-backed entries happen - /// atomically under the write lock, before any IO is dispatched, so a - /// reader racing the prefetch waits on the in-flight entries instead of - /// re-fetching the same bytes. - void Cache(std::vector pending_indices); + /// Selecting the ranges, marking is_cached_ and publishing the promise-backed entries all + /// happen atomically under the write lock, before any IO is dispatched: AddRanges() rewrites + /// pending_ranges_ and is_cached_ from another thread, and a reader racing the prefetch must + /// observe is_cached_ set only once the covering entries are already visible, so it waits on + /// the in-flight entries instead of re-fetching the same bytes. + void PreBuffer(uint64_t offset); /// Clear the prefetch state, waiting for the fetches still writing into the /// entry buffers. Leaves the block cache untouched. @@ -122,10 +132,14 @@ class ReadAheadCache::Impl { // Ordered by offset (so as to find a matching region by binary search) std::vector entries_; std::shared_ptr memory_pool_; - std::shared_mutex rw_mutex_; + mutable std::shared_mutex rw_mutex_; std::vector> is_cached_; std::vector pending_ranges_; - bool is_initialized_ = false; + // The round the currently registered ranges belong to, or kNoRegistrationRound when no round + // is open. Every Init() opens a new one, so ranges reported by a pass that outlived its round + // can be told apart from the ranges of the open round. + uint64_t registration_round_ = kNoRegistrationRound; + uint64_t last_registration_round_ = kNoRegistrationRound; // Caches the reads that no registered range covers, or null when the block // cache is disabled. Owns its own locking and counters. std::unique_ptr block_cache_; @@ -138,46 +152,13 @@ class ReadAheadCache::Impl { // The prefetch IO actually issued to the underlying stream. The block cache // counts its own fetches, which CollectMetrics() adds to these. AtomicCounterPair ios_; + // The ranges registered mid-read: the ones that were registered and the ones that were + // dropped, either as already registered or as belonging to a round that has ended. + AtomicCounterPair late_registered_; + AtomicCounterPair late_dropped_; }; -void ReadAheadCache::Impl::Cache(std::vector pending_indices) { - std::vector new_entries; - std::vector fetches; - // Mark is_cached_, publish the promise-backed entries and only then - // dispatch the IOs. The mark and the publication happen atomically under - // the write lock: a reader racing the prefetch observes is_cached_=true - // only once the covering entries are already visible, so it waits on - // their futures instead of issuing a duplicate underlying read. - { - std::unique_lock lock(rw_mutex_); - for (size_t idx : pending_indices) { - if (is_cached_[idx].exchange(true)) { - continue; - } - const ByteRange& range = pending_ranges_[idx]; - auto promise = std::make_shared>(); - auto future = promise->get_future(); - auto buffer = AllocateBytesKeepingPoolAlive(range.length, memory_pool_); - fetches.push_back({range, buffer, promise}); - new_entries.emplace_back(range, std::move(buffer), std::move(future)); - } - if (!new_entries.empty()) { - // Entries are never evicted: the cache holds every published - // range until ReleaseBuffers()/Reset(), so an in-flight fetch - // always keeps its entry and thus its future reachable. - std::vector merged(entries_.size() + new_entries.size()); - std::merge(entries_.begin(), entries_.end(), new_entries.begin(), new_entries.end(), - merged.begin()); - entries_ = std::move(merged); - } - } - DispatchFetches(fetches); -} - Status ReadAheadCache::Impl::Init(std::vector&& ranges) { - if (is_initialized_) { - return Status::Invalid("Cache has already been initialized"); - } PAIMON_ASSIGN_OR_RAISE( std::vector pending_ranges, ByteRangeCombiner::CoalesceByteRanges(std::move(ranges), config_.GetHoleSizeLimit(), @@ -186,38 +167,167 @@ Status ReadAheadCache::Impl::Init(std::vector&& ranges) { PAIMON_RETURN_NOT_OK(ValidateValueInRange(pending_range.offset, "range offset")); PAIMON_RETURN_NOT_OK(ValidateValueInRange(pending_range.length, "range length")); } - pending_ranges_ = pending_ranges; + // Locked like AddRanges(), which extends this very registration round from another thread. + std::unique_lock lock(rw_mutex_); + if (registration_round_ != kNoRegistrationRound) { + return Status::Invalid("Cache has already been initialized"); + } + pending_ranges_ = std::move(pending_ranges); is_cached_ = std::vector>(pending_ranges_.size()); for (auto& is_cached : is_cached_) { is_cached.store(false); } - is_initialized_ = true; + // Monotonic across Reset(), so that a round is never confused with an earlier one. + registration_round_ = ++last_registration_round_; return Status::OK(); } -void ReadAheadCache::Impl::PreBuffer(uint64_t offset) { - auto it = std::lower_bound(pending_ranges_.begin(), pending_ranges_.end(), offset, - [](const ByteRange& range, uint64_t offset) { - return range.offset + range.length <= offset; - }); - if (it == pending_ranges_.end() || it->offset > offset) { - return; +uint64_t ReadAheadCache::Impl::RegistrationRound() const { + std::shared_lock lock(rw_mutex_); + return registration_round_; +} + +Result> ReadAheadCache::Impl::AddRanges(std::vector&& ranges, + uint64_t expected_round) { + PAIMON_ASSIGN_OR_RAISE( + std::vector new_ranges, + ByteRangeCombiner::CoalesceByteRanges(std::move(ranges), config_.GetHoleSizeLimit(), + config_.GetLateRangeSizeLimit())); + for (const auto& new_range : new_ranges) { + PAIMON_RETURN_NOT_OK(ValidateValueInRange(new_range.offset, "range offset")); + PAIMON_RETURN_NOT_OK(ValidateValueInRange(new_range.length, "range length")); } - size_t start_idx = std::distance(pending_ranges_.begin(), it); - std::vector pending_indices; - size_t total_bytes = 0; - for (size_t i = start_idx; i < pending_ranges_.size(); ++i) { - total_bytes += pending_ranges_[i].length; - if (total_bytes > config_.GetPreBufferLimit()) { - break; + std::unique_lock lock(rw_mutex_); + if (expected_round == kNoRegistrationRound || expected_round != registration_round_) { + // The round these ranges belong to is not the open one: the cache was never initialized, + // its buffers were released, or it was reset for a new read-range generation. The reads + // keep issuing their own IO, as they did before these ranges became known. + for (const auto& new_range : new_ranges) { + late_dropped_.Add(new_range.length); + } + return std::optional{}; + } + // Merge the two sorted, internally disjoint lists, keeping the result disjoint so that + // FindCoveringEntries() may keep walking it: of a new range, only the parts no registered + // range covers are registered. is_cached_ is rebuilt alongside: it is indexed by + // pending_ranges_, and an already fetched range must not be fetched again. + std::vector merged; + std::vector merged_is_cached; + merged.reserve(pending_ranges_.size() + new_ranges.size()); + merged_is_cached.reserve(pending_ranges_.size() + new_ranges.size()); + std::optional first_added; + size_t old_idx = 0; + auto keep_registered = [&]() { + merged.push_back(pending_ranges_[old_idx]); + merged_is_cached.push_back(is_cached_[old_idx].load() ? 1 : 0); + ++old_idx; + }; + auto register_new = [&](uint64_t offset, uint64_t length) { + if (!first_added.has_value()) { + first_added = offset; + } + merged.emplace_back(offset, length); + merged_is_cached.push_back(0); + late_registered_.Add(length); + }; + for (const ByteRange& candidate : new_ranges) { + const uint64_t candidate_end = candidate.offset + candidate.length; + // Start of the part of the candidate that is neither registered nor dropped yet. + uint64_t cursor = candidate.offset; + uint64_t dropped_bytes = 0; + while (cursor < candidate_end) { + if (old_idx == pending_ranges_.size()) { + register_new(cursor, candidate_end - cursor); + break; + } + const ByteRange registered = pending_ranges_[old_idx]; + const uint64_t registered_end = registered.offset + registered.length; + if (registered_end <= cursor) { + // Entirely before what is left of the candidate, so its place in the merged list + // is settled. + keep_registered(); + continue; + } + if (registered.offset >= candidate_end) { + // The candidate ends before this one starts: the rest of it is new. + register_new(cursor, candidate_end - cursor); + break; + } + // Overlap: the hole before the registered range is new, and the overlapping part is + // dropped because the round that registered it is already fetching those bytes. The + // registered range is left for the next iteration to settle, as a later candidate may + // still start before it. + if (registered.offset > cursor) { + register_new(cursor, registered.offset - cursor); + } + dropped_bytes += + std::min(candidate_end, registered_end) - std::max(cursor, registered.offset); + cursor = registered_end; + } + if (dropped_bytes > 0) { + late_dropped_.Add(dropped_bytes); } - pending_indices.push_back(i); } + while (old_idx < pending_ranges_.size()) { + keep_registered(); + } + if (!first_added.has_value()) { + return std::optional{}; + } + pending_ranges_ = std::move(merged); + // std::atomic is neither copyable nor movable, so the inherited flags are carried over + // one by one instead of resizing in place. + is_cached_ = std::vector>(pending_ranges_.size()); + for (size_t i = 0; i < is_cached_.size(); ++i) { + is_cached_[i].store(merged_is_cached[i] != 0); + } + return first_added; +} - if (!pending_indices.empty()) { - Cache(std::move(pending_indices)); +void ReadAheadCache::Impl::PreBuffer(uint64_t offset) { + std::vector fetches; + { + std::unique_lock lock(rw_mutex_); + auto it = std::lower_bound(pending_ranges_.begin(), pending_ranges_.end(), offset, + [](const ByteRange& range, uint64_t offset) { + return range.offset + range.length <= offset; + }); + if (it == pending_ranges_.end() || it->offset > offset) { + return; + } + + std::vector new_entries; + size_t total_bytes = 0; + for (size_t i = std::distance(pending_ranges_.begin(), it); i < pending_ranges_.size(); + ++i) { + total_bytes += pending_ranges_[i].length; + if (total_bytes > config_.GetPreBufferLimit()) { + break; + } + if (is_cached_[i].exchange(true)) { + continue; + } + const ByteRange& range = pending_ranges_[i]; + auto promise = std::make_shared>(); + auto future = promise->get_future(); + auto buffer = AllocateBytesKeepingPoolAlive(range.length, memory_pool_); + fetches.push_back({range, buffer, promise}); + new_entries.emplace_back(range, std::move(buffer), std::move(future)); + } + if (!new_entries.empty()) { + // Entries are never evicted: the cache holds every published range until + // ReleaseBuffers()/Reset(), so an in-flight fetch always keeps its entry and thus + // its future reachable. + std::vector merged(entries_.size() + new_entries.size()); + std::merge(entries_.begin(), entries_.end(), new_entries.begin(), new_entries.end(), + merged.begin()); + entries_ = std::move(merged); + } } + // Dispatched OUTSIDE the lock, so that a fetch completing inline does not deadlock against a + // Read() waiting for it. + DispatchFetches(fetches); } ReadAheadCache::Impl::Impl(const std::shared_ptr& stream, const CacheConfig& config, @@ -245,6 +355,8 @@ void ReadAheadCache::Impl::Reset() { hits_.Reset(); misses_.Reset(); ios_.Reset(); + late_registered_.Reset(); + late_dropped_.Reset(); if (block_cache_ != nullptr) { // Only the counters: the blocks cache the file, not the registered // ranges, and a reader resetting the cache reads the same file again. @@ -271,7 +383,9 @@ void ReadAheadCache::Impl::ReleasePrefetchBuffers() { entries_.clear(); is_cached_.clear(); pending_ranges_.clear(); - is_initialized_ = false; + // Ends the registration round: the ranges a pass reports from here on belong to a round that + // is over, and AddRanges() drops them instead of registering bytes nobody reads. + registration_round_ = kNoRegistrationRound; // The read/io counters are deliberately kept: a reader closed at EOF must // still be able to report them through CollectMetrics(). } @@ -297,14 +411,28 @@ void ReadAheadCache::Impl::CollectMetrics(std::shared_ptr* metrics) con m->SetCounter(ReadAheadCacheMetrics::BLOCK_FETCH_BYTES, blocks.fetch_bytes); m->SetCounter(ReadAheadCacheMetrics::IO_COUNT, ios_.Count() + blocks.fetches); m->SetCounter(ReadAheadCacheMetrics::IO_BYTES, ios_.Bytes() + blocks.fetch_bytes); + m->SetCounter(ReadAheadCacheMetrics::LATE_REGISTERED, late_registered_.Count()); + m->SetCounter(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, late_registered_.Bytes()); + m->SetCounter(ReadAheadCacheMetrics::LATE_DROPPED, late_dropped_.Count()); + m->SetCounter(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, late_dropped_.Bytes()); } void ReadAheadCache::Impl::Warmup() { // Init() only registers the pending ranges; without this the first fetch // starts when the first Read() arrives, racing the reader's own miss fetch. - if (!pending_ranges_.empty()) { - PreBuffer(pending_ranges_.front().offset); + uint64_t from_offset = 0; + { + std::shared_lock lock(rw_mutex_); + if (pending_ranges_.empty()) { + return; + } + from_offset = pending_ranges_.front().offset; } + PreBuffer(from_offset); +} + +void ReadAheadCache::Impl::Warmup(uint64_t from_offset) { + PreBuffer(from_offset); } std::vector ReadAheadCache::Impl::FindCoveringEntries(const ByteRange& range) { @@ -396,6 +524,15 @@ Status ReadAheadCache::Init(std::vector&& ranges) { return impl_->Init(std::move(ranges)); } +Result> ReadAheadCache::AddRanges(std::vector&& ranges, + uint64_t expected_round) { + return impl_->AddRanges(std::move(ranges), expected_round); +} + +uint64_t ReadAheadCache::RegistrationRound() const { + return impl_->RegistrationRound(); +} + Result ReadAheadCache::Read(const ByteRange& range, char* dest) { return impl_->Read(range, dest); } @@ -412,6 +549,10 @@ void ReadAheadCache::Warmup() { impl_->Warmup(); } +void ReadAheadCache::Warmup(uint64_t from_offset) { + impl_->Warmup(from_offset); +} + void ReadAheadCache::CollectMetrics(std::shared_ptr* metrics) const { impl_->CollectMetrics(metrics); } diff --git a/src/paimon/common/utils/read_ahead_cache.h b/src/paimon/common/utils/read_ahead_cache.h index e44c277d2..9127a7dc3 100644 --- a/src/paimon/common/utils/read_ahead_cache.h +++ b/src/paimon/common/utils/read_ahead_cache.h @@ -24,6 +24,7 @@ #include #include +#include #include #include "paimon/fs/file_system.h" @@ -74,6 +75,20 @@ class PAIMON_EXPORT ReadAheadCacheMetrics { /// Total bytes requested by the IOs the cache itself issued to the /// underlying stream. static inline const char IO_BYTES[] = "read-ahead-cache.io.bytes"; + /// Ranges registered by AddRanges() after the cache was initialized, and the + /// bytes they cover. They are counted after coalescing and splitting, so the + /// count is the number of prefetch IOs the late ranges added rather than the + /// number of ranges the caller reported. + static inline const char LATE_REGISTERED[] = "read-ahead-cache.late.registered"; + static inline const char LATE_REGISTERED_BYTES[] = "read-ahead-cache.late.registered-bytes"; + /// The reported ranges AddRanges() did not register, and the bytes they + /// cover: a range already covered by an earlier registration, or a range of + /// a registration round that has ended. The count is the number of reported + /// ranges - after coalescing - that were dropped in whole or in part, so + /// `late.registered-bytes` and `late.dropped-bytes` together account for + /// every byte reported. + static inline const char LATE_DROPPED[] = "read-ahead-cache.late.dropped"; + static inline const char LATE_DROPPED_BYTES[] = "read-ahead-cache.late.dropped-bytes"; }; /// A byte range with offset and length. @@ -131,9 +146,36 @@ class PAIMON_EXPORT ReadAheadCache { /// @param ranges The byte ranges to be cached. /// @return Status of the operation. /// @note This method must be called before any Read() calls. Ranges will be coalesced based - /// on the cache configuration. + /// on the cache configuration. Every call opens a new registration round, see + /// RegistrationRound(). Status Init(std::vector&& ranges); + /// The identifier of the registration round Init() opened, or zero when no round is open: the + /// cache was never initialized, it was reset, or its buffers were released. Every Init() opens + /// a new round, so a caller that kept the identifier of the round it initialized can tell + /// AddRanges() which round the ranges it registers belong to. + uint64_t RegistrationRound() const; + + /// Register more byte ranges into an already initialized cache, for the ranges that only + /// become known while reading: the late-materialization payload pass learns which pages it + /// needs only after the probe pass has evaluated the predicate. + /// + /// Unlike Init(), this may be called repeatedly and concurrently with Read(). Read() finds its + /// covering entries by walking a disjoint, offset-ordered list, so of a new range only the + /// part no registered range covers is registered; the overlapping part is dropped, as the + /// round that registered it is already fetching those bytes. The registered part is cut into + /// ranges of at most `late_range_size_limit` bytes, one prefetch IO each, so a large pass is + /// fetched concurrently instead of in one long request. + /// + /// @param ranges The byte ranges to register on top of the ranges already registered. + /// @param expected_round The registration round the ranges belong to, as returned by + /// RegistrationRound() when that round was opened. Everything is dropped when the round is no + /// longer the open one, so a pass that outlived its round registers nothing: those bytes are + /// not going to be read. Zero is never an open round. + /// @return The offset of the first newly registered range, or nullopt when nothing was added. + Result> AddRanges(std::vector&& ranges, + uint64_t expected_round); + /// Read a range previously provided to Init(), copying the cached data /// directly into the given destination buffer. /// @@ -153,10 +195,15 @@ class PAIMON_EXPORT ReadAheadCache { /// when the first Read() arrives, racing the caller's own miss fetch. void Warmup(); - /// Collect the counters of the Read() calls, of the block cache and of the - /// IOs into the given metrics as counters named after - /// `ReadAheadCacheMetrics`. Only reads issued through Read() are counted - /// as hits, block hits or misses; the fetches the cache dispatches itself + /// Start fetching the registered ranges from `from_offset` forward, bounded by the + /// pre-buffer limit. Warmup() is this call made from the first registered range. + /// @param from_offset The offset to start fetching from, typically one AddRanges() returned. + void Warmup(uint64_t from_offset); + + /// Collect the counters of the Read() calls, of the block cache, of the IOs + /// and of the late registrations into the given metrics as counters named + /// after `ReadAheadCacheMetrics`. Only reads issued through Read() are + /// counted as hits, block hits or misses; the fetches the cache dispatches /// are counted in the io counters instead. /// @param metrics The metrics to write the counters into. A null /// pointer or a null shared pointer is a no-op. diff --git a/src/paimon/common/utils/read_ahead_cache_test.cpp b/src/paimon/common/utils/read_ahead_cache_test.cpp index 60fe87094..6c37655bf 100644 --- a/src/paimon/common/utils/read_ahead_cache_test.cpp +++ b/src/paimon/common/utils/read_ahead_cache_test.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -37,11 +38,14 @@ namespace paimon::test { // The range limits the tests exercise. The defaults are sized for real data -// files, which the small test files would never reach. +// files, which the small test files would never reach. The ranges registered +// mid-read are cut at the same limit as the ones registered up front, so a test +// that does not care about the late granularity sees one range per registration. CacheConfig TestCacheConfig(uint64_t range_size_limit, uint64_t hole_size_limit, uint64_t pre_buffer_limit) { CacheConfig config; config.SetRangeSizeLimit(range_size_limit); + config.SetLateRangeSizeLimit(range_size_limit); config.SetHoleSizeLimit(hole_size_limit); config.SetPreBufferLimit(pre_buffer_limit); return config; @@ -108,6 +112,13 @@ void AssertReadMiss(const ByteRange& range, ReadAheadCache* cache) { ASSERT_EQ(std::string(dest.size(), 'X'), dest); } +// Report the counter of the given metric name, or zero when it was not collected. +uint64_t CounterOf(const char* name, ReadAheadCache* cache) { + std::shared_ptr metrics = std::make_shared(); + cache->CollectMetrics(&metrics); + return metrics->GetCounter(name).value_or(0); +} + TEST(TestReadAheadCache, TestBasics) { CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, @@ -509,6 +520,253 @@ TEST(TestReadAheadCache, TestReinitAfterReset) { AssertReadMiss({20, 2}, &cache); } +// Test that AddRanges() registers ranges that only become known after the cache was +// initialized, so the reads that follow are served from the cache. +TEST(TestReadAheadCache, TestAddRangesRegistersNewRanges) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 10}}); + ReadAheadCache& cache = *cache_ptr; + + AssertReadMiss({16, 10}, &cache); + + ASSERT_OK_AND_ASSIGN(std::optional first_added, + cache.AddRanges({{16, 10}}, cache.RegistrationRound())); + ASSERT_TRUE(first_added.has_value()); + ASSERT_EQ(16u, first_added.value()); + + AssertReadEquals({16, 10}, "qrstuvwxyz", &cache); + // The range registered by Init() is still served. + AssertReadEquals({0, 10}, "abcdefghij", &cache); + + ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); + ASSERT_EQ(10u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, &cache)); + ASSERT_EQ(0u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); +} + +// Of a new range intersecting an already registered one, only the part the registered ranges do +// not cover is registered: the registered ranges stay disjoint, and the read the new range asked +// for is served instead of dropped along with it. +TEST(TestReadAheadCache, TestAddRangesClipsIntersectingRange) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 10}}); + ReadAheadCache& cache = *cache_ptr; + + ASSERT_OK_AND_ASSIGN(std::optional first_added, + cache.AddRanges({{5, 10}}, cache.RegistrationRound())); + ASSERT_TRUE(first_added.has_value()); + // [5, 10) is already registered, so the round starts at the first byte that is not. + ASSERT_EQ(10u, first_added.value()); + + // Served by the registered range and the newly registered one together, as they are adjacent. + AssertReadEquals({5, 10}, "fghijklmno", &cache); + AssertReadEquals({0, 10}, "abcdefghij", &cache); + + ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); + ASSERT_EQ(5u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, &cache)); + ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); + ASSERT_EQ(5u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); +} + +// A new range fully covered by the registered ones registers nothing at all. +TEST(TestReadAheadCache, TestAddRangesDropsCoveredRange) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 10}}); + ReadAheadCache& cache = *cache_ptr; + + ASSERT_OK_AND_ASSIGN(std::optional first_added, + cache.AddRanges({{2, 4}}, cache.RegistrationRound())); + ASSERT_FALSE(first_added.has_value()); + + AssertReadEquals({0, 10}, "abcdefghij", &cache); + ASSERT_EQ(0u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); + ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); + ASSERT_EQ(4u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); +} + +// The registered ranges stay offset-ordered and disjoint when the new ranges interleave with them, +// so that a read still finds its covering entries. +TEST(TestReadAheadCache, TestAddRangesInterleavesWithRegisteredRanges) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/26, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{4, 4}, {16, 4}}); + ReadAheadCache& cache = *cache_ptr; + + // Two ranges each straddling a registered one, plus one entirely in the hole between them. + ASSERT_OK_AND_ASSIGN(std::optional first_added, + cache.AddRanges({{2, 4}, {10, 2}, {18, 6}}, cache.RegistrationRound())); + ASSERT_TRUE(first_added.has_value()); + ASSERT_EQ(2u, first_added.value()); + + AssertReadEquals({2, 6}, "cdefgh", &cache); + AssertReadEquals({10, 2}, "kl", &cache); + AssertReadEquals({16, 8}, "qrstuvwx", &cache); + // The holes nobody registered are still misses. + AssertReadMiss({12, 4}, &cache); + + // [2, 4), [10, 12) and [20, 24) are new; [4, 8) and [16, 20) were registered already. + ASSERT_EQ(3u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); + ASSERT_EQ(8u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, &cache)); + ASSERT_EQ(2u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); + ASSERT_EQ(4u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); +} + +// A range registered mid-read is cut at the late range limit, so it is fetched by several +// concurrent requests rather than by one long one. The pieces are adjacent, so a read spanning +// them is still served. +TEST(TestReadAheadCache, TestAddRangesSplitsLateRanges) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/26, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); + config.SetLateRangeSizeLimit(5); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 5}}); + ReadAheadCache& cache = *cache_ptr; + + ASSERT_OK_AND_ASSIGN(std::optional first_added, + cache.AddRanges({{10, 15}}, cache.RegistrationRound())); + ASSERT_TRUE(first_added.has_value()); + ASSERT_EQ(10u, first_added.value()); + ASSERT_EQ(3u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); + ASSERT_EQ(15u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, &cache)); + + cache.Warmup(first_added.value()); + // One request per piece, instead of one request for the whole 15 bytes. + ASSERT_EQ(3u, CounterOf(ReadAheadCacheMetrics::IO_COUNT, &cache)); + + AssertReadEquals({10, 15}, "klmnopqrstuvwxy", &cache); + ASSERT_EQ(3u, CounterOf(ReadAheadCacheMetrics::IO_COUNT, &cache)); +} + +// AddRanges() rebuilds the per-range cached flags, so a range fetched before the call is not +// fetched a second time. +TEST(TestReadAheadCache, TestAddRangesKeepsCachedRanges) { + // A window of exactly one range, so that reading the first one never reaches the second. + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/10); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 10}}); + ReadAheadCache& cache = *cache_ptr; + + cache.Warmup(); + AssertReadEquals({0, 10}, "abcdefghij", &cache); + + auto io_count_of = [&cache]() { return CounterOf(ReadAheadCacheMetrics::IO_COUNT, &cache); }; + const uint64_t io_count_before = io_count_of(); + ASSERT_EQ(1u, io_count_before); + + ASSERT_OK_AND_ASSIGN(std::optional first_added, + cache.AddRanges({{16, 10}}, cache.RegistrationRound())); + ASSERT_TRUE(first_added.has_value()); + + AssertReadEquals({0, 10}, "abcdefghij", &cache); + ASSERT_EQ(io_count_before, io_count_of()); +} + +// A cache that was never initialized has no open registration round to extend. +TEST(TestReadAheadCache, TestAddRangesOnUninitializedCacheIsNoop) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::unique_ptr dir; + std::shared_ptr in = OpenTestFile(&dir, "data_file", content); + ReadAheadCache cache(in, config, /*file_size=*/0, TestPool()); + + ASSERT_EQ(0u, cache.RegistrationRound()); + ASSERT_OK_AND_ASSIGN(std::optional first_added, + cache.AddRanges({{0, 10}}, cache.RegistrationRound())); + ASSERT_FALSE(first_added.has_value()); + AssertReadMiss({0, 10}, &cache); + // Reported as dropped, so that the ranges nobody registers can be told from those nobody + // reports. + ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); + ASSERT_EQ(10u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); +} + +// Ranges of a round that has ended are dropped rather than registered into the round that +// replaced it: a pass whose read-range generation is gone has nobody left to read its bytes. +TEST(TestReadAheadCache, TestAddRangesOfEndedRoundIsNoop) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 10}}); + ReadAheadCache& cache = *cache_ptr; + + const uint64_t ended_round = cache.RegistrationRound(); + ASSERT_NE(0u, ended_round); + + cache.Reset(); + ASSERT_EQ(0u, cache.RegistrationRound()); + ASSERT_OK(cache.Init({{0, 10}})); + const uint64_t open_round = cache.RegistrationRound(); + ASSERT_NE(ended_round, open_round); + + ASSERT_OK_AND_ASSIGN(std::optional first_added, + cache.AddRanges({{16, 10}}, ended_round)); + ASSERT_FALSE(first_added.has_value()); + AssertReadMiss({16, 10}, &cache); + ASSERT_EQ(10u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); + + // The open round still accepts them. + ASSERT_OK_AND_ASSIGN(first_added, cache.AddRanges({{16, 10}}, open_round)); + ASSERT_TRUE(first_added.has_value()); + AssertReadEquals({16, 10}, "qrstuvwxyz", &cache); +} + +// Neither has a cache whose buffers were released: its registration round has ended. +TEST(TestReadAheadCache, TestAddRangesAfterReleaseBuffersIsNoop) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 10}}); + ReadAheadCache& cache = *cache_ptr; + + const uint64_t round = cache.RegistrationRound(); + cache.ReleaseBuffers(); + ASSERT_EQ(0u, cache.RegistrationRound()); + + ASSERT_OK_AND_ASSIGN(std::optional first_added, cache.AddRanges({{16, 10}}, round)); + ASSERT_FALSE(first_added.has_value()); + AssertReadMiss({16, 10}, &cache); +} + +// Warmup(offset) starts fetching from the given range rather than from the first one, which is +// what a pass registering its ranges mid-read needs: the earlier ranges belong to the pass that +// has already run. +TEST(TestReadAheadCache, TestWarmupFromOffset) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 5}, {8, 5}, {16, 5}}); + ReadAheadCache& cache = *cache_ptr; + + auto io_hook = paimon::IOHook::GetInstance(); + paimon::ScopeGuard guard([&io_hook]() { io_hook->Clear(); }); + // IOCount() only counts while armed; INT64_MAX never triggers the error mode. + io_hook->Reset(INT64_MAX, paimon::IOHook::Mode::RETURN_ERROR); + + cache.Warmup(/*from_offset=*/16); + // Only the range at 16 was fetched: the two before it were skipped. + ASSERT_EQ(io_hook->IOCount(), 1); + + AssertReadEquals({16, 5}, "qrstu", &cache); + ASSERT_EQ(io_hook->IOCount(), 1); +} + // Test that Init() merges ranges separated by a small hole, so a read // spanning the hole is served by the single coalesced entry. TEST(TestReadAheadCache, TestInitCoalescesSmallHoles) { diff --git a/src/paimon/testing/mock/mock_file_batch_reader.h b/src/paimon/testing/mock/mock_file_batch_reader.h index e017d19fd..8bc2348cf 100644 --- a/src/paimon/testing/mock/mock_file_batch_reader.h +++ b/src/paimon/testing/mock/mock_file_batch_reader.h @@ -77,6 +77,26 @@ class MockFileBatchReader : public PrefetchFileBatchReader { enable_randomize_batch_size_ = enabled; } + /// Hand out a different set of pre-buffer ranges per read schema, so that a test can tell the + /// passes of a reader that switches schemas mid-file apart: the first SetReadSchema() selects + /// index 0, the second index 1, and a pass beyond the configured sets reports no range. + void SetPreBufferRangesPerSchema( + std::vector>> ranges_by_schema) { + pre_buffer_ranges_by_schema_ = std::move(ranges_by_schema); + } + + void SetPreBufferRangeStatus(const Status& status) { + pre_buffer_range_status_ = status; + } + + Result>> PreBufferRange() override { + PAIMON_RETURN_NOT_OK(pre_buffer_range_status_); + if (schema_generation_ == 0 || schema_generation_ > pre_buffer_ranges_by_schema_.size()) { + return std::vector>{}; + } + return pre_buffer_ranges_by_schema_[schema_generation_ - 1]; + } + Status SetReadSchema(::ArrowSchema* read_schema, const std::shared_ptr& predicate, const std::optional& selection_bitmap) override { PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr arrow_schema, @@ -87,6 +107,7 @@ class MockFileBatchReader : public PrefetchFileBatchReader { // late-materialization reader moving from its probe pass to its payload pass, rely on it. current_pos_ = 0; previous_batch_first_row_num_ = std::numeric_limits::max(); + schema_generation_++; return Status::OK(); } @@ -276,6 +297,11 @@ class MockFileBatchReader : public PrefetchFileBatchReader { Status next_batch_status_; bool enable_randomize_batch_size_ = true; std::vector> read_ranges_; + // The pre-buffer ranges to report, one set per SetReadSchema() call, and how many of those + // calls have been made so far. + std::vector>> pre_buffer_ranges_by_schema_; + size_t schema_generation_ = 0; + Status pre_buffer_range_status_; int32_t warmup_count_ = 0; std::mt19937 random_engine_{std::random_device{}()}; // NOLINT(whitespace/braces) }; diff --git a/src/paimon/testing/mock/mock_file_system.h b/src/paimon/testing/mock/mock_file_system.h index 8df358271..704420801 100644 --- a/src/paimon/testing/mock/mock_file_system.h +++ b/src/paimon/testing/mock/mock_file_system.h @@ -43,8 +43,13 @@ class MockInputStream : public InputStream { Result Read(char* buffer, int64_t size, int64_t offset) override { return 0; } + /// Completes the read inline. Leaving the callback uncalled would leave the promise a + /// read-ahead cache attaches to it unresolved forever, so anything waiting for that fetch - + /// the cache releasing its buffers, or a read hitting the range - would hang. void ReadAsync(char* buffer, int64_t size, int64_t offset, - std::function&& callback) override {} + std::function&& callback) override { + callback(Status::OK()); + } Status Close() override { return Status::OK(); diff --git a/src/paimon/testing/mock/mock_format_reader_builder.h b/src/paimon/testing/mock/mock_format_reader_builder.h index 168196115..ddabe8ac4 100644 --- a/src/paimon/testing/mock/mock_format_reader_builder.h +++ b/src/paimon/testing/mock/mock_format_reader_builder.h @@ -19,6 +19,8 @@ #pragma once #include +#include +#include #include "arrow/type.h" #include "paimon/format/reader_builder.h" @@ -44,13 +46,23 @@ class MockFormatReaderBuilder : public ReaderBuilder { return this; } + /// Configure every reader this builder creates, see + /// MockFileBatchReader::SetPreBufferRangesPerSchema(). + void SetPreBufferRangesPerSchema( + std::vector>> ranges_by_schema) { + pre_buffer_ranges_by_schema_ = std::move(ranges_by_schema); + } + Result> Build( const std::shared_ptr& path) const override { - if (bitmap_) { - return std::make_unique(data_, schema_, bitmap_.value(), - read_batch_size_); + std::unique_ptr reader = + bitmap_ ? std::make_unique(data_, schema_, bitmap_.value(), + read_batch_size_) + : std::make_unique(data_, schema_, read_batch_size_); + if (!pre_buffer_ranges_by_schema_.empty()) { + reader->SetPreBufferRangesPerSchema(pre_buffer_ranges_by_schema_); } - return std::make_unique(data_, schema_, read_batch_size_); + return reader; } private: @@ -58,6 +70,7 @@ class MockFormatReaderBuilder : public ReaderBuilder { std::shared_ptr schema_; std::optional bitmap_; int32_t read_batch_size_; + std::vector>> pre_buffer_ranges_by_schema_; }; } // namespace paimon::test diff --git a/test/inte/read_inte_test.cpp b/test/inte/read_inte_test.cpp index 284cd362f..c383dd921 100644 --- a/test/inte/read_inte_test.cpp +++ b/test/inte/read_inte_test.cpp @@ -575,11 +575,22 @@ TEST_P(ReadInteTest, TestReadAheadCacheMetrics) { ASSERT_OK_AND_ASSIGN(uint64_t io_bytes, read_metrics->GetCounter(ReadAheadCacheMetrics::IO_BYTES)); ASSERT_GT(io_bytes, 0u); + // Only a late-materialization payload pass registers ranges mid-read, which this scan does + // not do, so the counters must be exposed and stay at zero. + ASSERT_OK_AND_ASSIGN(uint64_t late_registered, + read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_REGISTERED)); + ASSERT_EQ(late_registered, 0u); + ASSERT_OK(read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES)); + ASSERT_OK_AND_ASSIGN(uint64_t late_dropped, + read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_DROPPED)); + ASSERT_EQ(late_dropped, 0u); + ASSERT_OK(read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_DROPPED_BYTES)); } else { ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::READ_COUNT)); ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::READ_HITS)); ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::READ_MISSES)); ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::IO_COUNT)); + ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_REGISTERED)); } } From 170d152e7eaecd1a2094ded5aa1736f33009967d Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Fri, 11 Sep 2026 15:21:21 +0800 Subject: [PATCH 02/15] fix --- .../reader/prefetch_file_batch_reader.h | 2 +- include/paimon/utils/prefetch_cache_config.h | 49 +++++++++++++++++-- .../common/utils/byte_range_combiner.cpp | 47 ++++++++++++++++++ src/paimon/common/utils/byte_range_combiner.h | 14 ++++++ .../common/utils/byte_range_combiner_test.cpp | 49 +++++++++++++++++++ src/paimon/common/utils/read_ahead_cache.cpp | 5 +- 6 files changed, 160 insertions(+), 6 deletions(-) diff --git a/include/paimon/reader/prefetch_file_batch_reader.h b/include/paimon/reader/prefetch_file_batch_reader.h index 77f4aa467..d65a48e28 100644 --- a/include/paimon/reader/prefetch_file_batch_reader.h +++ b/include/paimon/reader/prefetch_file_batch_reader.h @@ -148,7 +148,7 @@ class PAIMON_EXPORT PrefetchFileBatchReader : public FileBatchReader { /// Installs the sink above, or clears it when `sink` is empty. By default a reader has no /// late byte ranges to report and ignores the sink. /// @param sink The sink to report late byte ranges to. - virtual void SetPreBufferSink(PreBufferSink /*sink*/) {} + virtual void SetPreBufferSink(PreBufferSink sink) {} }; } // namespace paimon diff --git a/include/paimon/utils/prefetch_cache_config.h b/include/paimon/utils/prefetch_cache_config.h index 122c2a497..88fe9a0ad 100644 --- a/include/paimon/utils/prefetch_cache_config.h +++ b/include/paimon/utils/prefetch_cache_config.h @@ -59,7 +59,7 @@ class PAIMON_EXPORT CacheConfig { } /// Returns the maximum gap size (in bytes) considered mergeable between - /// adjacent ranges. Defaults to 8 KiB. + /// adjacent ranges. Defaults to 512 KiB. uint64_t GetHoleSizeLimit() const { return hole_size_limit_; } @@ -103,6 +103,31 @@ class PAIMON_EXPORT CacheConfig { block_cache_limit_ = block_cache_limit; } + /// Returns the alignment (in bytes) the adaptive range size is rounded up to, which is also + /// the smallest size a range is cut to. Defaults to 4 MiB. + uint64_t GetRangeSplitAlignment() const { + return range_split_alignment_; + } + + /// Sets the alignment the adaptive range size is rounded up to. Zero turns the adaptive + /// sizing off, so the ranges are cut at the configured size limit instead. + void SetRangeSplitAlignment(uint64_t range_split_alignment) { + range_split_alignment_ = range_split_alignment; + } + + /// Returns the number of requests the adaptive range size aims to spread one round of + /// registered ranges over. Defaults to 10. + uint64_t GetRangeSplitConcurrency() const { + return range_split_concurrency_; + } + + /// Sets the number of requests the adaptive range size aims to spread one round of ranges + /// over. Zero turns the adaptive sizing off, so the ranges are cut at the configured size + /// limit instead. + void SetRangeSplitConcurrency(uint64_t range_split_concurrency) { + range_split_concurrency_ = range_split_concurrency; + } + private: // The defaults are aligned with the reader's request granularity and with // realistic data file sizes: @@ -117,11 +142,29 @@ class PAIMON_EXPORT CacheConfig { // at Init: those are fetched only just before they are read, and one range // is one request, so they are cut smaller than range_size_limit to be // fetched concurrently rather than in one long request. A read spanning - // several of them is still served, as they are adjacent. + // several of them is still served, as they are adjacent. It is an upper + // bound only: the size such a round is actually cut at is derived from the + // bytes it registers, see range_split_alignment below. + // - hole_size_limit trades bytes against requests: coalescing across a gap + // reads the gap too, but saves a request, and on remote storage a request + // costs a round trip whatever its size. The limit is therefore well above + // the page-sized gaps a filtered read leaves between the pages it keeps, + // which would otherwise each cost a request of their own. uint64_t range_size_limit_ = 32 * 1024 * 1024; uint64_t late_range_size_limit_ = 8 * 1024 * 1024; - uint64_t hole_size_limit_ = 8 * 1024; + uint64_t hole_size_limit_ = 512 * 1024; uint64_t pre_buffer_limit_ = 256 * 1024 * 1024; + // A fixed size limit cuts a small round into fewer requests than could be + // fetched at once, leaving the storage idle while each of them runs. So the + // size a round is cut at is derived from the round instead: its bytes are + // spread over range_split_concurrency requests, rounded up to + // range_split_alignment, and kept within the size limit above. A round + // smaller than alignment * concurrency is therefore cut finer than that + // limit and fetched in a single wave, while a larger one still stops at it. + // The alignment is also the floor, so the derivation never cuts a round into + // requests too small to amortize their own round trip. + uint64_t range_split_alignment_ = 4 * 1024 * 1024; + uint64_t range_split_concurrency_ = 10; // Blocks are aligned to the END of the file, so a block never reaches past // EOF. 64 KiB is the granularity the reads no prefetched range covers are // shared at: small enough that a metadata read at the tail of a file is diff --git a/src/paimon/common/utils/byte_range_combiner.cpp b/src/paimon/common/utils/byte_range_combiner.cpp index c306c100f..478921351 100644 --- a/src/paimon/common/utils/byte_range_combiner.cpp +++ b/src/paimon/common/utils/byte_range_combiner.cpp @@ -29,6 +29,37 @@ namespace paimon { +namespace { + +/// The size to cut `total_bytes` of ranges at so that they are spread over +/// `target_concurrency` requests, rounded up to `alignment` and kept in +/// (hole_size_limit, range_size_limit]. +uint64_t AdaptiveRangeSizeLimit(uint64_t total_bytes, uint64_t target_concurrency, + uint64_t alignment, uint64_t hole_size_limit, + uint64_t range_size_limit) { + // Leave the limit alone when the derivation is off, and when it is not larger than the hole + // size: CoalesceByteRanges() rejects such a pair, and it must report that as it always did + // rather than have a derived limit paper over it. + if (alignment == 0 || target_concurrency == 0 || range_size_limit <= hole_size_limit) { + return range_size_limit; + } + const uint64_t per_request = + total_bytes / target_concurrency + (total_bytes % target_concurrency == 0 ? 0 : 1); + // One alignment unit is the floor, and the limit must stay above the hole size for the + // reason above. hole_size_limit + 1 cannot overflow: it is below range_size_limit here. + uint64_t limit = std::max({alignment, per_request, hole_size_limit + 1}); + if (const uint64_t remainder = limit % alignment; remainder != 0) { + const uint64_t padding = alignment - remainder; + if (limit > std::numeric_limits::max() - padding) { + return range_size_limit; + } + limit += padding; + } + return std::min(limit, range_size_limit); +} + +} // namespace + Result> ByteRangeCombiner::CoalesceByteRanges( std::vector&& ranges, uint64_t hole_size_limit, uint64_t range_size_limit) { if (range_size_limit <= hole_size_limit) { @@ -137,4 +168,20 @@ Result> ByteRangeCombiner::CoalesceByteRanges( return coalesced; } +Result> ByteRangeCombiner::CoalesceByteRangesAdaptive( + std::vector&& ranges, uint64_t hole_size_limit, uint64_t range_size_limit, + uint64_t alignment, uint64_t target_concurrency) { + uint64_t total_bytes = 0; + for (const auto& range : ranges) { + // Saturate rather than wrap. CoalesceByteRanges() rejects the ranges that can add up + // this far, but only after the limit below has been derived from them. + total_bytes = range.length > std::numeric_limits::max() - total_bytes + ? std::numeric_limits::max() + : total_bytes + range.length; + } + return CoalesceByteRanges(std::move(ranges), hole_size_limit, + AdaptiveRangeSizeLimit(total_bytes, target_concurrency, alignment, + hole_size_limit, range_size_limit)); +} + } // namespace paimon diff --git a/src/paimon/common/utils/byte_range_combiner.h b/src/paimon/common/utils/byte_range_combiner.h index 90942719b..a211a93d3 100644 --- a/src/paimon/common/utils/byte_range_combiner.h +++ b/src/paimon/common/utils/byte_range_combiner.h @@ -32,6 +32,20 @@ struct ByteRangeCombiner { static Result> CoalesceByteRanges(std::vector&& ranges, uint64_t hole_size_limit, uint64_t range_size_limit); + + /// Coalesces `ranges` like CoalesceByteRanges() does, except that the size the ranges are + /// cut at is derived from how many bytes they cover instead of being fixed: the bytes are + /// spread over `target_concurrency` ranges, the result is rounded up to a multiple of + /// `alignment`, and it is kept within `range_size_limit`. So a small batch of ranges is cut + /// into as many ranges as can be fetched at once rather than into fewer, longer ones, while + /// a batch large enough to reach `range_size_limit` is cut exactly as CoalesceByteRanges() + /// would cut it. + /// + /// A zero `alignment` or `target_concurrency` turns the derivation off, falling back to + /// `range_size_limit`. + static Result> CoalesceByteRangesAdaptive( + std::vector&& ranges, uint64_t hole_size_limit, uint64_t range_size_limit, + uint64_t alignment, uint64_t target_concurrency); }; } // namespace paimon diff --git a/src/paimon/common/utils/byte_range_combiner_test.cpp b/src/paimon/common/utils/byte_range_combiner_test.cpp index 5a3cc1c6e..f38bd4792 100644 --- a/src/paimon/common/utils/byte_range_combiner_test.cpp +++ b/src/paimon/common/utils/byte_range_combiner_test.cpp @@ -99,4 +99,53 @@ TEST(ByteRangeCombinerTest, TestRejectsRangesBeyondInt64Bound) { check_invalid({{100, 10}, {0, std::numeric_limits::max()}}); } +// The adaptive variant derives the size the ranges are cut at from the bytes they cover, so a +// batch too small to fill the concurrency on its own is cut finer than the fixed limit cuts it. +TEST(ByteRangeCombinerTest, TestAdaptiveRangeSizeLimit) { + auto check = [](std::vector ranges, uint64_t alignment, uint64_t target_concurrency, + std::vector expected) -> void { + ASSERT_OK_AND_ASSIGN(auto coalesced, + ByteRangeCombiner::CoalesceByteRangesAdaptive( + std::move(ranges), /*hole_size_limit=*/1, + /*range_size_limit=*/8, alignment, target_concurrency)); + ASSERT_EQ(coalesced, expected); + }; + + // 20 bytes over 10 requests is 2 per request, which the alignment lifts to one 4 byte + // range each: cut at 4 instead of at the limit of 8. + check({{0, 20}}, /*alignment=*/4, /*target_concurrency=*/10, + {{0, 4}, {4, 4}, {8, 4}, {12, 4}, {16, 4}}); + // The same bytes cut at the fixed limit, for comparison: 3 ranges rather than 5. + check({{0, 20}}, /*alignment=*/0, /*target_concurrency=*/10, {{0, 8}, {8, 8}, {16, 4}}); + check({{0, 20}}, /*alignment=*/4, /*target_concurrency=*/0, {{0, 8}, {8, 8}, {16, 4}}); + // 30 bytes over 3 requests is 10 each, which rounds up past the limit, so the limit wins + // and the batch is cut exactly as the non-adaptive variant cuts it. + check({{0, 30}}, /*alignment=*/4, /*target_concurrency=*/3, {{0, 8}, {8, 8}, {16, 8}, {24, 6}}); + // The alignment is the floor: 10 bytes over 10 requests is 1 each, not cut into 1 byte + // requests that could never amortize their own round trip. + check({{0, 10}}, /*alignment=*/4, /*target_concurrency=*/10, {{0, 4}, {4, 4}, {8, 2}}); + // A derived limit still coalesces across the holes that fit within it, and still bounds the + // coalesced result: the third range would take it past 4 bytes, so it starts a new range. + check({{0, 1}, {2, 1}}, /*alignment=*/4, /*target_concurrency=*/10, {{0, 3}}); + check({{0, 1}, {2, 1}, {4, 1}}, /*alignment=*/4, /*target_concurrency=*/10, {{0, 3}, {4, 1}}); + check({}, /*alignment=*/4, /*target_concurrency=*/10, {}); +} + +// CoalesceByteRanges() rejects a size limit that does not exceed the hole size, so the derived +// limit must stay above it however small the batch is. +TEST(ByteRangeCombinerTest, TestAdaptiveRangeSizeLimitStaysAboveHoleSize) { + // 10 bytes over 10 requests would derive 4, below the hole size of 5. + ASSERT_OK_AND_ASSIGN(auto coalesced, ByteRangeCombiner::CoalesceByteRangesAdaptive( + {{0, 10}}, /*hole_size_limit=*/5, + /*range_size_limit=*/8, /*alignment=*/4, + /*target_concurrency=*/10)); + ASSERT_EQ(coalesced, (std::vector{{0, 8}, {8, 2}})); + + // A limit that cannot exceed the hole size is still reported, not papered over. + ASSERT_NOK_WITH_MSG(ByteRangeCombiner::CoalesceByteRangesAdaptive( + {{0, 10}}, /*hole_size_limit=*/8, /*range_size_limit=*/8, + /*alignment=*/4, /*target_concurrency=*/10), + "should be larger than hole size limit"); +} + } // namespace paimon::test diff --git a/src/paimon/common/utils/read_ahead_cache.cpp b/src/paimon/common/utils/read_ahead_cache.cpp index 8aa7bfdab..7e2da8a27 100644 --- a/src/paimon/common/utils/read_ahead_cache.cpp +++ b/src/paimon/common/utils/read_ahead_cache.cpp @@ -191,8 +191,9 @@ Result> ReadAheadCache::Impl::AddRanges(std::vector new_ranges, - ByteRangeCombiner::CoalesceByteRanges(std::move(ranges), config_.GetHoleSizeLimit(), - config_.GetLateRangeSizeLimit())); + ByteRangeCombiner::CoalesceByteRangesAdaptive( + std::move(ranges), config_.GetHoleSizeLimit(), config_.GetLateRangeSizeLimit(), + config_.GetRangeSplitAlignment(), config_.GetRangeSplitConcurrency())); for (const auto& new_range : new_ranges) { PAIMON_RETURN_NOT_OK(ValidateValueInRange(new_range.offset, "range offset")); PAIMON_RETURN_NOT_OK(ValidateValueInRange(new_range.length, "range length")); From 7e3741f5fd36e8b3a375e9164f564bb08f3680e3 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Tue, 15 Sep 2026 13:33:29 +0800 Subject: [PATCH 03/15] fix --- src/paimon/format/parquet/page_filtered_row_group_reader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp index 70048b125..17196198b 100644 --- a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp +++ b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp @@ -48,7 +48,7 @@ namespace { /// The estimate it caps is a heuristic over footer fields, which are attacker-controlled and /// need not describe the pages this read touches, so it must not turn into an unbounded eager /// allocation. Past this size the builder's doubling is already amortized against a large read. -constexpr int64_t kMaxMetadataValueBytesReservation = int64_t{16} * 1024 * 1024; +constexpr int64_t kMaxMetadataValueBytesReservation = int64_t{64} * 1024 * 1024; struct DataPageLayout { int64_t column_chunk_offset; From ad77fa32a07b5715b58db9226ffdad8b5b318b52 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Tue, 15 Sep 2026 13:36:38 +0800 Subject: [PATCH 04/15] fix --- src/paimon/format/parquet/page_filtered_row_group_reader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp index 17196198b..ee830dabf 100644 --- a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp +++ b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp @@ -48,7 +48,7 @@ namespace { /// The estimate it caps is a heuristic over footer fields, which are attacker-controlled and /// need not describe the pages this read touches, so it must not turn into an unbounded eager /// allocation. Past this size the builder's doubling is already amortized against a large read. -constexpr int64_t kMaxMetadataValueBytesReservation = int64_t{64} * 1024 * 1024; +constexpr int64_t kMaxMetadataValueBytesReservation = int64_t{128} * 1024 * 1024; struct DataPageLayout { int64_t column_chunk_offset; From ae3d0141476eba6f6e8ef26c9a7934b6631fdcc8 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Tue, 15 Sep 2026 13:39:43 +0800 Subject: [PATCH 05/15] fix --- src/paimon/format/parquet/page_filtered_row_group_reader.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp index ee830dabf..c6ab4aac6 100644 --- a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp +++ b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp @@ -20,6 +20,7 @@ #include "paimon/format/parquet/page_filtered_row_group_reader.h" #include +#include #include #include @@ -48,7 +49,7 @@ namespace { /// The estimate it caps is a heuristic over footer fields, which are attacker-controlled and /// need not describe the pages this read touches, so it must not turn into an unbounded eager /// allocation. Past this size the builder's doubling is already amortized against a large read. -constexpr int64_t kMaxMetadataValueBytesReservation = int64_t{128} * 1024 * 1024; +constexpr int64_t kMaxMetadataValueBytesReservation = std::numeric_limits::max(); struct DataPageLayout { int64_t column_chunk_offset; From 79a5fb04e6afebede6bf80648c67b07211c8ee07 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Tue, 15 Sep 2026 22:17:59 +0800 Subject: [PATCH 06/15] drop late --- include/paimon/utils/prefetch_cache_config.h | 39 ++++++------------- src/paimon/common/utils/read_ahead_cache.cpp | 2 +- src/paimon/common/utils/read_ahead_cache.h | 2 +- .../common/utils/read_ahead_cache_test.cpp | 6 +-- .../page_filtered_row_group_reader.cpp | 25 ++++++++---- 5 files changed, 34 insertions(+), 40 deletions(-) diff --git a/include/paimon/utils/prefetch_cache_config.h b/include/paimon/utils/prefetch_cache_config.h index 88fe9a0ad..6256b715c 100644 --- a/include/paimon/utils/prefetch_cache_config.h +++ b/include/paimon/utils/prefetch_cache_config.h @@ -34,8 +34,9 @@ namespace paimon { /// ReadAheadCache to balance memory usage, I/O efficiency, and latency hiding. class PAIMON_EXPORT CacheConfig { public: - /// Returns the maximum allowed size (in bytes) for a single cached range. - /// Defaults to 32 MiB. + /// Returns the maximum allowed size (in bytes) for a single cached range, both for the ranges + /// registered up front and, as the cap, for the ones registered mid-read. + /// Defaults to 8 MiB. uint64_t GetRangeSizeLimit() const { return range_size_limit_; } @@ -45,19 +46,6 @@ class PAIMON_EXPORT CacheConfig { range_size_limit_ = range_size_limit; } - /// Returns the maximum allowed size (in bytes) for a single range registered after the cache - /// was initialized, i.e. for the ranges that only become known while reading. - /// Defaults to 8 MiB. - uint64_t GetLateRangeSizeLimit() const { - return late_range_size_limit_; - } - - /// Sets the maximum allowed size (in bytes) for a single range registered after the cache was - /// initialized. - void SetLateRangeSizeLimit(uint64_t late_range_size_limit) { - late_range_size_limit_ = late_range_size_limit; - } - /// Returns the maximum gap size (in bytes) considered mergeable between /// adjacent ranges. Defaults to 512 KiB. uint64_t GetHoleSizeLimit() const { @@ -131,27 +119,24 @@ class PAIMON_EXPORT CacheConfig { private: // The defaults are aligned with the reader's request granularity and with // realistic data file sizes: - // - range_size_limit matches the parquet reader's 32 MiB request blocks - // (Arrow ReadRangeCache's own range limit); a smaller limit cuts entries - // below the request size, so a request can never be served from one piece. + // - range_size_limit bounds a single coalesced cached range, both for the + // ranges registered up front at Init and, as the cap, for the ones + // registered mid-read (see range_split_alignment below). One range is one + // prefetch IO, so a smaller limit spreads a large pass over several + // concurrent requests instead of one long serial one; a read spanning + // several adjacent ranges is still served as one hit, as they are + // contiguous. 8 MiB keeps a just-in-time mid-read pass concurrent while + // staying large enough to amortize each request's round trip. // - pre_buffer_limit must exceed the LARGEST single read a reader issues // (coalesced column-chunk reads of ~128 MiB were observed): fetches are // only dispatched up to this window, so a request reaching past it can // never be served and falls back to a second fetch of the same bytes. - // - late_range_size_limit bounds the ranges registered mid-read instead of - // at Init: those are fetched only just before they are read, and one range - // is one request, so they are cut smaller than range_size_limit to be - // fetched concurrently rather than in one long request. A read spanning - // several of them is still served, as they are adjacent. It is an upper - // bound only: the size such a round is actually cut at is derived from the - // bytes it registers, see range_split_alignment below. // - hole_size_limit trades bytes against requests: coalescing across a gap // reads the gap too, but saves a request, and on remote storage a request // costs a round trip whatever its size. The limit is therefore well above // the page-sized gaps a filtered read leaves between the pages it keeps, // which would otherwise each cost a request of their own. - uint64_t range_size_limit_ = 32 * 1024 * 1024; - uint64_t late_range_size_limit_ = 8 * 1024 * 1024; + uint64_t range_size_limit_ = 8 * 1024 * 1024; uint64_t hole_size_limit_ = 512 * 1024; uint64_t pre_buffer_limit_ = 256 * 1024 * 1024; // A fixed size limit cuts a small round into fewer requests than could be diff --git a/src/paimon/common/utils/read_ahead_cache.cpp b/src/paimon/common/utils/read_ahead_cache.cpp index 7e2da8a27..0113ed89e 100644 --- a/src/paimon/common/utils/read_ahead_cache.cpp +++ b/src/paimon/common/utils/read_ahead_cache.cpp @@ -192,7 +192,7 @@ Result> ReadAheadCache::Impl::AddRanges(std::vector new_ranges, ByteRangeCombiner::CoalesceByteRangesAdaptive( - std::move(ranges), config_.GetHoleSizeLimit(), config_.GetLateRangeSizeLimit(), + std::move(ranges), config_.GetHoleSizeLimit(), config_.GetRangeSizeLimit(), config_.GetRangeSplitAlignment(), config_.GetRangeSplitConcurrency())); for (const auto& new_range : new_ranges) { PAIMON_RETURN_NOT_OK(ValidateValueInRange(new_range.offset, "range offset")); diff --git a/src/paimon/common/utils/read_ahead_cache.h b/src/paimon/common/utils/read_ahead_cache.h index 9127a7dc3..3417f6c40 100644 --- a/src/paimon/common/utils/read_ahead_cache.h +++ b/src/paimon/common/utils/read_ahead_cache.h @@ -164,7 +164,7 @@ class PAIMON_EXPORT ReadAheadCache { /// covering entries by walking a disjoint, offset-ordered list, so of a new range only the /// part no registered range covers is registered; the overlapping part is dropped, as the /// round that registered it is already fetching those bytes. The registered part is cut into - /// ranges of at most `late_range_size_limit` bytes, one prefetch IO each, so a large pass is + /// ranges of at most `range_size_limit` bytes, one prefetch IO each, so a large pass is /// fetched concurrently instead of in one long request. /// /// @param ranges The byte ranges to register on top of the ranges already registered. diff --git a/src/paimon/common/utils/read_ahead_cache_test.cpp b/src/paimon/common/utils/read_ahead_cache_test.cpp index 6c37655bf..719931a07 100644 --- a/src/paimon/common/utils/read_ahead_cache_test.cpp +++ b/src/paimon/common/utils/read_ahead_cache_test.cpp @@ -45,7 +45,6 @@ CacheConfig TestCacheConfig(uint64_t range_size_limit, uint64_t hole_size_limit, uint64_t pre_buffer_limit) { CacheConfig config; config.SetRangeSizeLimit(range_size_limit); - config.SetLateRangeSizeLimit(range_size_limit); config.SetHoleSizeLimit(hole_size_limit); config.SetPreBufferLimit(pre_buffer_limit); return config; @@ -621,13 +620,12 @@ TEST(TestReadAheadCache, TestAddRangesInterleavesWithRegisteredRanges) { ASSERT_EQ(4u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); } -// A range registered mid-read is cut at the late range limit, so it is fetched by several +// A range registered mid-read is cut at the range size limit, so it is fetched by several // concurrent requests rather than by one long one. The pieces are adjacent, so a read spanning // them is still served. TEST(TestReadAheadCache, TestAddRangesSplitsLateRanges) { - CacheConfig config = TestCacheConfig(/*range_size_limit=*/26, + CacheConfig config = TestCacheConfig(/*range_size_limit=*/5, /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); - config.SetLateRangeSizeLimit(5); std::string content = "abcdefghijklmnopqrstuvwxyz"; std::shared_ptr cache_ptr = CreateTestFileAndCache("data_file", content, config, {{0, 5}}); diff --git a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp index c6ab4aac6..78f756fa0 100644 --- a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp +++ b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp @@ -45,11 +45,15 @@ namespace paimon::parquet { namespace { -/// Ceiling on the value bytes a leaf may reserve up front from column chunk metadata alone. -/// The estimate it caps is a heuristic over footer fields, which are attacker-controlled and -/// need not describe the pages this read touches, so it must not turn into an unbounded eager -/// allocation. Past this size the builder's doubling is already amortized against a large read. -constexpr int64_t kMaxMetadataValueBytesReservation = std::numeric_limits::max(); +/// Multiple of a column chunk's compressed size that a leaf may reserve up front in value bytes. +/// The estimate is a heuristic over footer fields, and `total_uncompressed_size` is a bare claim a +/// forged footer can inflate to make a leaf eagerly allocate gigabytes for a near-empty file. +/// `total_compressed_size` is instead checked against the file length before any read (Arrow's +/// ComputeColumnChunkRange, and GetDataPageLayout below, bound a chunk and its pages by it), so a +/// factor of it is bounded by bytes that physically exist. A leaf decompressing within the factor +/// is reserved in full; a more compressible one is capped and left to the builder's amortized +/// doubling, which is cheap against the I/O and decode of a read that large. +constexpr int64_t kMaxReservationDecompressionFactor = 8; struct DataPageLayout { int64_t column_chunk_offset; @@ -374,6 +378,8 @@ Result> PageFilteredRowGroupReader::ReadFil const int64_t reserve_values = effective_ranges.RowCount(); int64_t reserve_value_bytes = 0; const int64_t chunk_bytes = column_chunk ? column_chunk->total_uncompressed_size() : 0; + const int64_t chunk_compressed_bytes = + column_chunk ? column_chunk->total_compressed_size() : 0; const int64_t chunk_values = column_chunk ? column_chunk->num_values() : 0; if (chunk_bytes > 0 && chunk_values > 0 && reserve_values > 0) { // A heuristic, not a bound. total_uncompressed_size is uncompressed but still @@ -384,11 +390,16 @@ Result> PageFilteredRowGroupReader::ReadFil // including the pages this selection skips, so it misleads when wide values sit in // skipped pages. Either direction only costs performance — the reservation is a // hint the builder grows past when short — but they are why the result is capped - // instead of trusted. Fixed-width leaves ignore the byte count entirely. + // instead of trusted, by a factor of the file-bounded compressed size rather than the + // forgeable uncompressed one (see kMaxReservationDecompressionFactor). Fixed-width + // leaves ignore the byte count entirely. const double avg = static_cast(chunk_bytes) / static_cast(chunk_values); reserve_value_bytes = std::min( {SaturatingDoubleToInteger(avg * static_cast(reserve_values)), - chunk_bytes, kMaxMetadataValueBytesReservation}); + chunk_bytes, + SaturatingDoubleToInteger( + static_cast(chunk_compressed_bytes) * + static_cast(kMaxReservationDecompressionFactor))}); } PAIMON_RETURN_NOT_OK(ExecuteSkipReadPattern(col_idx, effective_ranges, effective_total, From 770a3c35793ad1cbde99a0c70940134d24541853 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Wed, 16 Sep 2026 11:14:38 +0800 Subject: [PATCH 07/15] simplelfy --- include/paimon/utils/prefetch_cache_config.h | 45 +----- .../common/io/cache_input_stream_test.cpp | 4 +- ...e_materializing_file_batch_reader_test.cpp | 7 - .../prefetch_file_batch_reader_impl.cpp | 39 ++--- .../reader/prefetch_file_batch_reader_impl.h | 19 +-- .../prefetch_file_batch_reader_impl_test.cpp | 4 +- .../common/utils/byte_range_combiner.cpp | 47 ------ src/paimon/common/utils/byte_range_combiner.h | 14 -- .../common/utils/byte_range_combiner_test.cpp | 49 ------ src/paimon/common/utils/read_ahead_cache.cpp | 112 +++---------- src/paimon/common/utils/read_ahead_cache.h | 70 +++----- .../common/utils/read_ahead_cache_test.cpp | 150 +++--------------- .../parquet_file_batch_reader_test.cpp | 2 +- test/inte/read_inte_test.cpp | 11 -- 14 files changed, 94 insertions(+), 479 deletions(-) diff --git a/include/paimon/utils/prefetch_cache_config.h b/include/paimon/utils/prefetch_cache_config.h index 6256b715c..d767fd929 100644 --- a/include/paimon/utils/prefetch_cache_config.h +++ b/include/paimon/utils/prefetch_cache_config.h @@ -91,40 +91,14 @@ class PAIMON_EXPORT CacheConfig { block_cache_limit_ = block_cache_limit; } - /// Returns the alignment (in bytes) the adaptive range size is rounded up to, which is also - /// the smallest size a range is cut to. Defaults to 4 MiB. - uint64_t GetRangeSplitAlignment() const { - return range_split_alignment_; - } - - /// Sets the alignment the adaptive range size is rounded up to. Zero turns the adaptive - /// sizing off, so the ranges are cut at the configured size limit instead. - void SetRangeSplitAlignment(uint64_t range_split_alignment) { - range_split_alignment_ = range_split_alignment; - } - - /// Returns the number of requests the adaptive range size aims to spread one round of - /// registered ranges over. Defaults to 10. - uint64_t GetRangeSplitConcurrency() const { - return range_split_concurrency_; - } - - /// Sets the number of requests the adaptive range size aims to spread one round of ranges - /// over. Zero turns the adaptive sizing off, so the ranges are cut at the configured size - /// limit instead. - void SetRangeSplitConcurrency(uint64_t range_split_concurrency) { - range_split_concurrency_ = range_split_concurrency; - } - private: // The defaults are aligned with the reader's request granularity and with // realistic data file sizes: // - range_size_limit bounds a single coalesced cached range, both for the - // ranges registered up front at Init and, as the cap, for the ones - // registered mid-read (see range_split_alignment below). One range is one - // prefetch IO, so a smaller limit spreads a large pass over several - // concurrent requests instead of one long serial one; a read spanning - // several adjacent ranges is still served as one hit, as they are + // ranges registered up front at Init and for the ones registered mid-read. + // One range is one prefetch IO, so a smaller limit spreads a large pass + // over several concurrent requests instead of one long serial one; a read + // spanning several adjacent ranges is still served as one hit, as they are // contiguous. 8 MiB keeps a just-in-time mid-read pass concurrent while // staying large enough to amortize each request's round trip. // - pre_buffer_limit must exceed the LARGEST single read a reader issues @@ -139,17 +113,6 @@ class PAIMON_EXPORT CacheConfig { uint64_t range_size_limit_ = 8 * 1024 * 1024; uint64_t hole_size_limit_ = 512 * 1024; uint64_t pre_buffer_limit_ = 256 * 1024 * 1024; - // A fixed size limit cuts a small round into fewer requests than could be - // fetched at once, leaving the storage idle while each of them runs. So the - // size a round is cut at is derived from the round instead: its bytes are - // spread over range_split_concurrency requests, rounded up to - // range_split_alignment, and kept within the size limit above. A round - // smaller than alignment * concurrency is therefore cut finer than that - // limit and fetched in a single wave, while a larger one still stops at it. - // The alignment is also the floor, so the derivation never cuts a round into - // requests too small to amortize their own round trip. - uint64_t range_split_alignment_ = 4 * 1024 * 1024; - uint64_t range_split_concurrency_ = 10; // Blocks are aligned to the END of the file, so a block never reaches past // EOF. 64 KiB is the granularity the reads no prefetched range covers are // shared at: small enough that a metadata read at the tail of a file is diff --git a/src/paimon/common/io/cache_input_stream_test.cpp b/src/paimon/common/io/cache_input_stream_test.cpp index 072af67bc..4c05c57c3 100644 --- a/src/paimon/common/io/cache_input_stream_test.cpp +++ b/src/paimon/common/io/cache_input_stream_test.cpp @@ -71,7 +71,7 @@ class CacheInputStreamTest : public ::testing::Test { // tests exercise the fallback of CacheInputStream on a cache miss. auto cache = std::make_shared(std::move(stream), config, /*file_size=*/0, pool_); - EXPECT_OK(cache->Init(std::move(ranges))); + EXPECT_OK(cache->AddRanges(std::move(ranges)).status()); return cache; } @@ -218,7 +218,7 @@ TEST_F(CacheInputStreamTest, TestReadAsyncCacheReadError) { config.SetPreBufferLimit(1024 * 1024); auto cache = std::make_shared(std::move(cache_stream), config, /*file_size=*/0, pool_); - ASSERT_OK(cache->Init(std::vector{{0, 10}})); + ASSERT_OK(cache->AddRanges(std::vector{{0, 10}}).status()); // Now activate IOHook so that the prefetch IO (triggered by cache_->Read -> PreBuffer) // will fail at the i-th IO operation diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp index 36c751a2a..acb4a23b6 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp +++ b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp @@ -896,13 +896,6 @@ TEST_F(LateMaterializingFileBatchReaderTest, PrefetchInnerRegistersPayloadPreBuf ASSERT_OK_AND_ASSIGN(uint64_t io_bytes, metrics->GetCounter(ReadAheadCacheMetrics::IO_BYTES)); // Both the probe ranges registered before the read and the payload ranges registered during it. EXPECT_EQ(io_bytes, 1024u + 2048u); - // The payload ranges are the ones that arrived mid-read, and none of them was dropped. - ASSERT_OK_AND_ASSIGN(uint64_t late_registered_bytes, - metrics->GetCounter(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES)); - EXPECT_EQ(late_registered_bytes, 2048u); - ASSERT_OK_AND_ASSIGN(uint64_t late_dropped_bytes, - metrics->GetCounter(ReadAheadCacheMetrics::LATE_DROPPED_BYTES)); - EXPECT_EQ(late_dropped_bytes, 0u); impl->Close(); } diff --git a/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp b/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp index 8668d9a47..a506dffa7 100644 --- a/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp +++ b/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp @@ -521,11 +521,10 @@ Status PrefetchFileBatchReaderImpl::CleanUp() { current_batch_global_row_ids_.clear(); read_ranges_freshed_ = false; // SetReadSchema()/RefreshReadRanges() call cache_->Reset() right after CleanUp(), clearing the - // cache's initialized state, so the next read-range generation must be allowed to Init again. + // cache's registered ranges, so the next read-range generation must be allowed to Init again. + // Clearing the flag also ends this generation's mid-read reports: a sub-reader reporting from + // here on registers nothing until the next generation warms the cache again. cache_warmed_.store(false); - // The registration round of this generation ends here, before the cache is reset: a sub-reader - // reporting ranges from now on registers nothing until the next round is opened. - cache_round_.store(kNoCacheRound); clean_prefetch_queue(); prefetch_metrics_->queue_depth.store(0, kMetricsMemoryOrder); for (size_t i = 0; i < readers_pos_.size(); i++) { @@ -780,10 +779,10 @@ void PrefetchFileBatchReaderImpl::WarmCacheOnce() { if (!cache_) { return; } - // Init() is not idempotent (a second call returns Invalid), so at most one warmup may run per - // read-range generation. A RAW Warmup() and the Workloop() call are ordered by the reader's own - // thread, which warms first and only then starts the background thread, so the loser of this - // exchange never continues on a cache another thread is still initializing. + // At most one warmup may run per read-range generation. A RAW Warmup() and the Workloop() call + // are ordered by the reader's own thread, which warms first and only then starts the background + // thread, so the loser of this exchange never continues on a cache another thread is still + // initializing. if (cache_warmed_.exchange(true)) { return; } @@ -796,17 +795,14 @@ void PrefetchFileBatchReaderImpl::WarmCacheOnce() { for (const auto& read_range : read_ranges.value()) { ranges.emplace_back(read_range.first, read_range.second); } - Status s = cache_->Init(std::move(ranges)); - if (!s.ok()) { - SetReadStatus(s); + Result> registered = cache_->AddRanges(std::move(ranges)); + if (!registered.ok()) { + SetReadStatus(registered.status()); return; } - // Init() only registers the ranges, so without this the first cache fetch races the readers' - // first reads instead of running ahead of them. + // AddRanges() only registers the ranges, so without this the first cache fetch races the + // readers' first reads instead of running ahead of them. cache_->Warmup(); - // The round the ranges of this generation belong to, for the sub-readers that report theirs - // mid-read. Stored after Init() opened it, cleared by CleanUp() when it ends. - cache_round_.store(cache_->RegistrationRound()); } void PrefetchFileBatchReaderImpl::RegisterLatePreBufferRanges( @@ -814,10 +810,9 @@ void PrefetchFileBatchReaderImpl::RegisterLatePreBufferRanges( if (!cache_ || read_ranges.empty()) { return; } - const uint64_t round = cache_round_.load(); - if (round == kNoCacheRound) { + if (!cache_warmed_.load()) { // The read-range generation these ranges belong to has ended, so nothing is going to read - // them. WarmCacheOnce() opens the next round. + // them. WarmCacheOnce() warms the next generation. return; } std::vector ranges; @@ -825,15 +820,13 @@ void PrefetchFileBatchReaderImpl::RegisterLatePreBufferRanges( for (const auto& read_range : read_ranges) { ranges.emplace_back(read_range.first, read_range.second); } - Result> first_added = cache_->AddRanges(std::move(ranges), round); + Result> first_added = cache_->AddRanges(std::move(ranges)); if (!first_added.ok()) { SetReadStatus(first_added.status()); return; } if (!first_added.value().has_value()) { - // Every range was already registered, or the registration round has ended (the cache was - // reset for a new read-range generation, or released by Close()). Nothing new to fetch - // either way. + // Every range was already registered, so there is nothing new to fetch. return; } // Fetch from the first new range rather than from the start: the ranges registered before it diff --git a/src/paimon/common/reader/prefetch_file_batch_reader_impl.h b/src/paimon/common/reader/prefetch_file_batch_reader_impl.h index 5dd2e8735..a0f170827 100644 --- a/src/paimon/common/reader/prefetch_file_batch_reader_impl.h +++ b/src/paimon/common/reader/prefetch_file_batch_reader_impl.h @@ -60,11 +60,6 @@ class Metrics; struct PrefetchMetricsState; struct PrefetchIoMetricsState; -// No read-ahead cache registration round is open, so the ranges a sub-reader reports belong to a -// read-range generation that is over. Matches ReadAheadCache::RegistrationRound() when the cache -// has no open round. -constexpr uint64_t kNoCacheRound = 0; - class PrefetchFileBatchReaderImpl : public PrefetchFileBatchReader { public: static Result> Create( @@ -148,9 +143,9 @@ class PrefetchFileBatchReaderImpl : public PrefetchFileBatchReader { void WarmCacheOnce(); /// Registers byte ranges a sub-reader discovered mid-read (the late-materialization payload - /// pass) with the shared cache and starts fetching them from there. A no-op without a cache, - /// or when the read-range generation the ranges belong to has ended: the ranges are tagged with - /// the cache's registration round, so a report that outlives its generation - a cache reset by + /// pass) with the shared cache and starts fetching them from there. A no-op without a cache, or + /// once the read-range generation the ranges belong to has ended (CleanUp() cleared + /// cache_warmed_): a report that outlives its generation - a cache reset by /// SetReadSchema()/RefreshReadRanges(), or released by Close() - registers nothing instead of /// prefetching bytes nobody reads. Errors are recorded via SetReadStatus() for the same reason /// as in WarmCacheOnce(). @@ -212,12 +207,10 @@ class PrefetchFileBatchReaderImpl : public PrefetchFileBatchReader { bool need_prefetch_ = false; bool read_ranges_freshed_ = false; // Guards the one-shot read-ahead cache Init/Warmup so it runs at most once per read-range - // generation. Reset by CleanUp() alongside read_ranges_freshed_. + // generation, and gates the mid-read range reports of that generation: CleanUp() clears it, so + // a sub-reader reporting after the generation ended registers nothing. Reset by CleanUp() + // alongside read_ranges_freshed_. std::atomic cache_warmed_{false}; - // The cache registration round WarmCacheOnce() opened for the current read-range generation, - // or kNoCacheRound when no round is open. Passed to ReadAheadCache::AddRanges() so that the - // ranges of a generation that has ended are not registered into the next one. - std::atomic cache_round_{kNoCacheRound}; const uint32_t prefetch_queue_capacity_; const bool enable_adaptive_prefetch_strategy_; const WarmupLevel warmup_level_; diff --git a/src/paimon/common/reader/prefetch_file_batch_reader_impl_test.cpp b/src/paimon/common/reader/prefetch_file_batch_reader_impl_test.cpp index aa0bd2e4e..7d0879d2a 100644 --- a/src/paimon/common/reader/prefetch_file_batch_reader_impl_test.cpp +++ b/src/paimon/common/reader/prefetch_file_batch_reader_impl_test.cpp @@ -1316,8 +1316,8 @@ TEST_P(PrefetchFileBatchReaderImplTest, TestWarmupLevelRaw) { WarmupLevel::RAW); reader->Warmup(); - // A second Warmup() must be a no-op: the one-shot guard prevents a duplicate cache Init(), - // which would otherwise fail with "Cache has already been initialized". + // A second Warmup() must be a no-op: the one-shot guard prevents opening the cache round + // twice, which would otherwise fail with "Cache has already been initialized". reader->Warmup(); std::shared_ptr metrics = reader->GetReaderMetrics(); diff --git a/src/paimon/common/utils/byte_range_combiner.cpp b/src/paimon/common/utils/byte_range_combiner.cpp index 478921351..c306c100f 100644 --- a/src/paimon/common/utils/byte_range_combiner.cpp +++ b/src/paimon/common/utils/byte_range_combiner.cpp @@ -29,37 +29,6 @@ namespace paimon { -namespace { - -/// The size to cut `total_bytes` of ranges at so that they are spread over -/// `target_concurrency` requests, rounded up to `alignment` and kept in -/// (hole_size_limit, range_size_limit]. -uint64_t AdaptiveRangeSizeLimit(uint64_t total_bytes, uint64_t target_concurrency, - uint64_t alignment, uint64_t hole_size_limit, - uint64_t range_size_limit) { - // Leave the limit alone when the derivation is off, and when it is not larger than the hole - // size: CoalesceByteRanges() rejects such a pair, and it must report that as it always did - // rather than have a derived limit paper over it. - if (alignment == 0 || target_concurrency == 0 || range_size_limit <= hole_size_limit) { - return range_size_limit; - } - const uint64_t per_request = - total_bytes / target_concurrency + (total_bytes % target_concurrency == 0 ? 0 : 1); - // One alignment unit is the floor, and the limit must stay above the hole size for the - // reason above. hole_size_limit + 1 cannot overflow: it is below range_size_limit here. - uint64_t limit = std::max({alignment, per_request, hole_size_limit + 1}); - if (const uint64_t remainder = limit % alignment; remainder != 0) { - const uint64_t padding = alignment - remainder; - if (limit > std::numeric_limits::max() - padding) { - return range_size_limit; - } - limit += padding; - } - return std::min(limit, range_size_limit); -} - -} // namespace - Result> ByteRangeCombiner::CoalesceByteRanges( std::vector&& ranges, uint64_t hole_size_limit, uint64_t range_size_limit) { if (range_size_limit <= hole_size_limit) { @@ -168,20 +137,4 @@ Result> ByteRangeCombiner::CoalesceByteRanges( return coalesced; } -Result> ByteRangeCombiner::CoalesceByteRangesAdaptive( - std::vector&& ranges, uint64_t hole_size_limit, uint64_t range_size_limit, - uint64_t alignment, uint64_t target_concurrency) { - uint64_t total_bytes = 0; - for (const auto& range : ranges) { - // Saturate rather than wrap. CoalesceByteRanges() rejects the ranges that can add up - // this far, but only after the limit below has been derived from them. - total_bytes = range.length > std::numeric_limits::max() - total_bytes - ? std::numeric_limits::max() - : total_bytes + range.length; - } - return CoalesceByteRanges(std::move(ranges), hole_size_limit, - AdaptiveRangeSizeLimit(total_bytes, target_concurrency, alignment, - hole_size_limit, range_size_limit)); -} - } // namespace paimon diff --git a/src/paimon/common/utils/byte_range_combiner.h b/src/paimon/common/utils/byte_range_combiner.h index a211a93d3..90942719b 100644 --- a/src/paimon/common/utils/byte_range_combiner.h +++ b/src/paimon/common/utils/byte_range_combiner.h @@ -32,20 +32,6 @@ struct ByteRangeCombiner { static Result> CoalesceByteRanges(std::vector&& ranges, uint64_t hole_size_limit, uint64_t range_size_limit); - - /// Coalesces `ranges` like CoalesceByteRanges() does, except that the size the ranges are - /// cut at is derived from how many bytes they cover instead of being fixed: the bytes are - /// spread over `target_concurrency` ranges, the result is rounded up to a multiple of - /// `alignment`, and it is kept within `range_size_limit`. So a small batch of ranges is cut - /// into as many ranges as can be fetched at once rather than into fewer, longer ones, while - /// a batch large enough to reach `range_size_limit` is cut exactly as CoalesceByteRanges() - /// would cut it. - /// - /// A zero `alignment` or `target_concurrency` turns the derivation off, falling back to - /// `range_size_limit`. - static Result> CoalesceByteRangesAdaptive( - std::vector&& ranges, uint64_t hole_size_limit, uint64_t range_size_limit, - uint64_t alignment, uint64_t target_concurrency); }; } // namespace paimon diff --git a/src/paimon/common/utils/byte_range_combiner_test.cpp b/src/paimon/common/utils/byte_range_combiner_test.cpp index f38bd4792..5a3cc1c6e 100644 --- a/src/paimon/common/utils/byte_range_combiner_test.cpp +++ b/src/paimon/common/utils/byte_range_combiner_test.cpp @@ -99,53 +99,4 @@ TEST(ByteRangeCombinerTest, TestRejectsRangesBeyondInt64Bound) { check_invalid({{100, 10}, {0, std::numeric_limits::max()}}); } -// The adaptive variant derives the size the ranges are cut at from the bytes they cover, so a -// batch too small to fill the concurrency on its own is cut finer than the fixed limit cuts it. -TEST(ByteRangeCombinerTest, TestAdaptiveRangeSizeLimit) { - auto check = [](std::vector ranges, uint64_t alignment, uint64_t target_concurrency, - std::vector expected) -> void { - ASSERT_OK_AND_ASSIGN(auto coalesced, - ByteRangeCombiner::CoalesceByteRangesAdaptive( - std::move(ranges), /*hole_size_limit=*/1, - /*range_size_limit=*/8, alignment, target_concurrency)); - ASSERT_EQ(coalesced, expected); - }; - - // 20 bytes over 10 requests is 2 per request, which the alignment lifts to one 4 byte - // range each: cut at 4 instead of at the limit of 8. - check({{0, 20}}, /*alignment=*/4, /*target_concurrency=*/10, - {{0, 4}, {4, 4}, {8, 4}, {12, 4}, {16, 4}}); - // The same bytes cut at the fixed limit, for comparison: 3 ranges rather than 5. - check({{0, 20}}, /*alignment=*/0, /*target_concurrency=*/10, {{0, 8}, {8, 8}, {16, 4}}); - check({{0, 20}}, /*alignment=*/4, /*target_concurrency=*/0, {{0, 8}, {8, 8}, {16, 4}}); - // 30 bytes over 3 requests is 10 each, which rounds up past the limit, so the limit wins - // and the batch is cut exactly as the non-adaptive variant cuts it. - check({{0, 30}}, /*alignment=*/4, /*target_concurrency=*/3, {{0, 8}, {8, 8}, {16, 8}, {24, 6}}); - // The alignment is the floor: 10 bytes over 10 requests is 1 each, not cut into 1 byte - // requests that could never amortize their own round trip. - check({{0, 10}}, /*alignment=*/4, /*target_concurrency=*/10, {{0, 4}, {4, 4}, {8, 2}}); - // A derived limit still coalesces across the holes that fit within it, and still bounds the - // coalesced result: the third range would take it past 4 bytes, so it starts a new range. - check({{0, 1}, {2, 1}}, /*alignment=*/4, /*target_concurrency=*/10, {{0, 3}}); - check({{0, 1}, {2, 1}, {4, 1}}, /*alignment=*/4, /*target_concurrency=*/10, {{0, 3}, {4, 1}}); - check({}, /*alignment=*/4, /*target_concurrency=*/10, {}); -} - -// CoalesceByteRanges() rejects a size limit that does not exceed the hole size, so the derived -// limit must stay above it however small the batch is. -TEST(ByteRangeCombinerTest, TestAdaptiveRangeSizeLimitStaysAboveHoleSize) { - // 10 bytes over 10 requests would derive 4, below the hole size of 5. - ASSERT_OK_AND_ASSIGN(auto coalesced, ByteRangeCombiner::CoalesceByteRangesAdaptive( - {{0, 10}}, /*hole_size_limit=*/5, - /*range_size_limit=*/8, /*alignment=*/4, - /*target_concurrency=*/10)); - ASSERT_EQ(coalesced, (std::vector{{0, 8}, {8, 2}})); - - // A limit that cannot exceed the hole size is still reported, not papered over. - ASSERT_NOK_WITH_MSG(ByteRangeCombiner::CoalesceByteRangesAdaptive( - {{0, 10}}, /*hole_size_limit=*/8, /*range_size_limit=*/8, - /*alignment=*/4, /*target_concurrency=*/10), - "should be larger than hole size limit"); -} - } // namespace paimon::test diff --git a/src/paimon/common/utils/read_ahead_cache.cpp b/src/paimon/common/utils/read_ahead_cache.cpp index 0113ed89e..aeea7161a 100644 --- a/src/paimon/common/utils/read_ahead_cache.cpp +++ b/src/paimon/common/utils/read_ahead_cache.cpp @@ -84,19 +84,13 @@ void CopyRangeFromEntries(const std::vector& covering, const By } // namespace -// No registration round is open: Init() never ran, or the round it opened has ended. -constexpr uint64_t kNoRegistrationRound = 0; - class ReadAheadCache::Impl { public: Impl(const std::shared_ptr& stream, const CacheConfig& config, uint64_t file_size, const std::shared_ptr& memory_pool); ~Impl(); - Status Init(std::vector&& ranges); - uint64_t RegistrationRound() const; - Result> AddRanges(std::vector&& ranges, - uint64_t expected_round); + Result> AddRanges(std::vector&& ranges); Result Read(const ByteRange& range, char* dest); void Reset(); void ReleaseBuffers(); @@ -105,6 +99,11 @@ class ReadAheadCache::Impl { void CollectMetrics(std::shared_ptr* metrics) const; private: + /// Merge coalesced, validated `new_ranges` into the disjoint, offset-ordered pending_ranges_, + /// rebuilding is_cached_ alongside, and return the offset of the first newly registered range + /// (nullopt when nothing was added). The caller holds the write lock. + std::optional MergeRangesLocked(std::vector&& new_ranges); + /// Dispatch the prefetch IOs for entries that have already been published /// into entries_. void DispatchFetches(const std::vector& fetches); @@ -135,11 +134,6 @@ class ReadAheadCache::Impl { mutable std::shared_mutex rw_mutex_; std::vector> is_cached_; std::vector pending_ranges_; - // The round the currently registered ranges belong to, or kNoRegistrationRound when no round - // is open. Every Init() opens a new one, so ranges reported by a pass that outlived its round - // can be told apart from the ranges of the open round. - uint64_t registration_round_ = kNoRegistrationRound; - uint64_t last_registration_round_ = kNoRegistrationRound; // Caches the reads that no registered range covers, or null when the block // cache is disabled. Owns its own locking and counters. std::unique_ptr block_cache_; @@ -152,63 +146,27 @@ class ReadAheadCache::Impl { // The prefetch IO actually issued to the underlying stream. The block cache // counts its own fetches, which CollectMetrics() adds to these. AtomicCounterPair ios_; - // The ranges registered mid-read: the ones that were registered and the ones that were - // dropped, either as already registered or as belonging to a round that has ended. - AtomicCounterPair late_registered_; - AtomicCounterPair late_dropped_; }; -Status ReadAheadCache::Impl::Init(std::vector&& ranges) { +Result> ReadAheadCache::Impl::AddRanges(std::vector&& ranges) { + // Both an up-front registration and a mid-read one are cut at the plain size limit. PAIMON_ASSIGN_OR_RAISE( - std::vector pending_ranges, + std::vector new_ranges, ByteRangeCombiner::CoalesceByteRanges(std::move(ranges), config_.GetHoleSizeLimit(), config_.GetRangeSizeLimit())); - for (const auto& pending_range : pending_ranges) { - PAIMON_RETURN_NOT_OK(ValidateValueInRange(pending_range.offset, "range offset")); - PAIMON_RETURN_NOT_OK(ValidateValueInRange(pending_range.length, "range length")); - } - // Locked like AddRanges(), which extends this very registration round from another thread. - std::unique_lock lock(rw_mutex_); - if (registration_round_ != kNoRegistrationRound) { - return Status::Invalid("Cache has already been initialized"); - } - pending_ranges_ = std::move(pending_ranges); - is_cached_ = std::vector>(pending_ranges_.size()); - for (auto& is_cached : is_cached_) { - is_cached.store(false); - } - // Monotonic across Reset(), so that a round is never confused with an earlier one. - registration_round_ = ++last_registration_round_; - return Status::OK(); -} - -uint64_t ReadAheadCache::Impl::RegistrationRound() const { - std::shared_lock lock(rw_mutex_); - return registration_round_; -} - -Result> ReadAheadCache::Impl::AddRanges(std::vector&& ranges, - uint64_t expected_round) { - PAIMON_ASSIGN_OR_RAISE( - std::vector new_ranges, - ByteRangeCombiner::CoalesceByteRangesAdaptive( - std::move(ranges), config_.GetHoleSizeLimit(), config_.GetRangeSizeLimit(), - config_.GetRangeSplitAlignment(), config_.GetRangeSplitConcurrency())); for (const auto& new_range : new_ranges) { PAIMON_RETURN_NOT_OK(ValidateValueInRange(new_range.offset, "range offset")); PAIMON_RETURN_NOT_OK(ValidateValueInRange(new_range.length, "range length")); } + // Locked: a registration rewrites the very pending_ranges_ and is_cached_ a concurrent Read() + // serves from, so it must not race the read finding its covering entries. std::unique_lock lock(rw_mutex_); - if (expected_round == kNoRegistrationRound || expected_round != registration_round_) { - // The round these ranges belong to is not the open one: the cache was never initialized, - // its buffers were released, or it was reset for a new read-range generation. The reads - // keep issuing their own IO, as they did before these ranges became known. - for (const auto& new_range : new_ranges) { - late_dropped_.Add(new_range.length); - } - return std::optional{}; - } + return MergeRangesLocked(std::move(new_ranges)); +} + +std::optional ReadAheadCache::Impl::MergeRangesLocked( + std::vector&& new_ranges) { // Merge the two sorted, internally disjoint lists, keeping the result disjoint so that // FindCoveringEntries() may keep walking it: of a new range, only the parts no registered // range covers are registered. is_cached_ is rebuilt alongside: it is indexed by @@ -230,13 +188,11 @@ Result> ReadAheadCache::Impl::AddRanges(std::vector> ReadAheadCache::Impl::AddRanges(std::vector cursor) { register_new(cursor, registered.offset - cursor); } - dropped_bytes += - std::min(candidate_end, registered_end) - std::max(cursor, registered.offset); cursor = registered_end; } - if (dropped_bytes > 0) { - late_dropped_.Add(dropped_bytes); - } } while (old_idx < pending_ranges_.size()) { keep_registered(); @@ -356,8 +306,6 @@ void ReadAheadCache::Impl::Reset() { hits_.Reset(); misses_.Reset(); ios_.Reset(); - late_registered_.Reset(); - late_dropped_.Reset(); if (block_cache_ != nullptr) { // Only the counters: the blocks cache the file, not the registered // ranges, and a reader resetting the cache reads the same file again. @@ -384,9 +332,6 @@ void ReadAheadCache::Impl::ReleasePrefetchBuffers() { entries_.clear(); is_cached_.clear(); pending_ranges_.clear(); - // Ends the registration round: the ranges a pass reports from here on belong to a round that - // is over, and AddRanges() drops them instead of registering bytes nobody reads. - registration_round_ = kNoRegistrationRound; // The read/io counters are deliberately kept: a reader closed at EOF must // still be able to report them through CollectMetrics(). } @@ -412,14 +357,10 @@ void ReadAheadCache::Impl::CollectMetrics(std::shared_ptr* metrics) con m->SetCounter(ReadAheadCacheMetrics::BLOCK_FETCH_BYTES, blocks.fetch_bytes); m->SetCounter(ReadAheadCacheMetrics::IO_COUNT, ios_.Count() + blocks.fetches); m->SetCounter(ReadAheadCacheMetrics::IO_BYTES, ios_.Bytes() + blocks.fetch_bytes); - m->SetCounter(ReadAheadCacheMetrics::LATE_REGISTERED, late_registered_.Count()); - m->SetCounter(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, late_registered_.Bytes()); - m->SetCounter(ReadAheadCacheMetrics::LATE_DROPPED, late_dropped_.Count()); - m->SetCounter(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, late_dropped_.Bytes()); } void ReadAheadCache::Impl::Warmup() { - // Init() only registers the pending ranges; without this the first fetch + // AddRanges() only registers the pending ranges; without this the first fetch // starts when the first Read() arrives, racing the reader's own miss fetch. uint64_t from_offset = 0; { @@ -521,17 +462,8 @@ ReadAheadCache::ReadAheadCache(const std::shared_ptr& stream, ReadAheadCache::~ReadAheadCache() = default; -Status ReadAheadCache::Init(std::vector&& ranges) { - return impl_->Init(std::move(ranges)); -} - -Result> ReadAheadCache::AddRanges(std::vector&& ranges, - uint64_t expected_round) { - return impl_->AddRanges(std::move(ranges), expected_round); -} - -uint64_t ReadAheadCache::RegistrationRound() const { - return impl_->RegistrationRound(); +Result> ReadAheadCache::AddRanges(std::vector&& ranges) { + return impl_->AddRanges(std::move(ranges)); } Result ReadAheadCache::Read(const ByteRange& range, char* dest) { diff --git a/src/paimon/common/utils/read_ahead_cache.h b/src/paimon/common/utils/read_ahead_cache.h index 3417f6c40..59bd9b9f5 100644 --- a/src/paimon/common/utils/read_ahead_cache.h +++ b/src/paimon/common/utils/read_ahead_cache.h @@ -75,20 +75,6 @@ class PAIMON_EXPORT ReadAheadCacheMetrics { /// Total bytes requested by the IOs the cache itself issued to the /// underlying stream. static inline const char IO_BYTES[] = "read-ahead-cache.io.bytes"; - /// Ranges registered by AddRanges() after the cache was initialized, and the - /// bytes they cover. They are counted after coalescing and splitting, so the - /// count is the number of prefetch IOs the late ranges added rather than the - /// number of ranges the caller reported. - static inline const char LATE_REGISTERED[] = "read-ahead-cache.late.registered"; - static inline const char LATE_REGISTERED_BYTES[] = "read-ahead-cache.late.registered-bytes"; - /// The reported ranges AddRanges() did not register, and the bytes they - /// cover: a range already covered by an earlier registration, or a range of - /// a registration round that has ended. The count is the number of reported - /// ranges - after coalescing - that were dropped in whole or in part, so - /// `late.registered-bytes` and `late.dropped-bytes` together account for - /// every byte reported. - static inline const char LATE_DROPPED[] = "read-ahead-cache.late.dropped"; - static inline const char LATE_DROPPED_BYTES[] = "read-ahead-cache.late.dropped-bytes"; }; /// A byte range with offset and length. @@ -128,7 +114,7 @@ struct PAIMON_EXPORT ByteRange { /// instead of being left to the caller. That block cache is owned by this one /// and shares its lifetime: it is configured from `block_size` and /// `block_cache_limit`, it survives Reset() - the blocks belong to the file -/// rather than to a registration round - and it is released by ReleaseBuffers(). +/// rather than to the registered ranges - and it is released by ReleaseBuffers(). class PAIMON_EXPORT ReadAheadCache { public: /// Construct a read cache with given options @@ -142,41 +128,27 @@ class PAIMON_EXPORT ReadAheadCache { uint64_t file_size, const std::shared_ptr& memory_pool); ~ReadAheadCache(); - /// Initialize the cache with given byte ranges to be cached. - /// @param ranges The byte ranges to be cached. - /// @return Status of the operation. - /// @note This method must be called before any Read() calls. Ranges will be coalesced based - /// on the cache configuration. Every call opens a new registration round, see - /// RegistrationRound(). - Status Init(std::vector&& ranges); - - /// The identifier of the registration round Init() opened, or zero when no round is open: the - /// cache was never initialized, it was reset, or its buffers were released. Every Init() opens - /// a new round, so a caller that kept the identifier of the round it initialized can tell - /// AddRanges() which round the ranges it registers belong to. - uint64_t RegistrationRound() const; - - /// Register more byte ranges into an already initialized cache, for the ranges that only - /// become known while reading: the late-materialization payload pass learns which pages it - /// needs only after the probe pass has evaluated the predicate. + /// Register byte ranges to prefetch, merging them into the ranges already registered. /// - /// Unlike Init(), this may be called repeatedly and concurrently with Read(). Read() finds its - /// covering entries by walking a disjoint, offset-ordered list, so of a new range only the - /// part no registered range covers is registered; the overlapping part is dropped, as the - /// round that registered it is already fetching those bytes. The registered part is cut into - /// ranges of at most `range_size_limit` bytes, one prefetch IO each, so a large pass is - /// fetched concurrently instead of in one long request. + /// This is the cache's only registration entry point, and it may be called repeatedly and + /// concurrently with Read(): the ranges known up front - a read plan's whole extent - are + /// registered before any Read(), and the ranges that only become known WHILE reading - a + /// late-materialization payload pass learns which pages it needs only after the probe pass has + /// evaluated the predicate - are registered as they are discovered. Read() finds its covering + /// entries by walking a disjoint, offset-ordered list, so of a new range only the part no + /// registered range covers is added; the overlapping part is dropped, as those bytes are + /// already being fetched. A registration is cut into ranges of at most `range_size_limit` + /// bytes, one prefetch IO each, so a large pass is fetched concurrently instead of in one long + /// request. /// - /// @param ranges The byte ranges to register on top of the ranges already registered. - /// @param expected_round The registration round the ranges belong to, as returned by - /// RegistrationRound() when that round was opened. Everything is dropped when the round is no - /// longer the open one, so a pass that outlived its round registers nothing: those bytes are - /// not going to be read. Zero is never an open round. + /// Deciding WHICH ranges to register - and in particular not reporting the ranges of a read + /// that has already ended - is the caller's concern; the cache registers whatever it is given. + /// + /// @param ranges The byte ranges to register. /// @return The offset of the first newly registered range, or nullopt when nothing was added. - Result> AddRanges(std::vector&& ranges, - uint64_t expected_round); + Result> AddRanges(std::vector&& ranges); - /// Read a range previously provided to Init(), copying the cached data + /// Read a range previously registered through AddRanges(), copying the cached data /// directly into the given destination buffer. /// /// Multi-segment hits are copied into `dest` segment by segment, without @@ -191,7 +163,7 @@ class PAIMON_EXPORT ReadAheadCache { Result Read(const ByteRange& range, char* dest); /// Start fetching the first batch of pending ranges immediately. - /// Init() only registers the ranges; without Warmup() the first fetch starts + /// AddRanges() only registers the ranges; without Warmup() the first fetch starts /// when the first Read() arrives, racing the caller's own miss fetch. void Warmup(); @@ -212,8 +184,8 @@ class PAIMON_EXPORT ReadAheadCache { /// Reset the cache to its initial state, clearing all cached data and configuration. /// /// This method waits for all ongoing asynchronous read operations to complete, - /// clears all cached entries, and resets the internal state so that Init() can be called again. - /// After calling Reset, the cache can be safely re-initialized with new ranges. + /// clears all cached entries, and resets the internal state so a fresh set of ranges can be + /// registered again with AddRanges(). /// /// The block cache is kept: it caches the file rather than the registered /// ranges, and a reader reusing the cache reads the same file again. diff --git a/src/paimon/common/utils/read_ahead_cache_test.cpp b/src/paimon/common/utils/read_ahead_cache_test.cpp index 719931a07..382a7bb3b 100644 --- a/src/paimon/common/utils/read_ahead_cache_test.cpp +++ b/src/paimon/common/utils/read_ahead_cache_test.cpp @@ -88,7 +88,7 @@ std::shared_ptr CreateTestFileAndCache(const std::string& filena std::shared_ptr in = OpenTestFile(&dir, filename, content); std::shared_ptr cache = std::make_shared(in, config, file_size, TestPool()); - EXPECT_OK(cache->Init(std::move(ranges))); + EXPECT_OK(cache->AddRanges(std::move(ranges)).status()); return cache; } @@ -152,7 +152,7 @@ TEST(TestReadAheadCache, TestMultiSegmentContiguousHit) { CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, /*hole_size_limit=*/2, /*pre_buffer_limit=*/1024); std::string content = "abcdefghijklmnopqrstuvwxyz"; - // A single 25-byte range exceeds range_size_limit, so Init() coalesces it + // A single 25-byte range exceeds range_size_limit, so AddRanges() coalesces and splits it // into three adjacent entries: {0,10}, {10,10} and {20,5}. std::shared_ptr cache_ptr = CreateTestFileAndCache("data_file", content, config, {{0, 25}}); @@ -390,7 +390,7 @@ TEST(TestReadAheadCache, TestInFlightEntryServesRacingReader) { auto gated = std::make_shared(OpenTestFile(&dir, "data_file", content)); ReadAheadCache cache(gated, config, /*file_size=*/0, TestPool()); - ASSERT_OK(cache.Init({{0, 5}})); + ASSERT_OK(cache.AddRanges({{0, 5}}).status()); cache.Warmup(); // The prefetch entry is published, but its fetch is still held. @@ -440,7 +440,7 @@ TEST(TestReadAheadCache, TestPrefetchBufferKeepsPoolAliveAfterCacheIsGone) { // Declared after the pool, so the cache is destroyed before the last // reference of this test to the pool is dropped. ReadAheadCache cache(gated, config, /*file_size=*/0, pool); - ASSERT_OK(cache.Init({{0, 5}})); + ASSERT_OK(cache.AddRanges({{0, 5}}).status()); cache.Warmup(); ASSERT_EQ(gated->AsyncReadCount(), 1); // The fetch is completed, but the stream keeps its callback - and with it @@ -484,22 +484,6 @@ TEST(TestReadAheadCache, TestPreBufferWindowLimit) { ASSERT_EQ(io_hook->IOCount(), 0); } -// Test that Init() rejects a second call until the cache is reset. -TEST(TestReadAheadCache, TestDoubleInit) { - CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, - /*hole_size_limit=*/2, /*pre_buffer_limit=*/1024); - std::string content = "abcdefghijklmnopqrstuvwxyz"; - std::shared_ptr cache_ptr = - CreateTestFileAndCache("data_file", content, config, {{0, 5}}); - ReadAheadCache& cache = *cache_ptr; - - Status status = cache.Init({{8, 5}}); - ASSERT_FALSE(status.ok()); - - // The original ranges still work. - AssertReadEquals({0, 5}, "abcde", &cache); -} - // Test that the cache can be re-initialized after Reset() and serves the new ranges. TEST(TestReadAheadCache, TestReinitAfterReset) { CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, @@ -512,7 +496,7 @@ TEST(TestReadAheadCache, TestReinitAfterReset) { AssertReadEquals({0, 5}, "abcde", &cache); cache.Reset(); - ASSERT_OK(cache.Init({{3, 4}})); + ASSERT_OK(cache.AddRanges({{3, 4}}).status()); AssertReadEquals({3, 4}, "defg", &cache); // The old ranges are gone. @@ -531,18 +515,13 @@ TEST(TestReadAheadCache, TestAddRangesRegistersNewRanges) { AssertReadMiss({16, 10}, &cache); - ASSERT_OK_AND_ASSIGN(std::optional first_added, - cache.AddRanges({{16, 10}}, cache.RegistrationRound())); + ASSERT_OK_AND_ASSIGN(std::optional first_added, cache.AddRanges({{16, 10}})); ASSERT_TRUE(first_added.has_value()); ASSERT_EQ(16u, first_added.value()); AssertReadEquals({16, 10}, "qrstuvwxyz", &cache); - // The range registered by Init() is still served. + // The range registered first is still served. AssertReadEquals({0, 10}, "abcdefghij", &cache); - - ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); - ASSERT_EQ(10u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, &cache)); - ASSERT_EQ(0u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); } // Of a new range intersecting an already registered one, only the part the registered ranges do @@ -556,20 +535,14 @@ TEST(TestReadAheadCache, TestAddRangesClipsIntersectingRange) { CreateTestFileAndCache("data_file", content, config, {{0, 10}}); ReadAheadCache& cache = *cache_ptr; - ASSERT_OK_AND_ASSIGN(std::optional first_added, - cache.AddRanges({{5, 10}}, cache.RegistrationRound())); + ASSERT_OK_AND_ASSIGN(std::optional first_added, cache.AddRanges({{5, 10}})); ASSERT_TRUE(first_added.has_value()); - // [5, 10) is already registered, so the round starts at the first byte that is not. + // [5, 10) is already registered, so the registration starts at the first byte that is not. ASSERT_EQ(10u, first_added.value()); // Served by the registered range and the newly registered one together, as they are adjacent. AssertReadEquals({5, 10}, "fghijklmno", &cache); AssertReadEquals({0, 10}, "abcdefghij", &cache); - - ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); - ASSERT_EQ(5u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, &cache)); - ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); - ASSERT_EQ(5u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); } // A new range fully covered by the registered ones registers nothing at all. @@ -581,14 +554,10 @@ TEST(TestReadAheadCache, TestAddRangesDropsCoveredRange) { CreateTestFileAndCache("data_file", content, config, {{0, 10}}); ReadAheadCache& cache = *cache_ptr; - ASSERT_OK_AND_ASSIGN(std::optional first_added, - cache.AddRanges({{2, 4}}, cache.RegistrationRound())); + ASSERT_OK_AND_ASSIGN(std::optional first_added, cache.AddRanges({{2, 4}})); ASSERT_FALSE(first_added.has_value()); AssertReadEquals({0, 10}, "abcdefghij", &cache); - ASSERT_EQ(0u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); - ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); - ASSERT_EQ(4u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); } // The registered ranges stay offset-ordered and disjoint when the new ranges interleave with them, @@ -603,7 +572,7 @@ TEST(TestReadAheadCache, TestAddRangesInterleavesWithRegisteredRanges) { // Two ranges each straddling a registered one, plus one entirely in the hole between them. ASSERT_OK_AND_ASSIGN(std::optional first_added, - cache.AddRanges({{2, 4}, {10, 2}, {18, 6}}, cache.RegistrationRound())); + cache.AddRanges({{2, 4}, {10, 2}, {18, 6}})); ASSERT_TRUE(first_added.has_value()); ASSERT_EQ(2u, first_added.value()); @@ -612,18 +581,12 @@ TEST(TestReadAheadCache, TestAddRangesInterleavesWithRegisteredRanges) { AssertReadEquals({16, 8}, "qrstuvwx", &cache); // The holes nobody registered are still misses. AssertReadMiss({12, 4}, &cache); - - // [2, 4), [10, 12) and [20, 24) are new; [4, 8) and [16, 20) were registered already. - ASSERT_EQ(3u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); - ASSERT_EQ(8u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, &cache)); - ASSERT_EQ(2u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); - ASSERT_EQ(4u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); } -// A range registered mid-read is cut at the range size limit, so it is fetched by several -// concurrent requests rather than by one long one. The pieces are adjacent, so a read spanning -// them is still served. -TEST(TestReadAheadCache, TestAddRangesSplitsLateRanges) { +// A registered range larger than the range size limit is cut into pieces, so it is fetched by +// several concurrent requests rather than by one long one. The pieces are adjacent, so a read +// spanning them is still served. +TEST(TestReadAheadCache, TestAddRangesSplitsLargeRange) { CacheConfig config = TestCacheConfig(/*range_size_limit=*/5, /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); std::string content = "abcdefghijklmnopqrstuvwxyz"; @@ -631,12 +594,9 @@ TEST(TestReadAheadCache, TestAddRangesSplitsLateRanges) { CreateTestFileAndCache("data_file", content, config, {{0, 5}}); ReadAheadCache& cache = *cache_ptr; - ASSERT_OK_AND_ASSIGN(std::optional first_added, - cache.AddRanges({{10, 15}}, cache.RegistrationRound())); + ASSERT_OK_AND_ASSIGN(std::optional first_added, cache.AddRanges({{10, 15}})); ASSERT_TRUE(first_added.has_value()); ASSERT_EQ(10u, first_added.value()); - ASSERT_EQ(3u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED, &cache)); - ASSERT_EQ(15u, CounterOf(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES, &cache)); cache.Warmup(first_added.value()); // One request per piece, instead of one request for the whole 15 bytes. @@ -664,83 +624,13 @@ TEST(TestReadAheadCache, TestAddRangesKeepsCachedRanges) { const uint64_t io_count_before = io_count_of(); ASSERT_EQ(1u, io_count_before); - ASSERT_OK_AND_ASSIGN(std::optional first_added, - cache.AddRanges({{16, 10}}, cache.RegistrationRound())); + ASSERT_OK_AND_ASSIGN(std::optional first_added, cache.AddRanges({{16, 10}})); ASSERT_TRUE(first_added.has_value()); AssertReadEquals({0, 10}, "abcdefghij", &cache); ASSERT_EQ(io_count_before, io_count_of()); } -// A cache that was never initialized has no open registration round to extend. -TEST(TestReadAheadCache, TestAddRangesOnUninitializedCacheIsNoop) { - CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, - /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); - std::string content = "abcdefghijklmnopqrstuvwxyz"; - std::unique_ptr dir; - std::shared_ptr in = OpenTestFile(&dir, "data_file", content); - ReadAheadCache cache(in, config, /*file_size=*/0, TestPool()); - - ASSERT_EQ(0u, cache.RegistrationRound()); - ASSERT_OK_AND_ASSIGN(std::optional first_added, - cache.AddRanges({{0, 10}}, cache.RegistrationRound())); - ASSERT_FALSE(first_added.has_value()); - AssertReadMiss({0, 10}, &cache); - // Reported as dropped, so that the ranges nobody registers can be told from those nobody - // reports. - ASSERT_EQ(1u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED, &cache)); - ASSERT_EQ(10u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); -} - -// Ranges of a round that has ended are dropped rather than registered into the round that -// replaced it: a pass whose read-range generation is gone has nobody left to read its bytes. -TEST(TestReadAheadCache, TestAddRangesOfEndedRoundIsNoop) { - CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, - /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); - std::string content = "abcdefghijklmnopqrstuvwxyz"; - std::shared_ptr cache_ptr = - CreateTestFileAndCache("data_file", content, config, {{0, 10}}); - ReadAheadCache& cache = *cache_ptr; - - const uint64_t ended_round = cache.RegistrationRound(); - ASSERT_NE(0u, ended_round); - - cache.Reset(); - ASSERT_EQ(0u, cache.RegistrationRound()); - ASSERT_OK(cache.Init({{0, 10}})); - const uint64_t open_round = cache.RegistrationRound(); - ASSERT_NE(ended_round, open_round); - - ASSERT_OK_AND_ASSIGN(std::optional first_added, - cache.AddRanges({{16, 10}}, ended_round)); - ASSERT_FALSE(first_added.has_value()); - AssertReadMiss({16, 10}, &cache); - ASSERT_EQ(10u, CounterOf(ReadAheadCacheMetrics::LATE_DROPPED_BYTES, &cache)); - - // The open round still accepts them. - ASSERT_OK_AND_ASSIGN(first_added, cache.AddRanges({{16, 10}}, open_round)); - ASSERT_TRUE(first_added.has_value()); - AssertReadEquals({16, 10}, "qrstuvwxyz", &cache); -} - -// Neither has a cache whose buffers were released: its registration round has ended. -TEST(TestReadAheadCache, TestAddRangesAfterReleaseBuffersIsNoop) { - CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, - /*hole_size_limit=*/0, /*pre_buffer_limit=*/1024); - std::string content = "abcdefghijklmnopqrstuvwxyz"; - std::shared_ptr cache_ptr = - CreateTestFileAndCache("data_file", content, config, {{0, 10}}); - ReadAheadCache& cache = *cache_ptr; - - const uint64_t round = cache.RegistrationRound(); - cache.ReleaseBuffers(); - ASSERT_EQ(0u, cache.RegistrationRound()); - - ASSERT_OK_AND_ASSIGN(std::optional first_added, cache.AddRanges({{16, 10}}, round)); - ASSERT_FALSE(first_added.has_value()); - AssertReadMiss({16, 10}, &cache); -} - // Warmup(offset) starts fetching from the given range rather than from the first one, which is // what a pass registering its ranges mid-read needs: the earlier ranges belong to the pass that // has already run. @@ -765,9 +655,9 @@ TEST(TestReadAheadCache, TestWarmupFromOffset) { ASSERT_EQ(io_hook->IOCount(), 1); } -// Test that Init() merges ranges separated by a small hole, so a read +// Test that AddRanges() merges ranges separated by a small hole, so a read // spanning the hole is served by the single coalesced entry. -TEST(TestReadAheadCache, TestInitCoalescesSmallHoles) { +TEST(TestReadAheadCache, TestCoalescesSmallHoles) { CacheConfig config = TestCacheConfig(/*range_size_limit=*/1024, /*hole_size_limit=*/2, /*pre_buffer_limit=*/1024); std::string content = "abcdefghijklmnopqrstuvwxyz"; @@ -895,7 +785,7 @@ TEST(TestReadAheadCache, TestBlockCacheDeclinedReadIsAMiss) { ASSERT_EQ(misses, 1u); } -// The blocks belong to the file rather than to a registration round: they +// The blocks belong to the file rather than to the registered ranges: they // survive Reset() and are only released by ReleaseBuffers(). TEST(TestReadAheadCache, TestBlockCacheSurvivesReset) { std::string content = "abcdefghijklmnopqrstuvwxyz"; diff --git a/src/paimon/format/parquet/parquet_file_batch_reader_test.cpp b/src/paimon/format/parquet/parquet_file_batch_reader_test.cpp index bed06bb76..3c1875ae6 100644 --- a/src/paimon/format/parquet/parquet_file_batch_reader_test.cpp +++ b/src/paimon/format/parquet/parquet_file_batch_reader_test.cpp @@ -1827,7 +1827,7 @@ TEST_F(ParquetFileBatchReaderTest, TestPreBufferRangeFeedsReadAheadCache) { for (const auto& range : pre_buffer_ranges) { byte_ranges.emplace_back(range.first, range.second); } - ASSERT_OK(cache->Init(std::move(byte_ranges))); + ASSERT_OK(cache->AddRanges(std::move(byte_ranges)).status()); // Dispatch the prefetch immediately so every pre-buffered range is covered // before the reads below consume them. cache->Warmup(); diff --git a/test/inte/read_inte_test.cpp b/test/inte/read_inte_test.cpp index c383dd921..284cd362f 100644 --- a/test/inte/read_inte_test.cpp +++ b/test/inte/read_inte_test.cpp @@ -575,22 +575,11 @@ TEST_P(ReadInteTest, TestReadAheadCacheMetrics) { ASSERT_OK_AND_ASSIGN(uint64_t io_bytes, read_metrics->GetCounter(ReadAheadCacheMetrics::IO_BYTES)); ASSERT_GT(io_bytes, 0u); - // Only a late-materialization payload pass registers ranges mid-read, which this scan does - // not do, so the counters must be exposed and stay at zero. - ASSERT_OK_AND_ASSIGN(uint64_t late_registered, - read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_REGISTERED)); - ASSERT_EQ(late_registered, 0u); - ASSERT_OK(read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_REGISTERED_BYTES)); - ASSERT_OK_AND_ASSIGN(uint64_t late_dropped, - read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_DROPPED)); - ASSERT_EQ(late_dropped, 0u); - ASSERT_OK(read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_DROPPED_BYTES)); } else { ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::READ_COUNT)); ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::READ_HITS)); ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::READ_MISSES)); ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::IO_COUNT)); - ASSERT_NOK(read_metrics->GetCounter(ReadAheadCacheMetrics::LATE_REGISTERED)); } } From f859cad9250a107ce81b7fe43c792ed0d404ff08 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Wed, 16 Sep 2026 14:10:52 +0800 Subject: [PATCH 08/15] opt --- .../common/metrics/atomic_counter_pair.h | 2 +- src/paimon/common/utils/file_block_cache.cpp | 57 ++- src/paimon/common/utils/file_block_cache.h | 8 +- src/paimon/common/utils/read_ahead_cache.cpp | 337 +++++++----------- src/paimon/common/utils/read_ahead_cache.h | 88 ++++- 5 files changed, 252 insertions(+), 240 deletions(-) diff --git a/src/paimon/common/metrics/atomic_counter_pair.h b/src/paimon/common/metrics/atomic_counter_pair.h index 5a8297ac3..27fd17298 100644 --- a/src/paimon/common/metrics/atomic_counter_pair.h +++ b/src/paimon/common/metrics/atomic_counter_pair.h @@ -35,7 +35,7 @@ struct AtomicCounterPair { std::atomic bytes{0}; /// Record one request covering `size` bytes. - void Add(uint64_t size) { + void Record(uint64_t size) { count.fetch_add(1, std::memory_order_relaxed); bytes.fetch_add(size, std::memory_order_relaxed); } diff --git a/src/paimon/common/utils/file_block_cache.cpp b/src/paimon/common/utils/file_block_cache.cpp index 829ffb23d..453a76eba 100644 --- a/src/paimon/common/utils/file_block_cache.cpp +++ b/src/paimon/common/utils/file_block_cache.cpp @@ -20,6 +20,8 @@ #include "paimon/common/utils/file_block_cache.h" #include +#include +#include #include "paimon/common/memory/bytes_utils.h" @@ -37,7 +39,7 @@ FileBlockCache::FileBlockCache(const std::shared_ptr& stream, uint6 FileBlockCache::~FileBlockCache() { // The fetches write into the block buffers, so they must not outlive the // stream they read from. - std::lock_guard lock(mutex_); + std::lock_guard lock(mutex_); for (auto& block : blocks_) { block.second->future.wait(); } @@ -50,11 +52,22 @@ bool FileBlockCache::Read(const ByteRange& range, char* dest) { const uint64_t index = IndexOf(range.offset); std::shared_ptr block; bool dispatch = false; + // Hit fast path under the read lock: concurrent reads of already-cached blocks + // proceed in parallel instead of serializing on an exclusive lock. { - // Publishing the promise-backed block under the lock before its fetch is - // dispatched is what makes concurrent readers of the same block wait for - // that one fetch instead of issuing their own. - std::lock_guard lock(mutex_); + std::shared_lock read_lock(mutex_); + auto it = blocks_.find(index); + if (it != blocks_.end()) { + block = it->second; + } + } + if (!block) { + // Miss: take the write lock and re-check, since another reader may have + // inserted the block while the read lock was dropped. Publishing the + // promise-backed block under the lock before its fetch is dispatched is what + // makes concurrent readers of the same block wait for that one fetch instead + // of issuing their own. + std::unique_lock write_lock(mutex_); auto it = blocks_.find(index); if (it != blocks_.end()) { block = it->second; @@ -95,32 +108,38 @@ bool FileBlockCache::Read(const ByteRange& range, char* dest) { return false; } std::memcpy(dest, block->buffer->data() + (range.offset - block->range.offset), range.length); - hits_.Add(range.length); + hit_metrics_.Record(range.length); return true; } void FileBlockCache::Release() { - std::lock_guard lock(mutex_); - // Blocks are never evicted, so waiting on blocks_ covers every dispatched - // fetch before the buffers they write into go away. - for (auto& block : blocks_) { + std::unordered_map> to_release; + { + std::lock_guard lock(mutex_); + // Swap the blocks out under the lock instead of waiting on them in place: an + // in-flight fetch may take arbitrarily long, and holding the lock while + // waiting would stall concurrent readers. + to_release.swap(blocks_); + cached_bytes_ = 0; + } + // Wait OUTSIDE the lock. Blocks are never evicted, so this covers every + // dispatched fetch before the buffers they write into go away. + for (auto& block : to_release) { block.second->future.wait(); } - blocks_.clear(); - cached_bytes_ = 0; } void FileBlockCache::ResetCounters() { - hits_.Reset(); - fetches_.Reset(); + hit_metrics_.Reset(); + fetch_metrics_.Reset(); } FileBlockCache::Counters FileBlockCache::GetCounters() const { Counters counters; - counters.hits = hits_.Count(); - counters.hit_bytes = hits_.Bytes(); - counters.fetches = fetches_.Count(); - counters.fetch_bytes = fetches_.Bytes(); + counters.hits = hit_metrics_.Count(); + counters.hit_bytes = hit_metrics_.Bytes(); + counters.fetches = fetch_metrics_.Count(); + counters.fetch_bytes = fetch_metrics_.Bytes(); return counters; } @@ -153,7 +172,7 @@ ByteRange FileBlockCache::RangeOf(uint64_t index) const { } void FileBlockCache::Fetch(const std::shared_ptr& block) { - fetches_.Add(block->range.length); + fetch_metrics_.Record(block->range.length); auto promise = block->promise; auto buffer = block->buffer; // The buffer and the promise are captured, so the async read keeps its diff --git a/src/paimon/common/utils/file_block_cache.h b/src/paimon/common/utils/file_block_cache.h index 3a2d5737a..10443b23f 100644 --- a/src/paimon/common/utils/file_block_cache.h +++ b/src/paimon/common/utils/file_block_cache.h @@ -22,7 +22,7 @@ #include #include #include -#include +#include #include #include "paimon/common/metrics/atomic_counter_pair.h" @@ -142,14 +142,14 @@ class PAIMON_EXPORT FileBlockCache { // Blocks are aligned, so keying them by index keeps them disjoint by // construction and needs no ordering. A plain mutex is enough: only the // reads that no prefetched range covers touch the map, and they are few. - mutable std::mutex mutex_; + mutable std::shared_mutex mutex_; std::unordered_map> blocks_; // Bytes held by blocks_, guarded by mutex_ and bounded by capacity_. uint64_t cached_bytes_ = 0; // The requests served out of a block, and the block fetches issued to the // underlying stream. - AtomicCounterPair hits_; - AtomicCounterPair fetches_; + AtomicCounterPair hit_metrics_; + AtomicCounterPair fetch_metrics_; }; } // namespace paimon diff --git a/src/paimon/common/utils/read_ahead_cache.cpp b/src/paimon/common/utils/read_ahead_cache.cpp index aeea7161a..f0695d810 100644 --- a/src/paimon/common/utils/read_ahead_cache.cpp +++ b/src/paimon/common/utils/read_ahead_cache.cpp @@ -23,7 +23,6 @@ #include "paimon/common/utils/read_ahead_cache.h" #include -#include #include #include #include @@ -41,23 +40,8 @@ namespace paimon { -struct RangeCacheEntry { - ByteRange range; - std::shared_ptr buffer; - std::shared_future future; // use shared_future in case of multiple get calls - - RangeCacheEntry() = default; - RangeCacheEntry(const ByteRange& range, std::shared_ptr buffer, - std::future future) - : range(range), buffer(std::move(buffer)), future(std::move(future).share()) {} - - friend bool operator<(const RangeCacheEntry& left, const RangeCacheEntry& right) { - return left.range.offset < right.range.offset; - } -}; - // Everything needed to dispatch the prefetch IO of an entry AFTER the entry -// has been published into entries_: the promise resolves the entry's future +// has been published into ranges_: the promise resolves the entry's future // and the buffer capture keeps the destination alive for the async IO. struct PendingFetch { ByteRange range; @@ -84,71 +68,7 @@ void CopyRangeFromEntries(const std::vector& covering, const By } // namespace -class ReadAheadCache::Impl { - public: - Impl(const std::shared_ptr& stream, const CacheConfig& config, uint64_t file_size, - const std::shared_ptr& memory_pool); - ~Impl(); - - Result> AddRanges(std::vector&& ranges); - Result Read(const ByteRange& range, char* dest); - void Reset(); - void ReleaseBuffers(); - void Warmup(); - void Warmup(uint64_t from_offset); - void CollectMetrics(std::shared_ptr* metrics) const; - - private: - /// Merge coalesced, validated `new_ranges` into the disjoint, offset-ordered pending_ranges_, - /// rebuilding is_cached_ alongside, and return the offset of the first newly registered range - /// (nullopt when nothing was added). The caller holds the write lock. - std::optional MergeRangesLocked(std::vector&& new_ranges); - - /// Dispatch the prefetch IOs for entries that have already been published - /// into entries_. - void DispatchFetches(const std::vector& fetches); - /// Find the entries fully covering the given range under the read lock. - /// Returns an empty vector on miss. Entries are copied (shared buffers) - /// so the caller may use them after releasing the lock. - std::vector FindCoveringEntries(const ByteRange& range); - - /// Mark, publish and fetch the pending ranges from the given offset forward, bounded by the - /// pre-buffer limit. - /// - /// Selecting the ranges, marking is_cached_ and publishing the promise-backed entries all - /// happen atomically under the write lock, before any IO is dispatched: AddRanges() rewrites - /// pending_ranges_ and is_cached_ from another thread, and a reader racing the prefetch must - /// observe is_cached_ set only once the covering entries are already visible, so it waits on - /// the in-flight entries instead of re-fetching the same bytes. - void PreBuffer(uint64_t offset); - - /// Clear the prefetch state, waiting for the fetches still writing into the - /// entry buffers. Leaves the block cache untouched. - void ReleasePrefetchBuffers(); - - std::shared_ptr stream_; - CacheConfig config_; - // Ordered by offset (so as to find a matching region by binary search) - std::vector entries_; - std::shared_ptr memory_pool_; - mutable std::shared_mutex rw_mutex_; - std::vector> is_cached_; - std::vector pending_ranges_; - // Caches the reads that no registered range covers, or null when the block - // cache is disabled. Owns its own locking and counters. - std::unique_ptr block_cache_; - // The Read() requests issued to the cache and how they were served, - // aggregated over all the streams sharing this cache. A read is counted - // either as a hit, a block cache hit or a miss. - AtomicCounterPair reads_; - AtomicCounterPair hits_; - AtomicCounterPair misses_; - // The prefetch IO actually issued to the underlying stream. The block cache - // counts its own fetches, which CollectMetrics() adds to these. - AtomicCounterPair ios_; -}; - -Result> ReadAheadCache::Impl::AddRanges(std::vector&& ranges) { +Result> ReadAheadCache::AddRanges(std::vector&& ranges) { // Both an up-front registration and a mid-read one are cut at the plain size limit. PAIMON_ASSIGN_OR_RAISE( std::vector new_ranges, @@ -159,46 +79,41 @@ Result> ReadAheadCache::Impl::AddRanges(std::vector(new_range.length, "range length")); } - // Locked: a registration rewrites the very pending_ranges_ and is_cached_ a concurrent Read() - // serves from, so it must not race the read finding its covering entries. + // Locked: a registration rewrites the very ranges_ a concurrent Read() serves from, so it + // must not race the read finding its covering entries. std::unique_lock lock(rw_mutex_); return MergeRangesLocked(std::move(new_ranges)); } -std::optional ReadAheadCache::Impl::MergeRangesLocked( - std::vector&& new_ranges) { +std::optional ReadAheadCache::MergeRangesLocked(std::vector&& new_ranges) { // Merge the two sorted, internally disjoint lists, keeping the result disjoint so that // FindCoveringEntries() may keep walking it: of a new range, only the parts no registered - // range covers are registered. is_cached_ is rebuilt alongside: it is indexed by - // pending_ranges_, and an already fetched range must not be fetched again. - std::vector merged; - std::vector merged_is_cached; - merged.reserve(pending_ranges_.size() + new_ranges.size()); - merged_is_cached.reserve(pending_ranges_.size() + new_ranges.size()); + // range covers are registered. Already registered entries are moved over as-is, so a range + // that was published keeps its buffer and future and is never fetched again. + std::vector merged; + merged.reserve(ranges_.size() + new_ranges.size()); std::optional first_added; size_t old_idx = 0; auto keep_registered = [&]() { - merged.push_back(pending_ranges_[old_idx]); - merged_is_cached.push_back(is_cached_[old_idx].load() ? 1 : 0); + merged.push_back(std::move(ranges_[old_idx])); ++old_idx; }; auto register_new = [&](uint64_t offset, uint64_t length) { if (!first_added.has_value()) { first_added = offset; } - merged.emplace_back(offset, length); - merged_is_cached.push_back(0); + merged.emplace_back(ByteRange(offset, length)); }; for (const ByteRange& candidate : new_ranges) { const uint64_t candidate_end = candidate.offset + candidate.length; // Start of the part of the candidate that is not registered yet. uint64_t cursor = candidate.offset; while (cursor < candidate_end) { - if (old_idx == pending_ranges_.size()) { + if (old_idx == ranges_.size()) { register_new(cursor, candidate_end - cursor); break; } - const ByteRange registered = pending_ranges_[old_idx]; + const ByteRange registered = ranges_[old_idx].range; const uint64_t registered_end = registered.offset + registered.length; if (registered_end <= cursor) { // Entirely before what is left of the candidate, so its place in the merged list @@ -220,60 +135,77 @@ std::optional ReadAheadCache::Impl::MergeRangesLocked( cursor = registered_end; } } - while (old_idx < pending_ranges_.size()) { + while (old_idx < ranges_.size()) { keep_registered(); } if (!first_added.has_value()) { return std::optional{}; } - pending_ranges_ = std::move(merged); - // std::atomic is neither copyable nor movable, so the inherited flags are carried over - // one by one instead of resizing in place. - is_cached_ = std::vector>(pending_ranges_.size()); - for (size_t i = 0; i < is_cached_.size(); ++i) { - is_cached_[i].store(merged_is_cached[i] != 0); - } + ranges_ = std::move(merged); return first_added; } -void ReadAheadCache::Impl::PreBuffer(uint64_t offset) { +bool ReadAheadCache::WindowHasUnpublishedLocked(uint64_t offset) const { + auto it = std::lower_bound(ranges_.begin(), ranges_.end(), offset, + [](const RangeCacheEntry& entry, uint64_t offset) { + return entry.range.offset + entry.range.length <= offset; + }); + if (it == ranges_.end() || it->range.offset > offset) { + return false; + } + size_t total_bytes = 0; + for (auto entry = it; entry != ranges_.end(); ++entry) { + total_bytes += entry->range.length; + if (total_bytes > config_.GetPreBufferLimit()) { + break; + } + if (!entry->Published()) { + return true; + } + } + return false; +} + +void ReadAheadCache::PreBuffer(uint64_t offset) { + // Fast path under the read lock: once the whole prefetch window is published there is nothing + // left to fetch, and returning here keeps the hot Read() path off the write lock, so concurrent + // readers' lookups are not serialized behind a publish that has nothing to publish. + { + std::shared_lock read_lock(rw_mutex_); + if (!WindowHasUnpublishedLocked(offset)) { + return; + } + } std::vector fetches; { std::unique_lock lock(rw_mutex_); - auto it = std::lower_bound(pending_ranges_.begin(), pending_ranges_.end(), offset, - [](const ByteRange& range, uint64_t offset) { - return range.offset + range.length <= offset; + auto it = std::lower_bound(ranges_.begin(), ranges_.end(), offset, + [](const RangeCacheEntry& entry, uint64_t offset) { + return entry.range.offset + entry.range.length <= offset; }); - if (it == pending_ranges_.end() || it->offset > offset) { + if (it == ranges_.end() || it->range.offset > offset) { return; } - std::vector new_entries; size_t total_bytes = 0; - for (size_t i = std::distance(pending_ranges_.begin(), it); i < pending_ranges_.size(); - ++i) { - total_bytes += pending_ranges_[i].length; + for (auto entry = it; entry != ranges_.end(); ++entry) { + total_bytes += entry->range.length; if (total_bytes > config_.GetPreBufferLimit()) { break; } - if (is_cached_[i].exchange(true)) { + if (entry->Published()) { continue; } - const ByteRange& range = pending_ranges_[i]; auto promise = std::make_shared>(); auto future = promise->get_future(); - auto buffer = AllocateBytesKeepingPoolAlive(range.length, memory_pool_); - fetches.push_back({range, buffer, promise}); - new_entries.emplace_back(range, std::move(buffer), std::move(future)); - } - if (!new_entries.empty()) { - // Entries are never evicted: the cache holds every published range until - // ReleaseBuffers()/Reset(), so an in-flight fetch always keeps its entry and thus - // its future reachable. - std::vector merged(entries_.size() + new_entries.size()); - std::merge(entries_.begin(), entries_.end(), new_entries.begin(), new_entries.end(), - merged.begin()); - entries_ = std::move(merged); + auto buffer = AllocateBytesKeepingPoolAlive(entry->range.length, memory_pool_); + fetches.push_back({entry->range, buffer, promise}); + // Published IN PLACE under the write lock, before the IO is dispatched below: a + // reader racing the prefetch then finds the entry and waits on its future instead of + // issuing a second fetch for the same bytes. Entries are never evicted, so an + // in-flight fetch always keeps its entry and thus its future reachable. + entry->buffer = std::move(buffer); + entry->future = std::move(future).share(); } } // Dispatched OUTSIDE the lock, so that a fetch completing inline does not deadlock against a @@ -281,8 +213,9 @@ void ReadAheadCache::Impl::PreBuffer(uint64_t offset) { DispatchFetches(fetches); } -ReadAheadCache::Impl::Impl(const std::shared_ptr& stream, const CacheConfig& config, - uint64_t file_size, const std::shared_ptr& memory_pool) +ReadAheadCache::ReadAheadCache(const std::shared_ptr& stream, + const CacheConfig& config, uint64_t file_size, + const std::shared_ptr& memory_pool) : stream_(stream), config_(config), memory_pool_(memory_pool) { // An unknown file size cannot be aligned to, and a zero limit or block size // means the block cache is turned off: leave it null in those cases. @@ -292,20 +225,22 @@ ReadAheadCache::Impl::Impl(const std::shared_ptr& stream, const Cac } } -ReadAheadCache::Impl::~Impl() { +ReadAheadCache::~ReadAheadCache() { std::unique_lock lock(rw_mutex_); - for (auto& entry : entries_) { - entry.future.wait(); + for (auto& entry : ranges_) { + if (entry.Published()) { + entry.future.wait(); + } } // The block cache waits for its own fetches when it is destroyed. } -void ReadAheadCache::Impl::Reset() { +void ReadAheadCache::Reset() { ReleasePrefetchBuffers(); - reads_.Reset(); - hits_.Reset(); - misses_.Reset(); - ios_.Reset(); + read_metrics_.Reset(); + hit_metrics_.Reset(); + miss_metrics_.Reset(); + io_metrics_.Reset(); if (block_cache_ != nullptr) { // Only the counters: the blocks cache the file, not the registered // ranges, and a reader resetting the cache reads the same file again. @@ -313,40 +248,47 @@ void ReadAheadCache::Impl::Reset() { } } -void ReadAheadCache::Impl::ReleaseBuffers() { +void ReadAheadCache::ReleaseBuffers() { ReleasePrefetchBuffers(); if (block_cache_ != nullptr) { block_cache_->Release(); } } -void ReadAheadCache::Impl::ReleasePrefetchBuffers() { - std::unique_lock lock(rw_mutex_); - // Entries are never evicted, so waiting on entries_ covers every - // dispatched fetch: no fetch is still writing into an entry buffer when the - // buffers go away. The buffers keep the memory pool alive themselves, for - // the callbacks that a stream destroys later than it resolves them. - for (auto& entry : entries_) { - entry.future.wait(); +void ReadAheadCache::ReleasePrefetchBuffers() { + std::vector to_release; + { + std::unique_lock lock(rw_mutex_); + // Swap the entries out under the lock instead of waiting on them in place: an in-flight + // fetch may take arbitrarily long, and holding the write lock while waiting would stall + // every concurrent reader. Once swapped, the entries are owned locally and no other thread + // can observe or mutate them. Entries are never evicted, so waiting on the published ones + // covers every dispatched fetch: no fetch is still writing into an entry buffer when the + // buffers go away. The buffers keep the memory pool alive themselves, for the callbacks + // that a stream destroys later than it resolves them. + to_release.swap(ranges_); + } + // Wait OUTSIDE the lock; the buffers are released when to_release destructs. + for (auto& entry : to_release) { + if (entry.Published()) { + entry.future.wait(); + } } - entries_.clear(); - is_cached_.clear(); - pending_ranges_.clear(); // The read/io counters are deliberately kept: a reader closed at EOF must // still be able to report them through CollectMetrics(). } -void ReadAheadCache::Impl::CollectMetrics(std::shared_ptr* metrics) const { +void ReadAheadCache::CollectMetrics(std::shared_ptr* metrics) const { if (metrics == nullptr || !*metrics) { return; } auto& m = *metrics; - m->SetCounter(ReadAheadCacheMetrics::READ_COUNT, reads_.Count()); - m->SetCounter(ReadAheadCacheMetrics::READ_BYTES, reads_.Bytes()); - m->SetCounter(ReadAheadCacheMetrics::READ_HITS, hits_.Count()); - m->SetCounter(ReadAheadCacheMetrics::READ_HIT_BYTES, hits_.Bytes()); - m->SetCounter(ReadAheadCacheMetrics::READ_MISSES, misses_.Count()); - m->SetCounter(ReadAheadCacheMetrics::READ_MISS_BYTES, misses_.Bytes()); + m->SetCounter(ReadAheadCacheMetrics::READ_COUNT, read_metrics_.Count()); + m->SetCounter(ReadAheadCacheMetrics::READ_BYTES, read_metrics_.Bytes()); + m->SetCounter(ReadAheadCacheMetrics::READ_HITS, hit_metrics_.Count()); + m->SetCounter(ReadAheadCacheMetrics::READ_HIT_BYTES, hit_metrics_.Bytes()); + m->SetCounter(ReadAheadCacheMetrics::READ_MISSES, miss_metrics_.Count()); + m->SetCounter(ReadAheadCacheMetrics::READ_MISS_BYTES, miss_metrics_.Bytes()); // The block cache keeps its own counters. Its fetches also go to the // underlying stream, so they are part of the io counters too. const FileBlockCache::Counters blocks = @@ -355,38 +297,40 @@ void ReadAheadCache::Impl::CollectMetrics(std::shared_ptr* metrics) con m->SetCounter(ReadAheadCacheMetrics::BLOCK_HIT_BYTES, blocks.hit_bytes); m->SetCounter(ReadAheadCacheMetrics::BLOCK_FETCHES, blocks.fetches); m->SetCounter(ReadAheadCacheMetrics::BLOCK_FETCH_BYTES, blocks.fetch_bytes); - m->SetCounter(ReadAheadCacheMetrics::IO_COUNT, ios_.Count() + blocks.fetches); - m->SetCounter(ReadAheadCacheMetrics::IO_BYTES, ios_.Bytes() + blocks.fetch_bytes); + m->SetCounter(ReadAheadCacheMetrics::IO_COUNT, io_metrics_.Count() + blocks.fetches); + m->SetCounter(ReadAheadCacheMetrics::IO_BYTES, io_metrics_.Bytes() + blocks.fetch_bytes); } -void ReadAheadCache::Impl::Warmup() { +void ReadAheadCache::Warmup() { // AddRanges() only registers the pending ranges; without this the first fetch // starts when the first Read() arrives, racing the reader's own miss fetch. uint64_t from_offset = 0; { std::shared_lock lock(rw_mutex_); - if (pending_ranges_.empty()) { + if (ranges_.empty()) { return; } - from_offset = pending_ranges_.front().offset; + from_offset = ranges_.front().range.offset; } PreBuffer(from_offset); } -void ReadAheadCache::Impl::Warmup(uint64_t from_offset) { +void ReadAheadCache::Warmup(uint64_t from_offset) { PreBuffer(from_offset); } -std::vector ReadAheadCache::Impl::FindCoveringEntries(const ByteRange& range) { +std::vector ReadAheadCache::FindCoveringEntries(const ByteRange& range) { std::vector covering; std::shared_lock lock(rw_mutex_); // Find the entry holding the start of the range: the first entry whose // end is beyond range.offset (entries are disjoint and sorted by offset). - auto it = std::lower_bound(entries_.begin(), entries_.end(), range.offset, + auto it = std::lower_bound(ranges_.begin(), ranges_.end(), range.offset, [](const RangeCacheEntry& e, uint64_t offset) { return e.range.offset + e.range.length <= offset; }); - if (it == entries_.end() || it->range.offset > range.offset) { + // An unpublished covering entry means its prefetch has not been dispatched + // yet, so the bytes are not there to serve: a miss. + if (it == ranges_.end() || it->range.offset > range.offset || !it->Published()) { return covering; } if (it->range.Contains(range)) { @@ -394,15 +338,15 @@ std::vector ReadAheadCache::Impl::FindCoveringEntries(const Byt return covering; } // The request spans several adjacent entries (a column chunk larger than - // one coalesced range): collect the contiguous run and check it covers - // the whole request. Entries are published before their fetch is - // dispatched, so a reader racing the prefetch waits for the in-flight - // fetch instead of issuing a second one for the same bytes. + // one coalesced range): collect the contiguous run of published entries and + // check it covers the whole request. Entries are published before their + // fetch is dispatched, so a reader racing the prefetch waits for the + // in-flight fetch instead of issuing a second one for the same bytes. uint64_t covered_end = it->range.offset + it->range.length; covering.push_back(*it); auto next = std::next(it); - while (covered_end < range.offset + range.length && next != entries_.end() && - next->range.offset == covered_end) { + while (covered_end < range.offset + range.length && next != ranges_.end() && + next->range.offset == covered_end && next->Published()) { covered_end = next->range.offset + next->range.length; covering.push_back(*next); ++next; @@ -413,11 +357,11 @@ std::vector ReadAheadCache::Impl::FindCoveringEntries(const Byt return covering; } -Result ReadAheadCache::Impl::Read(const ByteRange& range, char* dest) { +Result ReadAheadCache::Read(const ByteRange& range, char* dest) { if (range.length == 0) { return true; } - reads_.Add(range.length); + read_metrics_.Record(range.length); PreBuffer(range.offset); std::vector covering = FindCoveringEntries(range); if (covering.empty()) { @@ -428,7 +372,7 @@ Result ReadAheadCache::Impl::Read(const ByteRange& range, char* dest) { // The block cache counts its own hits, see CollectMetrics(). return true; } - misses_.Add(range.length); + miss_metrics_.Record(range.length); return false; } // Wait OUTSIDE the lock: the futures resolve when the prefetch stream's @@ -438,11 +382,11 @@ Result ReadAheadCache::Impl::Read(const ByteRange& range, char* dest) { } // The data copy runs OUTSIDE the lock for the same reason. CopyRangeFromEntries(covering, range, dest); - hits_.Add(range.length); + hit_metrics_.Record(range.length); return true; } -void ReadAheadCache::Impl::DispatchFetches(const std::vector& fetches) { +void ReadAheadCache::DispatchFetches(const std::vector& fetches) { for (const auto& fetch : fetches) { auto promise = fetch.promise; auto buffer = fetch.buffer; @@ -451,43 +395,8 @@ void ReadAheadCache::Impl::DispatchFetches(const std::vector& fetc stream_->ReadAsync( buffer->data(), read_size, read_offset, [promise, buffer](Status status) mutable { promise->set_value(status); }); - ios_.Add(fetch.range.length); + io_metrics_.Record(fetch.range.length); } } -ReadAheadCache::ReadAheadCache(const std::shared_ptr& stream, - const CacheConfig& config, uint64_t file_size, - const std::shared_ptr& memory_pool) - : impl_(std::make_unique(stream, config, file_size, memory_pool)) {} - -ReadAheadCache::~ReadAheadCache() = default; - -Result> ReadAheadCache::AddRanges(std::vector&& ranges) { - return impl_->AddRanges(std::move(ranges)); -} - -Result ReadAheadCache::Read(const ByteRange& range, char* dest) { - return impl_->Read(range, dest); -} - -void ReadAheadCache::Reset() { - return impl_->Reset(); -} - -void ReadAheadCache::ReleaseBuffers() { - return impl_->ReleaseBuffers(); -} - -void ReadAheadCache::Warmup() { - impl_->Warmup(); -} - -void ReadAheadCache::Warmup(uint64_t from_offset) { - impl_->Warmup(from_offset); -} - -void ReadAheadCache::CollectMetrics(std::shared_ptr* metrics) const { - impl_->CollectMetrics(metrics); -} - } // namespace paimon diff --git a/src/paimon/common/utils/read_ahead_cache.h b/src/paimon/common/utils/read_ahead_cache.h index 59bd9b9f5..e9834ac81 100644 --- a/src/paimon/common/utils/read_ahead_cache.h +++ b/src/paimon/common/utils/read_ahead_cache.h @@ -23,10 +23,14 @@ #pragma once #include +#include #include #include +#include +#include #include +#include "paimon/common/metrics/atomic_counter_pair.h" #include "paimon/fs/file_system.h" #include "paimon/memory/memory_pool.h" #include "paimon/result.h" @@ -37,6 +41,9 @@ namespace paimon { class Metrics; +class FileBlockCache; +class Bytes; +struct PendingFetch; /// Metric names for the read-ahead cache. class PAIMON_EXPORT ReadAheadCacheMetrics { @@ -99,6 +106,32 @@ struct PAIMON_EXPORT ByteRange { } }; +/// A registered range of the cache. `buffer` is null until the prefetch for the +/// range is published (dispatched); once published it holds the destination the +/// async IO writes into and `future` resolves when that IO completes. So +/// `Published()` distinguishes "registered, not fetched yet" from "fetched". +struct RangeCacheEntry { + ByteRange range; + std::shared_ptr buffer; + std::shared_future future; // use shared_future in case of multiple get calls + + RangeCacheEntry() = default; + /// Register a range without publishing it: no buffer, no fetch yet. + explicit RangeCacheEntry(const ByteRange& range) : range(range) {} + RangeCacheEntry(const ByteRange& range, std::shared_ptr buffer, + std::future future) + : range(range), buffer(std::move(buffer)), future(std::move(future).share()) {} + + /// True once the prefetch for this range has been published (buffer assigned). + bool Published() const { + return buffer != nullptr; + } + + friend bool operator<(const RangeCacheEntry& left, const RangeCacheEntry& right) { + return left.range.offset < right.range.offset; + } +}; + /// A read cache designed to hide IO latencies when reading. /// Prefetching strategy: When a range is read, the cache will prefetch up to /// `pre_buffer_range_count` additional adjacent ranges ahead of the requested offset. This helps @@ -201,8 +234,59 @@ class PAIMON_EXPORT ReadAheadCache { void ReleaseBuffers(); private: - class Impl; - std::unique_ptr impl_; + /// Merge coalesced, validated `new_ranges` into the disjoint, offset-ordered ranges_, moving + /// the already registered entries (and thus their published buffer/future) over as-is, and + /// return the offset of the first newly registered range (nullopt when nothing was added). + /// The caller holds the write lock. + std::optional MergeRangesLocked(std::vector&& new_ranges); + + /// Dispatch the prefetch IOs for entries that have already been published into ranges_. + void DispatchFetches(const std::vector& fetches); + + /// Find the published entries fully covering the given range under the read lock. + /// Returns an empty vector on miss. Entries are copied (shared buffers) + /// so the caller may use them after releasing the lock. + std::vector FindCoveringEntries(const ByteRange& range); + + /// True when the prefetch window starting at `offset` still holds an entry whose fetch has not + /// been published. The caller holds either lock; PreBuffer() uses it under the read lock to + /// skip the write lock entirely when there is nothing left to fetch. + bool WindowHasUnpublishedLocked(uint64_t offset) const; + + /// Publish and fetch the registered ranges from the given offset forward, bounded by the + /// pre-buffer limit. + /// + /// Selecting the ranges and publishing them (filling buffer/future in place in ranges_) all + /// happen under the write lock, before any IO is dispatched: AddRanges() rewrites ranges_ + /// from another thread, and a reader racing the prefetch must observe an entry published only + /// once it is already visible in ranges_, so it waits on the in-flight entry instead of + /// re-fetching the same bytes. + void PreBuffer(uint64_t offset); + + /// Clear the prefetch state, waiting for the fetches still writing into the + /// entry buffers. Leaves the block cache untouched. + void ReleasePrefetchBuffers(); + + std::shared_ptr stream_; + CacheConfig config_; + // Every registered range, ordered by offset and disjoint (so a matching + // region is found by binary search). An entry whose buffer is still null is + // registered but not fetched yet; publishing fills buffer/future in place. + std::vector ranges_; + std::shared_ptr memory_pool_; + mutable std::shared_mutex rw_mutex_; + // Caches the reads that no registered range covers, or null when the block + // cache is disabled. Owns its own locking and counters. + std::unique_ptr block_cache_; + // The Read() requests issued to the cache and how they were served, + // aggregated over all the streams sharing this cache. A read is counted + // either as a hit, a block cache hit or a miss. + AtomicCounterPair read_metrics_; + AtomicCounterPair hit_metrics_; + AtomicCounterPair miss_metrics_; + // The prefetch IO actually issued to the underlying stream. The block cache + // counts its own fetches, which CollectMetrics() adds to these. + AtomicCounterPair io_metrics_; }; } // namespace paimon From 0aff1c76b9ae532aa33c4462956fc6e43c1c6be4 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Wed, 16 Sep 2026 14:14:27 +0800 Subject: [PATCH 09/15] fix --- src/paimon/CMakeLists.txt | 1 - .../operation/commit/retry_waiter_test.cpp | 50 ------------------- 2 files changed, 51 deletions(-) delete mode 100644 src/paimon/core/operation/commit/retry_waiter_test.cpp diff --git a/src/paimon/CMakeLists.txt b/src/paimon/CMakeLists.txt index 803bdb2b8..a9bb452c0 100644 --- a/src/paimon/CMakeLists.txt +++ b/src/paimon/CMakeLists.txt @@ -900,7 +900,6 @@ if(PAIMON_BUILD_TESTS) core/operation/commit/row_tracking_commit_utils_test.cpp core/operation/commit/realtime_commit_properties_test.cpp core/operation/commit/sequence_snapshot_properties_test.cpp - core/operation/commit/retry_waiter_test.cpp core/operation/key_value_file_store_write_test.cpp core/operation/internal_read_context_test.cpp core/operation/abstract_split_read_test.cpp diff --git a/src/paimon/core/operation/commit/retry_waiter_test.cpp b/src/paimon/core/operation/commit/retry_waiter_test.cpp deleted file mode 100644 index 2d8ea105c..000000000 --- a/src/paimon/core/operation/commit/retry_waiter_test.cpp +++ /dev/null @@ -1,50 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -#include "paimon/core/operation/commit/retry_waiter.h" - -#include - -#include "gtest/gtest.h" - -namespace paimon::test { - -TEST(RetryWaiterTest, TestRetryWaitWithZeroBoundsReturnsQuickly) { - RetryWaiter waiter(/*min_retry_wait_ms=*/0, /*max_retry_wait_ms=*/0); - - auto begin = std::chrono::steady_clock::now(); - waiter.RetryWait(/*retry_count=*/3); - auto elapsed = std::chrono::duration_cast( - std::chrono::steady_clock::now() - begin); - - ASSERT_LT(elapsed.count(), 20); -} - -TEST(RetryWaiterTest, TestRetryWaitWithLargeRetryCountIsClamped) { - RetryWaiter waiter(/*min_retry_wait_ms=*/10, /*max_retry_wait_ms=*/10); - - auto begin = std::chrono::steady_clock::now(); - waiter.RetryWait(/*retry_count=*/1024); - auto elapsed = std::chrono::duration_cast( - std::chrono::steady_clock::now() - begin); - - ASSERT_GE(elapsed.count(), 8); -} - -} // namespace paimon::test From 1eae4ea8c6ebb1de0caa00723ff8610ae0e532cd Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Wed, 16 Sep 2026 14:50:07 +0800 Subject: [PATCH 10/15] fix --- .../reader/prefetch_file_batch_reader.h | 15 ++--- .../late_materializing_file_batch_reader.cpp | 4 +- .../late_materializing_file_batch_reader.h | 14 ++--- ...e_materializing_file_batch_reader_test.cpp | 19 +++--- .../prefetch_file_batch_reader_impl.cpp | 12 ++-- .../reader/prefetch_file_batch_reader_impl.h | 6 +- src/paimon/common/utils/read_ahead_cache.h | 60 ++++++++----------- 7 files changed, 63 insertions(+), 67 deletions(-) diff --git a/include/paimon/reader/prefetch_file_batch_reader.h b/include/paimon/reader/prefetch_file_batch_reader.h index d65a48e28..1969194bf 100644 --- a/include/paimon/reader/prefetch_file_batch_reader.h +++ b/include/paimon/reader/prefetch_file_batch_reader.h @@ -137,18 +137,19 @@ class PAIMON_EXPORT PrefetchFileBatchReader : public FileBatchReader { return std::vector>{}; } - /// Sink a reader reports byte ranges to when they only become known after reading has + /// Callback a reader reports byte ranges through when they only become known after reading has /// started, so the prefetch layer can register them with its shared read-ahead cache. /// /// PreBufferRange() covers what is known up front; a reader whose ranges depend on data it /// has already read - the late-materialization payload pass only knows which pages hold the - /// matched rows once the probe pass has run - reports them here instead. - using PreBufferSink = std::function>&&)>; + /// matched rows once the probe pass has run - reports them through this callback instead. + using PreBufferRangeCallback = + std::function>&&)>; - /// Installs the sink above, or clears it when `sink` is empty. By default a reader has no - /// late byte ranges to report and ignores the sink. - /// @param sink The sink to report late byte ranges to. - virtual void SetPreBufferSink(PreBufferSink sink) {} + /// Installs the callback above, or clears it when `callback` is empty. By default a reader has + /// no late byte ranges to report and ignores the callback. + /// @param callback The callback to report late byte ranges through. + virtual void SetPreBufferRangeCallback(PreBufferRangeCallback callback) {} }; } // namespace paimon diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader.cpp b/src/paimon/common/reader/late_materializing_file_batch_reader.cpp index e0ef74ac1..10bdaea18 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader.cpp +++ b/src/paimon/common/reader/late_materializing_file_batch_reader.cpp @@ -268,7 +268,7 @@ Status LateMaterializingFileBatchReader::ReportPayloadPreBufferRanges() { // reader's target row groups down to the pages holding the matched rows. Reporting them // here lets the shared read-ahead cache fetch them while this pass is still assembling // its first batch, instead of every payload read waiting for its own IO. - if (!pre_buffer_sink_ || prefetch_inner_ == nullptr) { + if (!pre_buffer_range_callback_ || prefetch_inner_ == nullptr) { return Status::OK(); } // The error is propagated rather than swallowed: the ranges come from the file metadata, so a @@ -278,7 +278,7 @@ Status LateMaterializingFileBatchReader::ReportPayloadPreBufferRanges() { if (ranges.empty()) { return Status::OK(); } - pre_buffer_sink_(std::move(ranges)); + pre_buffer_range_callback_(std::move(ranges)); return Status::OK(); } diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader.h b/src/paimon/common/reader/late_materializing_file_batch_reader.h index b9f826df0..db46fd62d 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader.h +++ b/src/paimon/common/reader/late_materializing_file_batch_reader.h @@ -95,7 +95,7 @@ class LateMaterializingFileBatchReader : public PrefetchFileBatchReader { Result>> PreBufferRange() override { // Deliberately only the probe ranges: this is asked once before any read, when the payload // pass still has no idea which pages hold the matched rows. The payload ranges are - // reported through pre_buffer_sink_ once the probe pass has run, see + // reported through pre_buffer_range_callback_ once the probe pass has run, see // ReportPayloadPreBufferRanges(). if (prefetch_inner_ == nullptr) { return std::vector>{}; @@ -103,10 +103,10 @@ class LateMaterializingFileBatchReader : public PrefetchFileBatchReader { return prefetch_inner_->PreBufferRange(); } - void SetPreBufferSink(PreBufferSink sink) override { + void SetPreBufferRangeCallback(PreBufferRangeCallback callback) override { // Not forwarded to prefetch_inner_: a format reader has no late ranges of its own, and // forwarding would report the same ranges twice. - pre_buffer_sink_ = std::move(sink); + pre_buffer_range_callback_ = std::move(callback); } private: @@ -149,9 +149,9 @@ class LateMaterializingFileBatchReader : public PrefetchFileBatchReader { const std::shared_ptr& predicate, const std::optional& selection); - /// Report the byte ranges the payload pass is about to read to pre_buffer_sink_, so that the - /// prefetch layer can fetch them ahead of the pass. A no-op without a sink or without a - /// prefetch-capable inner reader. + /// Report the byte ranges the payload pass is about to read to pre_buffer_range_callback_, so + /// that the prefetch layer can fetch them ahead of the pass. A no-op without a callback or + /// without a prefetch-capable inner reader. Status ReportPayloadPreBufferRanges(); /// Returns the inner reader's prefetch interface, or an error when the format reader does not @@ -178,7 +178,7 @@ class LateMaterializingFileBatchReader : public PrefetchFileBatchReader { std::shared_ptr arrow_pool_; /// Where the payload byte ranges are reported. Part of how this reader is wired into the /// prefetch layer rather than of a read generation, so Reset() leaves it alone. - PreBufferSink pre_buffer_sink_; + PreBufferRangeCallback pre_buffer_range_callback_; LatMatState state_ = kInit; std::shared_ptr full_schema_; // projection holding only the predicate fields; nullptr when probing is not applicable diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp index acb4a23b6..3501e6190 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp +++ b/src/paimon/common/reader/late_materializing_file_batch_reader_test.cpp @@ -766,9 +766,9 @@ TEST_F(LateMaterializingFileBatchReaderTest, FailsOnPredicateTypeMismatch) { } // The payload byte ranges are only known once the probe pass has run, so they are reported through -// the sink rather than through PreBufferRange(), and early enough to still be prefetched: before -// the first payload batch leaves the reader. -TEST_F(LateMaterializingFileBatchReaderTest, PayloadPreBufferRangesReportedToSink) { +// the callback rather than through PreBufferRange(), and early enough to still be prefetched: +// before the first payload batch leaves the reader. +TEST_F(LateMaterializingFileBatchReaderTest, PayloadPreBufferRangesReportedToCallback) { auto data = BuildData({0, 1, 2, 3, 4, 5, 6, 7, 8, 9}); auto mock = std::make_unique(data, full_type_, /*batch_size=*/3); // Aliased because a type carrying a comma cannot be passed to the ASSERT_OK_AND_ASSIGN macro. @@ -781,7 +781,7 @@ TEST_F(LateMaterializingFileBatchReaderTest, PayloadPreBufferRangesReportedToSin int32_t batches = 0; std::vector reported; std::vector batches_when_reported; - reader->SetPreBufferSink([&](ByteRanges&& ranges) { + reader->SetPreBufferRangeCallback([&](ByteRanges&& ranges) { reported.push_back(std::move(ranges)); batches_when_reported.push_back(batches); }); @@ -811,14 +811,15 @@ TEST_F(LateMaterializingFileBatchReaderTest, PayloadPreBufferRangesReportedToSin } // Without a predicate there is no payload pass, so there is no late range to report. -TEST_F(LateMaterializingFileBatchReaderTest, NoSinkReportWithoutLateMaterialization) { +TEST_F(LateMaterializingFileBatchReaderTest, NoCallbackReportWithoutLateMaterialization) { auto data = BuildData({0, 1, 2, 3, 4}); auto mock = std::make_unique(data, full_type_, /*batch_size=*/2); mock->SetPreBufferRangesPerSchema({{{0, 1024}}, {{100000, 2048}}}); ASSERT_OK_AND_ASSIGN(auto reader, LateMaterializingFileBatchReader::Create( std::move(mock), GetArrowPool(pool_))); int32_t calls = 0; - reader->SetPreBufferSink([&calls](std::vector>&&) { calls++; }); + reader->SetPreBufferRangeCallback( + [&calls](std::vector>&&) { calls++; }); ASSERT_OK(SetReadSchema(reader.get(), arrow::schema(full_fields_), /*predicate=*/nullptr, std::nullopt)); @@ -835,7 +836,8 @@ TEST_F(LateMaterializingFileBatchReaderTest, EmptyMatchDoesNotReportPayloadRange ASSERT_OK_AND_ASSIGN(auto reader, LateMaterializingFileBatchReader::Create( std::move(mock), GetArrowPool(pool_))); int32_t calls = 0; - reader->SetPreBufferSink([&calls](std::vector>&&) { calls++; }); + reader->SetPreBufferRangeCallback( + [&calls](std::vector>&&) { calls++; }); auto predicate = PredicateBuilder::GreaterThan(/*field_index=*/0, "k", FieldType::BIGINT, Literal(100l)); ASSERT_OK(SetReadSchema(reader.get(), arrow::schema(full_fields_), predicate, std::nullopt)); @@ -854,7 +856,8 @@ TEST_F(LateMaterializingFileBatchReaderTest, PayloadPreBufferRangeErrorFailsRead ASSERT_OK_AND_ASSIGN(auto reader, LateMaterializingFileBatchReader::Create( std::move(mock), GetArrowPool(pool_))); int32_t calls = 0; - reader->SetPreBufferSink([&calls](std::vector>&&) { calls++; }); + reader->SetPreBufferRangeCallback( + [&calls](std::vector>&&) { calls++; }); auto predicate = PredicateBuilder::GreaterOrEqual(/*field_index=*/0, "k", FieldType::BIGINT, Literal(2l)); ASSERT_OK(SetReadSchema(reader.get(), arrow::schema(full_fields_), predicate, std::nullopt)); diff --git a/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp b/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp index a506dffa7..4e5e540e4 100644 --- a/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp +++ b/src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp @@ -327,7 +327,7 @@ Result> PrefetchFileBatchReaderImpl // late-materialization payload pass) report them here instead of through PreBufferRange(), // which is asked once per read-range generation before any read. Capturing the impl raw is // safe: it owns readers_, and CleanUp() joins the background thread before it goes away. - sub_reader->SetPreBufferSink( + sub_reader->SetPreBufferRangeCallback( [impl = reader.get()](std::vector>&& ranges) { impl->RegisterLatePreBufferRanges(std::move(ranges)); }); @@ -366,7 +366,7 @@ PrefetchFileBatchReaderImpl::PrefetchFileBatchReaderImpl( PrefetchFileBatchReaderImpl::~PrefetchFileBatchReaderImpl() { (void)CleanUp(); - ClearPreBufferSinks(); + ClearPreBufferRangeCallbacks(); } Status PrefetchFileBatchReaderImpl::SetReadSchema( @@ -834,11 +834,11 @@ void PrefetchFileBatchReaderImpl::RegisterLatePreBufferRanges( cache_->Warmup(first_added.value().value()); } -void PrefetchFileBatchReaderImpl::ClearPreBufferSinks() { - // Belt and braces: the sinks capture this, and by now CleanUp() has joined the background +void PrefetchFileBatchReaderImpl::ClearPreBufferRangeCallbacks() { + // Belt and braces: the callbacks capture this, and by now CleanUp() has joined the background // thread, so no sub-reader can report a range any more. for (const auto& sub_reader : readers_) { - sub_reader->SetPreBufferSink(nullptr); + sub_reader->SetPreBufferRangeCallback(nullptr); } } @@ -1082,7 +1082,7 @@ void PrefetchFileBatchReaderImpl::Close() { for (const auto& reader : readers_) { reader->Close(); } - ClearPreBufferSinks(); + ClearPreBufferRangeCallbacks(); } } // namespace paimon diff --git a/src/paimon/common/reader/prefetch_file_batch_reader_impl.h b/src/paimon/common/reader/prefetch_file_batch_reader_impl.h index a0f170827..164cf6c6b 100644 --- a/src/paimon/common/reader/prefetch_file_batch_reader_impl.h +++ b/src/paimon/common/reader/prefetch_file_batch_reader_impl.h @@ -151,9 +151,9 @@ class PrefetchFileBatchReaderImpl : public PrefetchFileBatchReader { /// as in WarmCacheOnce(). void RegisterLatePreBufferRanges(std::vector>&& read_ranges); - /// Detaches the sinks installed on the sub-readers, so that a sub-reader outliving this reader - /// cannot call back into it. Called once the background thread has been joined. - void ClearPreBufferSinks(); + /// Detaches the callbacks installed on the sub-readers, so that a sub-reader outliving this + /// reader cannot call back into it. Called once the background thread has been joined. + void ClearPreBufferRangeCallbacks(); void SetReadStatus(const Status& status); Status GetReadStatus() const; Result IsEofRange(const std::pair& read_range) const; diff --git a/src/paimon/common/utils/read_ahead_cache.h b/src/paimon/common/utils/read_ahead_cache.h index e9834ac81..4eec73836 100644 --- a/src/paimon/common/utils/read_ahead_cache.h +++ b/src/paimon/common/utils/read_ahead_cache.h @@ -132,11 +132,10 @@ struct RangeCacheEntry { } }; -/// A read cache designed to hide IO latencies when reading. -/// Prefetching strategy: When a range is read, the cache will prefetch up to -/// `pre_buffer_range_count` additional adjacent ranges ahead of the requested offset. This helps -/// hide I/O latency for sequential access. Example: If you read range [0, 100), and -/// pre_buffer_range_count=2, the next two configured ranges will also be prefetched. +/// A read cache designed to hide IO latency when reading. +/// Prefetching strategy: a Read() (or Warmup()) at an offset publishes and fetches the registered +/// ranges from that offset forward until their cumulative size exceeds `pre_buffer_limit`, so a +/// sequential read runs ahead of the caller instead of fetching one range at a time. /// /// The cache never evicts: every published range stays cached until /// ReleaseBuffers() or Reset(). It is meant to hold the prefetched ranges of @@ -163,19 +162,15 @@ class PAIMON_EXPORT ReadAheadCache { /// Register byte ranges to prefetch, merging them into the ranges already registered. /// - /// This is the cache's only registration entry point, and it may be called repeatedly and - /// concurrently with Read(): the ranges known up front - a read plan's whole extent - are - /// registered before any Read(), and the ranges that only become known WHILE reading - a - /// late-materialization payload pass learns which pages it needs only after the probe pass has - /// evaluated the predicate - are registered as they are discovered. Read() finds its covering - /// entries by walking a disjoint, offset-ordered list, so of a new range only the part no - /// registered range covers is added; the overlapping part is dropped, as those bytes are - /// already being fetched. A registration is cut into ranges of at most `range_size_limit` - /// bytes, one prefetch IO each, so a large pass is fetched concurrently instead of in one long - /// request. - /// - /// Deciding WHICH ranges to register - and in particular not reporting the ranges of a read - /// that has already ended - is the caller's concern; the cache registers whatever it is given. + /// The cache's only registration entry point; safe to call repeatedly and concurrently with + /// Read(). Ranges known up front are registered before any Read(); ranges that only become + /// known while reading (a late-materialization payload pass learns its pages after the probe + /// pass) are registered as they are discovered. The list stays disjoint and offset-ordered, so + /// only the part of a new range that no registered range covers is added - the overlap is + /// dropped, as those bytes are already being fetched - and each registration is cut into ranges + /// of at most `range_size_limit` bytes (one prefetch IO each) so a large pass is fetched + /// concurrently. Which ranges to register, and not reporting those of an ended read, is the + /// caller's concern. /// /// @param ranges The byte ranges to register. /// @return The offset of the first newly registered range, or nullopt when nothing was added. @@ -195,9 +190,9 @@ class PAIMON_EXPORT ReadAheadCache { /// filled; false on cache miss (`dest` is left untouched). Result Read(const ByteRange& range, char* dest); - /// Start fetching the first batch of pending ranges immediately. - /// AddRanges() only registers the ranges; without Warmup() the first fetch starts - /// when the first Read() arrives, racing the caller's own miss fetch. + /// Start fetching from the first registered range forward immediately. AddRanges() only + /// registers the ranges; without Warmup() the first fetch starts when the first Read() arrives, + /// racing the caller's own miss fetch. void Warmup(); /// Start fetching the registered ranges from `from_offset` forward, bounded by the @@ -205,26 +200,23 @@ class PAIMON_EXPORT ReadAheadCache { /// @param from_offset The offset to start fetching from, typically one AddRanges() returned. void Warmup(uint64_t from_offset); - /// Collect the counters of the Read() calls, of the block cache, of the IOs - /// and of the late registrations into the given metrics as counters named - /// after `ReadAheadCacheMetrics`. Only reads issued through Read() are - /// counted as hits, block hits or misses; the fetches the cache dispatches - /// are counted in the io counters instead. + /// Collect the counters of the Read() calls, of the block cache and of the IOs into the given + /// metrics as counters named after `ReadAheadCacheMetrics`. Only reads issued through Read() + /// are counted as hits, block hits or misses; the fetches the cache dispatches are counted in + /// the io counters instead. /// @param metrics The metrics to write the counters into. A null /// pointer or a null shared pointer is a no-op. void CollectMetrics(std::shared_ptr* metrics) const; - /// Reset the cache to its initial state, clearing all cached data and configuration. - /// - /// This method waits for all ongoing asynchronous read operations to complete, - /// clears all cached entries, and resets the internal state so a fresh set of ranges can be - /// registered again with AddRanges(). + /// Drop the prefetched entries and zero all counters, so a fresh set of ranges can be + /// registered with AddRanges(). Waits for the in-flight fetches before their buffers go away. /// - /// The block cache is kept: it caches the file rather than the registered - /// ranges, and a reader reusing the cache reads the same file again. + /// The block cache is kept - it caches the file rather than the registered ranges, and a reader + /// reusing the cache reads the same file again - but its counters are zeroed too. Unlike + /// ReleaseBuffers(), the counters do not survive. void Reset(); - /// Release all cached buffers and pending ranges while keeping the hit/miss + /// Release all cached buffers and registered ranges while keeping the hit/miss /// counters intact. /// /// Unlike Reset(), the counters recorded by Read() remain readable through From 892a14d171f7be98dce4608163fe54530392d4a6 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Wed, 16 Sep 2026 15:12:43 +0800 Subject: [PATCH 11/15] fix --- src/paimon/common/utils/read_ahead_cache.cpp | 6 +-- .../common/utils/read_ahead_cache_test.cpp | 49 +++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/src/paimon/common/utils/read_ahead_cache.cpp b/src/paimon/common/utils/read_ahead_cache.cpp index f0695d810..68dc18034 100644 --- a/src/paimon/common/utils/read_ahead_cache.cpp +++ b/src/paimon/common/utils/read_ahead_cache.cpp @@ -138,9 +138,9 @@ std::optional ReadAheadCache::MergeRangesLocked(std::vector while (old_idx < ranges_.size()) { keep_registered(); } - if (!first_added.has_value()) { - return std::optional{}; - } + // Always move the merged list back: keep_registered() has already moved the existing entries' + // buffer/future out of ranges_, so returning early without this would drop the published state + // (and the future to wait on) even when nothing new was registered. ranges_ = std::move(merged); return first_added; } diff --git a/src/paimon/common/utils/read_ahead_cache_test.cpp b/src/paimon/common/utils/read_ahead_cache_test.cpp index 382a7bb3b..c9f7454a1 100644 --- a/src/paimon/common/utils/read_ahead_cache_test.cpp +++ b/src/paimon/common/utils/read_ahead_cache_test.cpp @@ -631,6 +631,55 @@ TEST(TestReadAheadCache, TestAddRangesKeepsCachedRanges) { ASSERT_EQ(io_count_before, io_count_of()); } +// Re-registering a range the cache already covers adds nothing, and must leave the published entry +// intact: the earlier fetch stays valid, so a later read of the same range is still a hit and +// issues no further IO. A no-op registration that dropped the buffer would turn this read into a +// miss (and lose the future to wait on an in-flight fetch). +TEST(TestReadAheadCache, TestReRegisteringCoveredRangeKeepsCache) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/10); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 10}}); + ReadAheadCache& cache = *cache_ptr; + + cache.Warmup(); + AssertReadEquals({0, 10}, "abcdefghij", &cache); + auto io_count_of = [&cache]() { return CounterOf(ReadAheadCacheMetrics::IO_COUNT, &cache); }; + const uint64_t io_count_before = io_count_of(); + ASSERT_EQ(1u, io_count_before); + + // [2,6) is already covered by [0,10), so nothing new is registered. + ASSERT_OK_AND_ASSIGN(std::optional first_added, cache.AddRanges({{2, 6}})); + ASSERT_FALSE(first_added.has_value()); + + // The published entry survived the no-op registration: still a hit, no new IO. + AssertReadEquals({0, 10}, "abcdefghij", &cache); + ASSERT_EQ(io_count_before, io_count_of()); +} + +// An empty registration is likewise a no-op that keeps the already published entries. +TEST(TestReadAheadCache, TestEmptyAddRangesKeepsCache) { + CacheConfig config = TestCacheConfig(/*range_size_limit=*/10, + /*hole_size_limit=*/0, /*pre_buffer_limit=*/10); + std::string content = "abcdefghijklmnopqrstuvwxyz"; + std::shared_ptr cache_ptr = + CreateTestFileAndCache("data_file", content, config, {{0, 10}}); + ReadAheadCache& cache = *cache_ptr; + + cache.Warmup(); + AssertReadEquals({0, 10}, "abcdefghij", &cache); + auto io_count_of = [&cache]() { return CounterOf(ReadAheadCacheMetrics::IO_COUNT, &cache); }; + const uint64_t io_count_before = io_count_of(); + ASSERT_EQ(1u, io_count_before); + + ASSERT_OK_AND_ASSIGN(std::optional first_added, cache.AddRanges({})); + ASSERT_FALSE(first_added.has_value()); + + AssertReadEquals({0, 10}, "abcdefghij", &cache); + ASSERT_EQ(io_count_before, io_count_of()); +} + // Warmup(offset) starts fetching from the given range rather than from the first one, which is // what a pass registering its ranges mid-read needs: the earlier ranges belong to the pass that // has already run. From 443a73479036ec041f8dee0b0395a29cfe3ce01a Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Wed, 16 Sep 2026 16:58:50 +0800 Subject: [PATCH 12/15] fix --- src/paimon/format/parquet/page_filtered_row_group_reader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp index 78f756fa0..dd8297e8f 100644 --- a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp +++ b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp @@ -53,7 +53,7 @@ namespace { /// factor of it is bounded by bytes that physically exist. A leaf decompressing within the factor /// is reserved in full; a more compressible one is capped and left to the builder's amortized /// doubling, which is cheap against the I/O and decode of a read that large. -constexpr int64_t kMaxReservationDecompressionFactor = 8; +constexpr int64_t kMaxReservationDecompressionFactor = 20; struct DataPageLayout { int64_t column_chunk_offset; From 9e4542027109c5663f9041c5d14c5ad1b37f6f36 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Wed, 16 Sep 2026 17:39:20 +0800 Subject: [PATCH 13/15] debug --- include/paimon/utils/prefetch_cache_config.h | 2 +- src/paimon/format/parquet/page_filtered_row_group_reader.cpp | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/include/paimon/utils/prefetch_cache_config.h b/include/paimon/utils/prefetch_cache_config.h index d767fd929..c0296b015 100644 --- a/include/paimon/utils/prefetch_cache_config.h +++ b/include/paimon/utils/prefetch_cache_config.h @@ -110,7 +110,7 @@ class PAIMON_EXPORT CacheConfig { // costs a round trip whatever its size. The limit is therefore well above // the page-sized gaps a filtered read leaves between the pages it keeps, // which would otherwise each cost a request of their own. - uint64_t range_size_limit_ = 8 * 1024 * 1024; + uint64_t range_size_limit_ = 4 * 1024 * 1024; uint64_t hole_size_limit_ = 512 * 1024; uint64_t pre_buffer_limit_ = 256 * 1024 * 1024; // Blocks are aligned to the END of the file, so a block never reaches past diff --git a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp index dd8297e8f..29ab4134a 100644 --- a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp +++ b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -402,6 +403,10 @@ Result> PageFilteredRowGroupReader::ReadFil static_cast(kMaxReservationDecompressionFactor))}); } + std::cerr << "[reserve_value_bytes] col_idx=" << col_idx + << " reserve_values=" << reserve_values + << " reserve_value_bytes=" << reserve_value_bytes << std::endl; + PAIMON_RETURN_NOT_OK(ExecuteSkipReadPattern(col_idx, effective_ranges, effective_total, reserve_values, reserve_value_bytes, column_reader.get())); From 2c8fa0421f32226ef96306b17219947cb3c668bc Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Thu, 17 Sep 2026 10:58:31 +0800 Subject: [PATCH 14/15] debug --- .../page_filtered_row_group_reader.cpp | 37 ++++++++++++++----- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp index 29ab4134a..62a95bd07 100644 --- a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp +++ b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp @@ -382,6 +382,11 @@ Result> PageFilteredRowGroupReader::ReadFil const int64_t chunk_compressed_bytes = column_chunk ? column_chunk->total_compressed_size() : 0; const int64_t chunk_values = column_chunk ? column_chunk->num_values() : 0; + // Named intermediates of the estimate below, declared here so the debug print after the + // block can show the whole calculation; they stay 0 when the guard does not hold. + double avg_bytes_per_value = 0.0; + int64_t estimate_by_avg = 0; + int64_t cap_by_decompression = 0; if (chunk_bytes > 0 && chunk_values > 0 && reserve_values > 0) { // A heuristic, not a bound. total_uncompressed_size is uncompressed but still // ENCODED: a dictionary page stores each value once and its data pages only @@ -395,17 +400,31 @@ Result> PageFilteredRowGroupReader::ReadFil // forgeable uncompressed one (see kMaxReservationDecompressionFactor). Fixed-width // leaves ignore the byte count entirely. const double avg = static_cast(chunk_bytes) / static_cast(chunk_values); - reserve_value_bytes = std::min( - {SaturatingDoubleToInteger(avg * static_cast(reserve_values)), - chunk_bytes, - SaturatingDoubleToInteger( - static_cast(chunk_compressed_bytes) * - static_cast(kMaxReservationDecompressionFactor))}); + avg_bytes_per_value = avg; + // Candidate 1: average width over the whole chunk times the rows actually appended. + estimate_by_avg = SaturatingDoubleToInteger( + avg * static_cast(reserve_values)); + // Candidate 3: the file-bounded compressed size times the decompression factor. + cap_by_decompression = SaturatingDoubleToInteger( + static_cast(chunk_compressed_bytes) * + static_cast(kMaxReservationDecompressionFactor)); + // Candidate 2 is chunk_bytes itself (the uncompressed size), passed to min directly. + reserve_value_bytes = std::min({estimate_by_avg, chunk_bytes, cap_by_decompression}); } - std::cerr << "[reserve_value_bytes] col_idx=" << col_idx - << " reserve_values=" << reserve_values - << " reserve_value_bytes=" << reserve_value_bytes << std::endl; + std::cerr << "[reserve_value_bytes] col_idx=" << col_idx << std::boolalpha + << " guard(chunk_bytes>0&&chunk_values>0&&reserve_values>0)=" + << (chunk_bytes > 0 && chunk_values > 0 && reserve_values > 0) + << " | inputs: reserve_values=" << reserve_values + << " chunk_values=" << chunk_values + << " chunk_bytes(uncompressed)=" << chunk_bytes + << " chunk_compressed_bytes=" << chunk_compressed_bytes + << " avg_bytes_per_value=" << avg_bytes_per_value + << " | min of: estimate_by_avg(avg*reserve_values)=" << estimate_by_avg + << ", cap_by_uncompressed(chunk_bytes)=" << chunk_bytes + << ", cap_by_decompression(compressed*" << kMaxReservationDecompressionFactor + << ")=" << cap_by_decompression + << " => reserve_value_bytes=" << reserve_value_bytes << std::endl; PAIMON_RETURN_NOT_OK(ExecuteSkipReadPattern(col_idx, effective_ranges, effective_total, reserve_values, reserve_value_bytes, From 2f3d925b9d1b9d3c1a3fa6859a0449486fcbcee3 Mon Sep 17 00:00:00 2001 From: "yonghao.fyh" Date: Thu, 17 Sep 2026 11:17:50 +0800 Subject: [PATCH 15/15] Revert "fix" This reverts commit 443a73479036ec041f8dee0b0395a29cfe3ce01a. --- include/paimon/utils/prefetch_cache_config.h | 2 +- .../late_materializing_file_batch_reader.h | 1 + .../page_filtered_row_group_reader.cpp | 38 ++++--------------- 3 files changed, 9 insertions(+), 32 deletions(-) diff --git a/include/paimon/utils/prefetch_cache_config.h b/include/paimon/utils/prefetch_cache_config.h index c0296b015..d767fd929 100644 --- a/include/paimon/utils/prefetch_cache_config.h +++ b/include/paimon/utils/prefetch_cache_config.h @@ -110,7 +110,7 @@ class PAIMON_EXPORT CacheConfig { // costs a round trip whatever its size. The limit is therefore well above // the page-sized gaps a filtered read leaves between the pages it keeps, // which would otherwise each cost a request of their own. - uint64_t range_size_limit_ = 4 * 1024 * 1024; + uint64_t range_size_limit_ = 8 * 1024 * 1024; uint64_t hole_size_limit_ = 512 * 1024; uint64_t pre_buffer_limit_ = 256 * 1024 * 1024; // Blocks are aligned to the END of the file, so a block never reaches past diff --git a/src/paimon/common/reader/late_materializing_file_batch_reader.h b/src/paimon/common/reader/late_materializing_file_batch_reader.h index db46fd62d..962acf6e6 100644 --- a/src/paimon/common/reader/late_materializing_file_batch_reader.h +++ b/src/paimon/common/reader/late_materializing_file_batch_reader.h @@ -178,6 +178,7 @@ class LateMaterializingFileBatchReader : public PrefetchFileBatchReader { std::shared_ptr arrow_pool_; /// Where the payload byte ranges are reported. Part of how this reader is wired into the /// prefetch layer rather than of a read generation, so Reset() leaves it alone. + /// TODO(yonghao.fyh): refactor pre buffer cache in the future PreBufferRangeCallback pre_buffer_range_callback_; LatMatState state_ = kInit; std::shared_ptr full_schema_; diff --git a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp index 62a95bd07..78f756fa0 100644 --- a/src/paimon/format/parquet/page_filtered_row_group_reader.cpp +++ b/src/paimon/format/parquet/page_filtered_row_group_reader.cpp @@ -21,7 +21,6 @@ #include #include -#include #include #include @@ -54,7 +53,7 @@ namespace { /// factor of it is bounded by bytes that physically exist. A leaf decompressing within the factor /// is reserved in full; a more compressible one is capped and left to the builder's amortized /// doubling, which is cheap against the I/O and decode of a read that large. -constexpr int64_t kMaxReservationDecompressionFactor = 20; +constexpr int64_t kMaxReservationDecompressionFactor = 8; struct DataPageLayout { int64_t column_chunk_offset; @@ -382,11 +381,6 @@ Result> PageFilteredRowGroupReader::ReadFil const int64_t chunk_compressed_bytes = column_chunk ? column_chunk->total_compressed_size() : 0; const int64_t chunk_values = column_chunk ? column_chunk->num_values() : 0; - // Named intermediates of the estimate below, declared here so the debug print after the - // block can show the whole calculation; they stay 0 when the guard does not hold. - double avg_bytes_per_value = 0.0; - int64_t estimate_by_avg = 0; - int64_t cap_by_decompression = 0; if (chunk_bytes > 0 && chunk_values > 0 && reserve_values > 0) { // A heuristic, not a bound. total_uncompressed_size is uncompressed but still // ENCODED: a dictionary page stores each value once and its data pages only @@ -400,32 +394,14 @@ Result> PageFilteredRowGroupReader::ReadFil // forgeable uncompressed one (see kMaxReservationDecompressionFactor). Fixed-width // leaves ignore the byte count entirely. const double avg = static_cast(chunk_bytes) / static_cast(chunk_values); - avg_bytes_per_value = avg; - // Candidate 1: average width over the whole chunk times the rows actually appended. - estimate_by_avg = SaturatingDoubleToInteger( - avg * static_cast(reserve_values)); - // Candidate 3: the file-bounded compressed size times the decompression factor. - cap_by_decompression = SaturatingDoubleToInteger( - static_cast(chunk_compressed_bytes) * - static_cast(kMaxReservationDecompressionFactor)); - // Candidate 2 is chunk_bytes itself (the uncompressed size), passed to min directly. - reserve_value_bytes = std::min({estimate_by_avg, chunk_bytes, cap_by_decompression}); + reserve_value_bytes = std::min( + {SaturatingDoubleToInteger(avg * static_cast(reserve_values)), + chunk_bytes, + SaturatingDoubleToInteger( + static_cast(chunk_compressed_bytes) * + static_cast(kMaxReservationDecompressionFactor))}); } - std::cerr << "[reserve_value_bytes] col_idx=" << col_idx << std::boolalpha - << " guard(chunk_bytes>0&&chunk_values>0&&reserve_values>0)=" - << (chunk_bytes > 0 && chunk_values > 0 && reserve_values > 0) - << " | inputs: reserve_values=" << reserve_values - << " chunk_values=" << chunk_values - << " chunk_bytes(uncompressed)=" << chunk_bytes - << " chunk_compressed_bytes=" << chunk_compressed_bytes - << " avg_bytes_per_value=" << avg_bytes_per_value - << " | min of: estimate_by_avg(avg*reserve_values)=" << estimate_by_avg - << ", cap_by_uncompressed(chunk_bytes)=" << chunk_bytes - << ", cap_by_decompression(compressed*" << kMaxReservationDecompressionFactor - << ")=" << cap_by_decompression - << " => reserve_value_bytes=" << reserve_value_bytes << std::endl; - PAIMON_RETURN_NOT_OK(ExecuteSkipReadPattern(col_idx, effective_ranges, effective_total, reserve_values, reserve_value_bytes, column_reader.get()));