From b2f029072b2403b668031912280d399e6d608de8 Mon Sep 17 00:00:00 2001
From: Zhen Ye <chyezh@outlook.com>
Date: Tue, 3 Sep 2024 18:17:04 +0800
Subject: [PATCH] fix: SkipIndex cause segment fault (#35908)

issue: #35882
pr: #35907

Signed-off-by: chyezh <chyezh@outlook.com>
---
 internal/core/src/index/SkipIndex.cpp |  4 +-
 internal/core/src/index/SkipIndex.h   | 60 ++++++++++++++-------------
 2 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/internal/core/src/index/SkipIndex.cpp b/internal/core/src/index/SkipIndex.cpp
index dcf850bae27a1..286b6ccf37edb 100644
--- a/internal/core/src/index/SkipIndex.cpp
+++ b/internal/core/src/index/SkipIndex.cpp
@@ -123,8 +123,8 @@ SkipIndex::LoadString(milvus::FieldId field_id,
                 max_string = val;
             }
         }
-        chunkMetrics->min_ = Metrics(min_string);
-        chunkMetrics->max_ = Metrics(max_string);
+        chunkMetrics->min_ = Metrics(std::string(min_string));
+        chunkMetrics->max_ = Metrics(std::string(max_string));
     }
     std::unique_lock lck(mutex_);
     if (fieldChunkMetrics_.count(field_id) == 0) {
diff --git a/internal/core/src/index/SkipIndex.h b/internal/core/src/index/SkipIndex.h
index dba2cb1ebe89a..4287f931c5dd5 100644
--- a/internal/core/src/index/SkipIndex.h
+++ b/internal/core/src/index/SkipIndex.h
@@ -18,19 +18,41 @@
 
 namespace milvus {
 
-using Metrics = std::
-    variant<int8_t, int16_t, int32_t, int64_t, float, double, std::string_view>;
+using Metrics =
+    std::variant<int8_t, int16_t, int32_t, int64_t, float, double, std::string>;
 
+// MetricsDataType is used to avoid copy when get min/max value from FieldChunkMetrics
 template <typename T>
 using MetricsDataType =
     std::conditional_t<std::is_same_v<T, std::string>, std::string_view, T>;
 
+// ReverseMetricsDataType is used to avoid copy when get min/max value from FieldChunkMetrics
+template <typename T>
+using ReverseMetricsDataType =
+    std::conditional_t<std::is_same_v<T, std::string_view>, std::string, T>;
+
 struct FieldChunkMetrics {
     Metrics min_;
     Metrics max_;
     bool hasValue_;
 
     FieldChunkMetrics() : hasValue_(false){};
+
+    template <typename T>
+    std::pair<MetricsDataType<T>, MetricsDataType<T>>
+    GetMinMax() const {
+        AssertInfo(hasValue_,
+                   "GetMinMax should never be called when hasValue_ is false");
+        MetricsDataType<T> lower_bound;
+        MetricsDataType<T> upper_bound;
+        try {
+            lower_bound = std::get<ReverseMetricsDataType<T>>(min_);
+            upper_bound = std::get<ReverseMetricsDataType<T>>(max_);
+        } catch (const std::bad_variant_access& e) {
+            return {};
+        }
+        return {lower_bound, upper_bound};
+    }
 };
 
 class SkipIndex {
@@ -96,22 +118,6 @@ class SkipIndex {
         static constexpr bool value = isAllowedType && !isDisabledType;
     };
 
-    template <typename T>
-    std::pair<MetricsDataType<T>, MetricsDataType<T>>
-    GetMinMax(const FieldChunkMetrics& field_chunk_metrics) const {
-        MetricsDataType<T> lower_bound;
-        MetricsDataType<T> upper_bound;
-        try {
-            lower_bound =
-                std::get<MetricsDataType<T>>(field_chunk_metrics.min_);
-            upper_bound =
-                std::get<MetricsDataType<T>>(field_chunk_metrics.max_);
-        } catch (const std::bad_variant_access&) {
-            return {};
-        }
-        return {lower_bound, upper_bound};
-    }
-
     template <typename T>
     std::enable_if_t<SkipIndex::IsAllowedType<T>::value, bool>
     MinMaxUnaryFilter(const FieldChunkMetrics& field_chunk_metrics,
@@ -120,13 +126,12 @@ class SkipIndex {
         if (!field_chunk_metrics.hasValue_) {
             return false;
         }
-        std::pair<MetricsDataType<T>, MetricsDataType<T>> minMax =
-            GetMinMax<T>(field_chunk_metrics);
-        if (minMax.first == MetricsDataType<T>() ||
-            minMax.second == MetricsDataType<T>()) {
+        auto [lower_bound, upper_bound] = field_chunk_metrics.GetMinMax<T>();
+        if (lower_bound == MetricsDataType<T>() ||
+            upper_bound == MetricsDataType<T>()) {
             return false;
         }
-        return RangeShouldSkip<T>(val, minMax.first, minMax.second, op_type);
+        return RangeShouldSkip<T>(val, lower_bound, upper_bound, op_type);
     }
 
     template <typename T>
@@ -147,15 +152,12 @@ class SkipIndex {
         if (!field_chunk_metrics.hasValue_) {
             return false;
         }
-        std::pair<MetricsDataType<T>, MetricsDataType<T>> minMax =
-            GetMinMax<T>(field_chunk_metrics);
-        if (minMax.first == MetricsDataType<T>() ||
-            minMax.second == MetricsDataType<T>()) {
+        auto [lower_bound, upper_bound] = field_chunk_metrics.GetMinMax<T>();
+        if (lower_bound == MetricsDataType<T>() ||
+            upper_bound == MetricsDataType<T>()) {
             return false;
         }
         bool should_skip = false;
-        MetricsDataType<T> lower_bound = minMax.first;
-        MetricsDataType<T> upper_bound = minMax.second;
         if (lower_inclusive && upper_inclusive) {
             should_skip =
                 (lower_val > upper_bound) || (upper_val < lower_bound);