From 2e7d6a02e4c7b78403c1304856e7ab0aa4c0df71 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Tue, 15 Sep 2026 18:14:10 +0800 Subject: [PATCH 1/2] [fix](be) Keep a requested constant value on a column reader cache hit --- be/src/storage/segment/column_reader.h | 6 ++ .../storage/segment/column_reader_cache.cpp | 17 +++- .../segment/column_reader_cache_test.cpp | 92 +++++++++++++++++++ 3 files changed, 113 insertions(+), 2 deletions(-) diff --git a/be/src/storage/segment/column_reader.h b/be/src/storage/segment/column_reader.h index 33f10df173eb8c..457db38c48bca2 100644 --- a/be/src/storage/segment/column_reader.h +++ b/be/src/storage/segment/column_reader.h @@ -195,6 +195,10 @@ class ColumnReader : public MetadataAdder, const EncodingInfo* encoding_info() const { return _encoding_info; } virtual bool has_zone_map() const { return _zone_map_index != nullptr; } + // True for a reader that serves a value supplied by the caller instead of the one on disk. + // The reader cache needs to tell the two apart: a caller that asked for a constant must not be + // served an on-disk reader another caller cached earlier. + virtual bool is_constant() const { return false; } bool has_bloom_filter_index(bool ngram) const; // Check if this column could match `cond' using segment zone map. // Since segment zone map is stored in metadata, this function is fast without I/O. @@ -1078,6 +1082,8 @@ class ConstantColumnReader : public ColumnReader { bool has_zone_map() const override { return true; } + bool is_constant() const override { return true; } + // The base ColumnReader default-constructs without initializing its _meta_type. The data-read // path (Segment::new_column_iterator) verifies tablet_column.type() == reader->get_meta_type() // when config::enable_column_type_check is on (default true), so derive the real OLAP type from diff --git a/be/src/storage/segment/column_reader_cache.cpp b/be/src/storage/segment/column_reader_cache.cpp index 4b9d0cbe107dba..23d943e305b57c 100644 --- a/be/src/storage/segment/column_reader_cache.cpp +++ b/be/src/storage/segment/column_reader_cache.cpp @@ -60,6 +60,15 @@ std::shared_ptr ColumnReaderCache::_lookup(const ColumnReaderCache void ColumnReaderCache::_insert_locked_nocheck(const ColumnReaderCacheKey& key, const std::shared_ptr& reader) { + // Replacing an existing key updates its node in place. Pushing a second node for the same key + // would leave the first one unreachable in the list while eviction erases the map entry of + // whichever copy reaches the tail, dropping the live reader from the map. + if (auto it = _cache_map.find(key); it != _cache_map.end()) { + it->second->reader = reader; + it->second->last_access = std::chrono::steady_clock::now(); + _lru_list.splice(_lru_list.begin(), _lru_list, it->second); + return; + } // If capacity exceeded, remove least recently used (tail) if (_cache_map.size() >= config::max_segment_partial_column_cache_size) { g_segment_column_reader_cache_count << -1; @@ -98,8 +107,12 @@ Status ColumnReaderCache::get_column_reader(int32_t col_uid, OlapReaderStatistics* stats, const io::IOContext* source_io_ctx, std::optional const_value) { - // Attempt to find in cache - if (auto cached = _lookup({col_uid, {}})) { + // A caller that passes const_value reads a column whose on-disk value is a placeholder, so it + // must not be served the on-disk reader that a caller without const_value cached earlier: that + // reader would hand back the placeholder both as row data and as a zone map. Fall through and + // build the constant reader, replacing the cached entry so later callers get the real value too. + if (auto cached = _lookup({col_uid, {}}); + cached != nullptr && (!const_value.has_value() || cached->is_constant())) { *column_reader = cached; return Status::OK(); } diff --git a/be/test/storage/segment/column_reader_cache_test.cpp b/be/test/storage/segment/column_reader_cache_test.cpp index 2a70be90ec2d5a..dcf0984db9d232 100644 --- a/be/test/storage/segment/column_reader_cache_test.cpp +++ b/be/test/storage/segment/column_reader_cache_test.cpp @@ -31,6 +31,7 @@ #include "core/assert_cast.h" #include "core/data_type/data_type_array.h" #include "core/data_type/data_type_nullable.h" +#include "core/field.h" #include "io/fs/file_reader.h" #include "io/fs/local_file_system.h" #include "storage/segment/column_meta_accessor.h" @@ -229,6 +230,97 @@ TEST_F(ColumnReaderCacheTest, BasicCacheOperations) { EXPECT_EQ(readers[1], reader); } +// A caller asking for a constant-backed reader must get one even when a caller that passed no +// constant cached the on-disk reader for that column first. +TEST_F(ColumnReaderCacheTest, ConstValueIsNotDroppedOnCacheHit) { + setup_column_uid_mapping(1, 0); + + ColumnMetaPB col_meta; + col_meta.set_type(static_cast(FieldType::OLAP_FIELD_TYPE_BIGINT)); + col_meta.set_unique_id(1); + col_meta.set_encoding(get_v2_default_encoding(static_cast(col_meta.type()))); + col_meta.mutable_indexes()->Add()->set_type(ORDINAL_INDEX); + setup_segment_footer({col_meta}); + + // The placeholder reader lands in the cache first, as build_segment_zonemap_context does. + std::shared_ptr on_disk_reader; + Status status = _cache->get_column_reader(1, &on_disk_reader, &_stats); + ASSERT_TRUE(status.ok()) << status.to_string(); + ASSERT_NE(on_disk_reader, nullptr); + EXPECT_FALSE(on_disk_reader->is_constant()); + + std::shared_ptr const_reader; + status = _cache->get_column_reader(1, &const_reader, &_stats, nullptr, + Field::create_field(Int64 {42})); + ASSERT_TRUE(status.ok()) << status.to_string(); + ASSERT_NE(const_reader, nullptr); + EXPECT_TRUE(const_reader->is_constant()); + EXPECT_NE(const_reader, on_disk_reader); + EXPECT_EQ(const_reader->get_meta_type(), FieldType::OLAP_FIELD_TYPE_BIGINT); + + // The summary it reports is the constant, which is what makes zone-map pruning agree with the + // rows this reader hands back. Iterator output has its own coverage in + // constant_column_iterator_test.cpp. + segment_v2::ZoneMap zone_map; + ASSERT_TRUE(const_reader->get_segment_zone_map(&zone_map).ok()); + EXPECT_TRUE(zone_map.has_not_null); + EXPECT_FALSE(zone_map.has_null); + EXPECT_FALSE(zone_map.pass_all); + EXPECT_EQ(zone_map.min_value.get(), 42); + EXPECT_EQ(zone_map.max_value.get(), 42); + + // The entry was replaced rather than duplicated, so a later caller that cannot supply the + // constant also stops seeing the placeholder. + std::shared_ptr after_replace; + status = _cache->get_column_reader(1, &after_replace, &_stats); + ASSERT_TRUE(status.ok()) << status.to_string(); + EXPECT_EQ(after_replace, const_reader); + + auto readers = _cache->get_available_readers(false); + EXPECT_EQ(readers.size(), 1); + EXPECT_EQ(readers[1], const_reader); + + // Asking again with a constant is a plain cache hit now. + std::shared_ptr second_const; + status = _cache->get_column_reader(1, &second_const, &_stats, nullptr, + Field::create_field(Int64 {42})); + ASSERT_TRUE(status.ok()) << status.to_string(); + EXPECT_EQ(second_const, const_reader); +} + +// Replacing an entry must update its node rather than push a second one for the same key. +TEST_F(ColumnReaderCacheTest, SameKeyReplacementDoesNotLeaveAStaleLruNode) { + config::max_segment_partial_column_cache_size = 3; + ColumnMetaPB metas[4]; + for (int uid = 1; uid <= 4; ++uid) { + setup_column_uid_mapping(uid, uid - 1); + metas[uid - 1].set_type(static_cast(FieldType::OLAP_FIELD_TYPE_BIGINT)); + metas[uid - 1].set_unique_id(uid); + metas[uid - 1].set_encoding( + get_v2_default_encoding(static_cast(metas[uid - 1].type()))); + metas[uid - 1].mutable_indexes()->Add()->set_type(ORDINAL_INDEX); + } + setup_segment_footer({metas[0], metas[1], metas[2], metas[3]}); + + // Replace uid 1's entry, then fill the cache so that eviction has to pick a victim. + std::shared_ptr reader; + ASSERT_TRUE(_cache->get_column_reader(1, &reader, &_stats).ok()); + ASSERT_TRUE(_cache->get_column_reader(1, &reader, &_stats, nullptr, + Field::create_field(Int64 {42})) + .ok()); + ASSERT_TRUE(reader->is_constant()); + for (int uid = 2; uid <= 4; ++uid) { + std::shared_ptr other; + ASSERT_TRUE(_cache->get_column_reader(uid, &other, &_stats).ok()); + } + + // A second node for uid 1 would still be reachable through the LRU list while eviction erased + // uid 1's map entry, leaving the two views disagreeing about what is cached. + auto readers = _cache->get_available_readers(false); + EXPECT_EQ(readers.count(1), 0); + EXPECT_EQ(readers.size(), 3); +} + // Test LRU eviction TEST_F(ColumnReaderCacheTest, LRUEviction) { // Set cache size to 2 From 3c7b4c426417d2f96beab15082a670fd7fc54d1b Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Wed, 16 Sep 2026 03:29:09 +0800 Subject: [PATCH 2/2] [fix](be) Skip physical index initialization for a constant column reader --- be/src/storage/segment/column_reader.h | 20 +++++++++++++++---- .../segment/constant_column_iterator_test.cpp | 10 ++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/be/src/storage/segment/column_reader.h b/be/src/storage/segment/column_reader.h index 457db38c48bca2..4acd251ec51be8 100644 --- a/be/src/storage/segment/column_reader.h +++ b/be/src/storage/segment/column_reader.h @@ -175,10 +175,10 @@ class ColumnReader : public MetadataAdder, Status new_map_iterator(ColumnIteratorUPtr* iterator, const TabletColumn* tablet_column); Status new_agg_state_iterator(ColumnIteratorUPtr* iterator); - Status new_index_iterator(const std::shared_ptr& index_file_reader, - const TabletIndex* index_meta, const std::string& rowset_id, - uint32_t segment_id, size_t rows_of_segment, - std::unique_ptr* iterator); + virtual Status new_index_iterator(const std::shared_ptr& index_file_reader, + const TabletIndex* index_meta, const std::string& rowset_id, + uint32_t segment_id, size_t rows_of_segment, + std::unique_ptr* iterator); Status seek_at_or_before(ordinal_t ordinal, OrdinalPageIndexIterator* iter, const ColumnIteratorOptions& iter_opts); @@ -1103,6 +1103,18 @@ class ConstantColumnReader : public ColumnReader { Status get_segment_zone_map(segment_v2::ZoneMap* zone_map) const override; + // This reader serves a value the caller supplied, so the on-disk index for the column describes + // something else: for a placeholder column it indexes the placeholder. Leaving the iterator + // unset makes the caller fall back to reading through this reader, the same as the path that + // finds no reader at all. The base implementation would also run on physical state this class + // never initializes. + Status new_index_iterator(const std::shared_ptr& /*index_file_reader*/, + const TabletIndex* /*index_meta*/, const std::string& /*rowset_id*/, + uint32_t /*segment_id*/, size_t /*rows_of_segment*/, + std::unique_ptr* /*iterator*/) override { + return Status::OK(); + } + private: Field _value; }; diff --git a/be/test/storage/segment/constant_column_iterator_test.cpp b/be/test/storage/segment/constant_column_iterator_test.cpp index 0c596199312252..2db7ba763a0e88 100644 --- a/be/test/storage/segment/constant_column_iterator_test.cpp +++ b/be/test/storage/segment/constant_column_iterator_test.cpp @@ -97,6 +97,16 @@ TEST_F(ConstantColumnIteratorTest, MatchConditionUsesConstantZoneMap) { EXPECT_FALSE(matched); } +// A constant reader has no physical index to open, so index-iterator creation is a no-op rather +// than an attempt to load one from state this reader never initializes. +TEST_F(ConstantColumnIteratorTest, NewIndexIteratorIsANoOp) { + ConstantColumnReader reader(Field::create_field(int64_t {7})); + std::unique_ptr iter; + auto st = reader.new_index_iterator(nullptr, nullptr, "", 0, 0, &iter); + ASSERT_TRUE(st.ok()) << st; + EXPECT_EQ(iter, nullptr); +} + // next_batch fills every row with the constant value, advances the ordinal, // and reports has_null = false for a non-null value. TEST_F(ConstantColumnIteratorTest, NextBatchFillsConstant) {