unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
refactoring

enhance: simplify index raw data check

Project: milvus-io/milvus | Language: C++ | Concepts: refactoring | Difficulty: mid
Link: https://github.com/milvus-io/milvus/pull/52030

The problem

The code for checking whether an index contains raw data was unnecessarily complex. It used a static helper function (CheckIndexHasRawData) that duplicated logic already present elsewhere, making the code harder to follow and maintain.

Why it's non-trivial

A naive fix might simply inline the check, but care must be taken to ensure that all logic paths (with or without a load_resource_request) are handled correctly, and that the code remains readable and consistent with the rest of the codebase.

The approach

1. Remove the static helper function and its declaration:

The following function was deleted from the implementation file:

-bool
-SegmentLoadInfo::CheckIndexHasRawData(const LoadIndexInfo& load_index_info) {
-    if (load_index_info.load_resource_request.has_value()) {
-        return milvus::index::IndexFactory::CanUseIndexRawDataForField(
-            load_index_info.field_type,
-            load_index_info.load_resource_request->has_raw_data);
-    } else {
-        auto request =
-            milvus::index::IndexFactory::GetInstance().IndexLoadResource(
-                load_index_info.field_type,
-                load_index_info.element_type,
-                load_index_info.index_engine_version,
-                load_index_info.index_size,
-                load_index_info.index_params,
-                load_index_info.enable_mmap,
-                load_index_info.num_rows,
-                load_index_info.dim);
-        return milvus::index::IndexFactory::CanUseIndexRawDataForField(
-            load_index_info.field_type, request.has_raw_data);
-    }
-}

This function checked if the index had raw data, either by using an existing load_resource_request or by creating a new one. Its removal eliminates duplicate logic.

The function’s declaration and documentation were also removed from the header:

-    /**
-    * @brief Check if a field's index has raw data
-    *
-    * Determines whether the index for a given field contains raw data
-    * by querying the IndexFactory with the index parameters.
-    *
-    * @param load_index_info The LoadIndexInfo containing index parameters
-    * @return true if the index has raw data, false otherwise
-    */
-    [[nodiscard]] static bool
-    CheckIndexHasRawData(const LoadIndexInfo& load_index_info);

This cleans up the class interface and removes outdated documentation.

2. Inline the logic directly where needed:

In the main logic, the check is now performed directly, using a single construction of request:

+            auto request =
+                milvus::index::IndexFactory::GetInstance().IndexLoadResource(
+                    load_index_info.field_type,
+                    load_index_info.element_type,
+                    load_index_info.index_engine_version,
+                    load_index_info.index_size,
+                    load_index_info.index_params,
+                    load_index_info.enable_mmap,
+                    load_index_info.num_rows,
+                    load_index_info.dim);
             if (!needs_file_context) {
-                load_index_info.load_resource_request =
-                    milvus::index::IndexFactory::GetInstance()
-                        .IndexLoadResource(load_index_info.field_type,
-                                           load_index_info.element_type,
-                                           load_index_info.index_engine_version,
-                                           load_index_info.index_size,
-                                           load_index_info.index_params,
-                                           load_index_info.enable_mmap,
-                                           load_index_info.num_rows,
-                                           load_index_info.dim);
+                load_index_info.load_resource_request = request;
             }
-            // Check if index has raw data before moving
-            if (CheckIndexHasRawData(load_index_info)) {
+            if (milvus::index::IndexFactory::CanUseIndexRawDataForField(
+                    load_index_info.field_type, request.has_raw_data)) {
                 field_index_has_raw_data_.insert(field_id);
             }

This avoids redundant construction of the request object and makes the logic clearer: the result of IndexLoadResource is used both to set load_resource_request and to check for raw data.

From the review thread

No notable review discussion on this PR.

Takeaways

  • Remove redundant abstractions: If a helper function simply wraps logic that is only used in one place, inlining it can make the code more readable and maintainable.
  • Avoid duplicate computation: Construct shared objects (like request) once and reuse them for multiple related operations.
  • Simplify control flow: Directly expressing logic in place, when not repeated, can reduce indirection and cognitive load for future readers.