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.