[fix](be) Keep a requested constant value on a column reader cache hit

be/src/storage/segment/column_reader.h

@@ -175,10 +175,10 @@ class ColumnReader : public MetadataAdder<ColumnReader>,
     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<IndexFileReader>& index_file_reader,
-                              const TabletIndex* index_meta, const std::string& rowset_id,
-                              uint32_t segment_id, size_t rows_of_segment,
-                              std::unique_ptr<IndexIterator>* iterator);
+    virtual Status new_index_iterator(const std::shared_ptr<IndexFileReader>& index_file_reader,
+                                      const TabletIndex* index_meta, const std::string& rowset_id,
+                                      uint32_t segment_id, size_t rows_of_segment,
+                                      std::unique_ptr<IndexIterator>* iterator);
 
     Status seek_at_or_before(ordinal_t ordinal, OrdinalPageIndexIterator* iter,
                              const ColumnIteratorOptions& iter_opts);
@@ -195,6 +195,10 @@ class ColumnReader : public MetadataAdder<ColumnReader>,
     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
@@ -1097,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<IndexFileReader>& /*index_file_reader*/,
+                              const TabletIndex* /*index_meta*/, const std::string& /*rowset_id*/,
+                              uint32_t /*segment_id*/, size_t /*rows_of_segment*/,
+                              std::unique_ptr<IndexIterator>* /*iterator*/) override {
+        return Status::OK();
+    }
+
 private:
     Field _value;
 };

be/src/storage/segment/column_reader_cache.cpp

@@ -60,6 +60,15 @@ std::shared_ptr<ColumnReader> ColumnReaderCache::_lookup(const ColumnReaderCache
 
 void ColumnReaderCache::_insert_locked_nocheck(const ColumnReaderCacheKey& key,
                                                const std::shared_ptr<ColumnReader>& 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<Field> 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();
     }

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<int32_t>(FieldType::OLAP_FIELD_TYPE_BIGINT));
+    col_meta.set_unique_id(1);
+    col_meta.set_encoding(get_v2_default_encoding(static_cast<FieldType>(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<ColumnReader> 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<ColumnReader> const_reader;
+    status = _cache->get_column_reader(1, &const_reader, &_stats, nullptr,
+                                       Field::create_field<TYPE_BIGINT>(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<TYPE_BIGINT>(), 42);
+    EXPECT_EQ(zone_map.max_value.get<TYPE_BIGINT>(), 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<ColumnReader> 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<ColumnReader> second_const;
+    status = _cache->get_column_reader(1, &second_const, &_stats, nullptr,
+                                       Field::create_field<TYPE_BIGINT>(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<int32_t>(FieldType::OLAP_FIELD_TYPE_BIGINT));
+        metas[uid - 1].set_unique_id(uid);
+        metas[uid - 1].set_encoding(
+                get_v2_default_encoding(static_cast<FieldType>(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<ColumnReader> reader;
+    ASSERT_TRUE(_cache->get_column_reader(1, &reader, &_stats).ok());
+    ASSERT_TRUE(_cache->get_column_reader(1, &reader, &_stats, nullptr,
+                                          Field::create_field<TYPE_BIGINT>(Int64 {42}))
+                        .ok());
+    ASSERT_TRUE(reader->is_constant());
+    for (int uid = 2; uid <= 4; ++uid) {
+        std::shared_ptr<ColumnReader> 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

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<TYPE_BIGINT>(int64_t {7}));
+    std::unique_ptr<IndexIterator> 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) {