fix: SkipIndex cause segment fault (#35908)

issue: #35882 pr: #35907 Signed-off-by: chyezh <[email protected]>
milvus-io · Sep 4, 2024 · b2f0290 · b2f0290
1 parent a1d3932
commit b2f0290
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 31 deletions.
diff --git 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
@@ -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);