Fix a bug that index property was not correctly passed (#1383)

Signed-off-by: yhmo <yihua.mo@zilliz.com>

sdk-core/src/main/java/io/milvus/client/AbstractMilvusGrpcClient.java

@@ -21,6 +21,7 @@ package io.milvus.client;
 
 import com.google.common.collect.Lists;
 import com.google.common.util.concurrent.*;
+import com.google.gson.reflect.TypeToken;
 import io.grpc.StatusRuntimeException;
 import io.milvus.common.utils.GTsDict;
 import io.milvus.common.utils.JsonUtils;
@@ -1331,9 +1332,22 @@ public abstract class AbstractMilvusGrpcClient implements MilvusClient {
         try {
             // prepare index parameters
             CreateIndexRequest.Builder createIndexRequestBuilder = CreateIndexRequest.newBuilder();
-            List<KeyValuePair> extraParamList = ParamUtils.AssembleKvPair(requestParam.getExtraParam());
-            if (CollectionUtils.isNotEmpty(extraParamList)) {
-                extraParamList.forEach(createIndexRequestBuilder::addExtraParams);
+            Map<String, String> extraParams = requestParam.getExtraParam();
+            for (Map.Entry<String, String> entry : extraParams.entrySet()) {
+                if (entry.getKey().equals(Constant.PARAMS)) {
+                    Map<String, String> tempParams = JsonUtils.fromJson(entry.getValue(), new TypeToken<Map<String, String>>() {}.getType());
+                    for (String key : tempParams.keySet()) {
+                        createIndexRequestBuilder.addExtraParams(KeyValuePair.newBuilder()
+                                .setKey(key)
+                                .setValue(tempParams.get(key))
+                                .build());
+                    }
+                } else {
+                    createIndexRequestBuilder.addExtraParams(KeyValuePair.newBuilder()
+                            .setKey(entry.getKey())
+                            .setValue(entry.getValue())
+                            .build());
+                }
             }
 
             CreateIndexRequest.Builder builder = createIndexRequestBuilder

sdk-core/src/main/java/io/milvus/response/DescIndexResponseWrapper.java

@@ -19,6 +19,8 @@
 
 package io.milvus.response;
 
+import com.google.gson.reflect.TypeToken;
+import io.milvus.common.utils.JsonUtils;
 import io.milvus.grpc.IndexDescription;
 import io.milvus.grpc.DescribeIndexResponse;
 
@@ -149,12 +151,18 @@ public class DescIndexResponseWrapper {
         }
 
         public String getExtraParam() {
-            if (this.params.containsKey(Constant.PARAMS)) {
-                // may throw IllegalArgumentException
-                return params.get(Constant.PARAMS);
+            Map<String, String> extraParams = new HashMap<>();
+            for (Map.Entry<String, String> entry : this.params.entrySet()) {
+                if (entry.getKey().equals(Constant.INDEX_TYPE) || entry.getKey().equals(Constant.METRIC_TYPE)) {
+                } else if (entry.getKey().equals(Constant.PARAMS)) {
+                    Map<String, String> tempParams = JsonUtils.fromJson(entry.getValue(), new TypeToken<Map<String, String>>() {}.getType());
+                    extraParams.putAll(tempParams);
+                } else {
+                    extraParams.put(entry.getKey(), entry.getValue());
+                }
             }
 
-            return "";
+            return JsonUtils.toJson(extraParams);
         }
     }
 }

sdk-core/src/main/java/io/milvus/v2/service/index/IndexService.java

@@ -66,15 +66,12 @@ public class IndexService extends BaseService {
             }
             Map<String, Object> extraParams = indexParam.getExtraParams();
             if (extraParams != null && !extraParams.isEmpty()) {
-                JsonObject params = new JsonObject();
                 for (String key : extraParams.keySet()) {
-                    params.addProperty(key, extraParams.get(key).toString());
+                    builder.addExtraParams(KeyValuePair.newBuilder()
+                            .setKey(key)
+                            .setValue(extraParams.get(key).toString())
+                            .build());
                 }
-                // the extra params is a JSON format string like "{\"M\": 8, \"efConstruction\": 64}"
-                builder.addExtraParams(KeyValuePair.newBuilder()
-                        .setKey(Constant.PARAMS)
-                        .setValue(params.toString())
-                        .build());
             }
 
             Status status = blockingStub.createIndex(builder.build());

sdk-core/src/main/java/io/milvus/v2/service/index/response/DescribeIndexResp.java

@@ -77,7 +77,17 @@ public class DescribeIndexResp {
         private IndexBuildState indexState = IndexBuildState.IndexStateNone;
         @Builder.Default
         String indexFailedReason = "";
+
+        // In 2.4/2.5, properties only contains one item "mmap.enabled".
+        // To keep consistence with other SDKs, we intend to remove this member from IndexDesc,
+        // and put "mmap.enabled" into the "extraParams", the reasons:
+        //  (1) when createIndex() is call, "mmap.enabled" is passed by the IndexParam.extraParams
+        //  (2) other SDKs don't have a "properties" member for describeIndex()
+        //  (3) now the "mmap.enabled" is dispatched to "properties" by ConvertUtils.convertToDescribeIndexResp(),
+        //      once there are new property available, the new property will be dispatched to "extraParams",
+        //      the "properties" member is not maintainable.
         @Builder.Default
+        @Deprecated
         private Map<String, String> properties = new HashMap<>();
     }
 }

sdk-core/src/main/java/io/milvus/v2/utils/ConvertUtils.java

@@ -97,9 +97,14 @@ public class ConvertUtils {
                     // may throw IllegalArgumentException
                     metricType = IndexParam.MetricType.valueOf(param.getValue());
                 } else if (param.getKey().equals(Constant.MMAP_ENABLED)) {
-                    properties.put(param.getKey(), param.getValue());
+                    properties.put(param.getKey(), param.getValue()); // just for compatible with old versions
+                    extraParams.put(param.getKey(), param.getValue());
                 } else if (param.getKey().equals(Constant.PARAMS)) {
-                    extraParams = JsonUtils.fromJson(param.getValue(), new TypeToken<Map<String, String>>() {}.getType());
+                    Map<String, String> tempParams = JsonUtils.fromJson(param.getValue(), new TypeToken<Map<String, String>>() {}.getType());
+                    tempParams.remove(Constant.MMAP_ENABLED); // "mmap.enabled" in "params" is not processed by server
+                    extraParams.putAll(tempParams);
+                } else {
+                    extraParams.put(param.getKey(), param.getValue());
                 }
             }
 

sdk-core/src/test/java/io/milvus/client/MilvusClientDockerTest.java

@@ -41,10 +41,7 @@ import io.milvus.param.highlevel.dml.DeleteIdsParam;
 import io.milvus.param.highlevel.dml.GetIdsParam;
 import io.milvus.param.highlevel.dml.response.DeleteResponse;
 import io.milvus.param.highlevel.dml.response.GetResponse;
-import io.milvus.param.index.CreateIndexParam;
-import io.milvus.param.index.DescribeIndexParam;
-import io.milvus.param.index.DropIndexParam;
-import io.milvus.param.index.GetIndexStateParam;
+import io.milvus.param.index.*;
 import io.milvus.param.partition.GetPartitionStatisticsParam;
 import io.milvus.param.partition.ShowPartitionsParam;
 import io.milvus.pool.MilvusClientV1Pool;
@@ -457,13 +454,14 @@ class MilvusClientDockerTest {
         Assertions.assertEquals(R.Status.Success.getCode(), createIndexR.getStatus().intValue());
 
         // create index on vector field
+        String params = "{\"efConstruction\":64,\"M\":16,\"mmap.enabled\":true}";
         indexParam = CreateIndexParam.newBuilder()
                 .withCollectionName(randomCollectionName)
                 .withFieldName(DataType.FloatVector.name())
                 .withIndexName("abv")
                 .withIndexType(IndexType.HNSW)
                 .withMetricType(MetricType.L2)
-                .withExtraParam("{\"M\":16,\"efConstruction\":64}")
+                .withExtraParam(params)
                 .withSyncMode(Boolean.TRUE)
                 .withSyncWaitingInterval(500L)
                 .withSyncWaitingTimeout(30L)
@@ -491,8 +489,24 @@ class MilvusClientDockerTest {
         Assertions.assertEquals(rowCount, indexDesc.getIndexedRows());
         Assertions.assertEquals(0L, indexDesc.getPendingIndexRows());
         Assertions.assertTrue(indexDesc.getIndexFailedReason().isEmpty());
+        String extraParams = indexDesc.getExtraParam();
+        Assertions.assertEquals(params.replace("\"", ""), extraParams.replace("\"", ""));
         System.out.println("Index description: " + indexDesc.toString());
 
+        R<RpcStatus> alterR = client.alterIndex(AlterIndexParam.newBuilder()
+                .withCollectionName(randomCollectionName)
+                .withIndexName("abv")
+                .withMMapEnabled(false)
+                .build());
+        Assertions.assertEquals(R.Status.Success.getCode(), alterR.getStatus().intValue());
+
+        descIndexR = client.describeIndex(descIndexParam);
+        Assertions.assertEquals(R.Status.Success.getCode(), descIndexR.getStatus().intValue());
+        indexDescWrapper = new DescIndexResponseWrapper(descIndexR.getData());
+        indexDesc = indexDescWrapper.getIndexDescByFieldName(DataType.FloatVector.name());
+        extraParams = indexDesc.getExtraParam();
+        Assertions.assertEquals("{efConstruction:64,M:16,mmap.enabled:false}", extraParams.replace("\"", ""));
+
         // load collection
         R<RpcStatus> loadR = client.loadCollection(LoadCollectionParam.newBuilder()
                 .withCollectionName(randomCollectionName)

sdk-core/src/test/java/io/milvus/client/MilvusServiceClientTest.java

@@ -1430,7 +1430,7 @@ class MilvusServiceClientTest {
                 .withFieldName("aaa")
                 .withIndexType(IndexType.IVF_FLAT)
                 .withMetricType(MetricType.L2)
-                .withExtraParam("dummy")
+                .withExtraParam("{\"dummy\": 0}")
                 .withSyncMode(Boolean.TRUE)
                 .withSyncWaitingInterval(500L)
                 .withSyncWaitingTimeout(2L)
@@ -1445,7 +1445,7 @@ class MilvusServiceClientTest {
                 .withFieldName("field1")
                 .withIndexType(IndexType.BIN_IVF_FLAT)
                 .withMetricType(MetricType.L2)
-                .withExtraParam("dummy")
+                .withExtraParam("{\"dummy\": 1}")
                 .withSyncMode(Boolean.TRUE)
                 .withSyncWaitingInterval(500L)
                 .withSyncWaitingTimeout(2L)
@@ -1472,7 +1472,7 @@ class MilvusServiceClientTest {
                 .withFieldName("field1")
                 .withIndexType(IndexType.IVF_FLAT)
                 .withMetricType(MetricType.L2)
-                .withExtraParam("dummy")
+                .withExtraParam("{\"dummy\": 2}")
                 .withSyncMode(Boolean.TRUE)
                 .withSyncWaitingInterval(500L)
                 .withSyncWaitingTimeout(2L)
@@ -2737,7 +2737,8 @@ class MilvusServiceClientTest {
         assertEquals(fieldName, indexDesc.getFieldName());
         assertEquals(indexType, indexDesc.getIndexType());
         assertEquals(metricType, indexDesc.getMetricType());
-        assertEquals(0, extraParam.compareTo(indexDesc.getExtraParam()));
+        String params = indexDesc.getExtraParam();
+        assertEquals(0, extraParam.compareTo(params.replace("\"", "")));
 
         assertFalse(wrapper.toString().isEmpty());
     }

sdk-core/src/test/java/io/milvus/v2/client/MilvusClientV2DockerTest.java

@@ -1248,6 +1248,9 @@ class MilvusClientV2DockerTest {
         Map<String, String> indexProps = desc.getProperties();
         Assertions.assertTrue(indexProps.containsKey(Constant.MMAP_ENABLED));
         Assertions.assertEquals("false", indexProps.get(Constant.MMAP_ENABLED));
+        extraParams = desc.getExtraParams();
+        Assertions.assertTrue(extraParams.containsKey(Constant.MMAP_ENABLED));
+        Assertions.assertEquals("false", extraParams.get(Constant.MMAP_ENABLED));
 
         client.dropIndexProperties(DropIndexPropertiesReq.builder()
                 .collectionName(randomCollectionName)
@@ -1261,6 +1264,8 @@ class MilvusClientV2DockerTest {
         desc = descResp.getIndexDescByFieldName("vector");
         indexProps = desc.getProperties();
         Assertions.assertFalse(indexProps.containsKey(Constant.MMAP_ENABLED));
+        extraParams = desc.getExtraParams();
+        Assertions.assertFalse(extraParams.containsKey(Constant.MMAP_ENABLED));
 
         // drop index
         client.dropIndex(DropIndexReq.builder()