Skip to content

Commit cd509c8

Browse files
wangting0128wangting0128
authored
fix: add WarmupKey to checkParams filter for CreateIndex idempotency (milvus-io#47595)
issue: milvus-io#39803 When creating an index, WarmupKey is removed from TypeParams before storage (in index_service.go). However, checkParams function was not filtering WarmupKey when comparing TypeParams for idempotency check, causing the second CreateIndex call to fail with "at most one distinct index is allowed per field" error. This fix adds WarmupKey to the DeleteParams filter in checkParams function, ensuring consistent behavior with index creation and proper idempotency. Signed-off-by: wangting0128 <ting.wang@zilliz.com>
1 parent 88cd109 commit cd509c8

File tree

3 files changed

+215
-2
lines changed

3 files changed

+215
-2
lines changed

‎internal/datacoord/index_meta.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,8 @@ func checkIdenticalJson(index *model.Index, req *indexpb.CreateIndexRequest) boo
266266
}
267267

268268
func checkParams(fieldIndex *model.Index, req *indexpb.CreateIndexRequest) bool {
269-
metaTypeParams := DeleteParams(fieldIndex.TypeParams, []string{common.MmapEnabledKey})
270-
reqTypeParams := DeleteParams(req.TypeParams, []string{common.MmapEnabledKey})
269+
metaTypeParams := DeleteParams(fieldIndex.TypeParams, []string{common.MmapEnabledKey, common.WarmupKey})
270+
reqTypeParams := DeleteParams(req.TypeParams, []string{common.MmapEnabledKey, common.WarmupKey})
271271
if len(metaTypeParams) != len(reqTypeParams) {
272272
return false
273273
}

‎internal/datacoord/index_meta_test.go‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1716,3 +1716,124 @@ func TestMeta_GetSegmentIndexStatus(t *testing.T) {
17161716
assert.Empty(t, segmentIndexes)
17171717
})
17181718
}
1719+
1720+
func TestCheckParams(t *testing.T) {
1721+
t.Run("same params without warmup", func(t *testing.T) {
1722+
fieldIndex := &model.Index{
1723+
TypeParams: []*commonpb.KeyValuePair{
1724+
{Key: common.DimKey, Value: "128"},
1725+
},
1726+
UserIndexParams: []*commonpb.KeyValuePair{
1727+
{Key: common.MetricTypeKey, Value: "L2"},
1728+
{Key: common.IndexTypeKey, Value: "HNSW"},
1729+
},
1730+
}
1731+
req := &indexpb.CreateIndexRequest{
1732+
TypeParams: []*commonpb.KeyValuePair{
1733+
{Key: common.DimKey, Value: "128"},
1734+
},
1735+
UserIndexParams: []*commonpb.KeyValuePair{
1736+
{Key: common.MetricTypeKey, Value: "L2"},
1737+
{Key: common.IndexTypeKey, Value: "HNSW"},
1738+
},
1739+
}
1740+
assert.True(t, checkParams(fieldIndex, req))
1741+
})
1742+
1743+
t.Run("same params with warmup in request only", func(t *testing.T) {
1744+
// This test verifies the fix for the idempotency bug
1745+
// When index is created, WarmupKey is removed from stored TypeParams
1746+
// But when checking idempotency, WarmupKey should also be ignored in request
1747+
fieldIndex := &model.Index{
1748+
TypeParams: []*commonpb.KeyValuePair{
1749+
{Key: common.DimKey, Value: "128"},
1750+
// Note: WarmupKey is NOT in stored TypeParams because it was removed during CreateIndex
1751+
},
1752+
UserIndexParams: []*commonpb.KeyValuePair{
1753+
{Key: common.MetricTypeKey, Value: "L2"},
1754+
{Key: common.IndexTypeKey, Value: "HNSW"},
1755+
},
1756+
}
1757+
req := &indexpb.CreateIndexRequest{
1758+
TypeParams: []*commonpb.KeyValuePair{
1759+
{Key: common.DimKey, Value: "128"},
1760+
{Key: common.WarmupKey, Value: "sync"}, // WarmupKey in request should be ignored
1761+
},
1762+
UserIndexParams: []*commonpb.KeyValuePair{
1763+
{Key: common.MetricTypeKey, Value: "L2"},
1764+
{Key: common.IndexTypeKey, Value: "HNSW"},
1765+
},
1766+
}
1767+
// Before fix: this would return false because len(metaTypeParams) != len(reqTypeParams)
1768+
// After fix: this should return true because WarmupKey is filtered from both
1769+
assert.True(t, checkParams(fieldIndex, req))
1770+
})
1771+
1772+
t.Run("same params with warmup in different order", func(t *testing.T) {
1773+
fieldIndex := &model.Index{
1774+
TypeParams: []*commonpb.KeyValuePair{
1775+
{Key: common.DimKey, Value: "128"},
1776+
},
1777+
UserIndexParams: []*commonpb.KeyValuePair{
1778+
{Key: common.MetricTypeKey, Value: "L2"},
1779+
{Key: common.IndexTypeKey, Value: "HNSW"},
1780+
},
1781+
}
1782+
req := &indexpb.CreateIndexRequest{
1783+
TypeParams: []*commonpb.KeyValuePair{
1784+
{Key: common.WarmupKey, Value: "sync"},
1785+
{Key: common.DimKey, Value: "128"},
1786+
},
1787+
UserIndexParams: []*commonpb.KeyValuePair{
1788+
{Key: common.IndexTypeKey, Value: "HNSW"},
1789+
{Key: common.MetricTypeKey, Value: "L2"},
1790+
},
1791+
}
1792+
assert.True(t, checkParams(fieldIndex, req))
1793+
})
1794+
1795+
t.Run("different params", func(t *testing.T) {
1796+
fieldIndex := &model.Index{
1797+
TypeParams: []*commonpb.KeyValuePair{
1798+
{Key: common.DimKey, Value: "128"},
1799+
},
1800+
UserIndexParams: []*commonpb.KeyValuePair{
1801+
{Key: common.MetricTypeKey, Value: "L2"},
1802+
{Key: common.IndexTypeKey, Value: "HNSW"},
1803+
},
1804+
}
1805+
req := &indexpb.CreateIndexRequest{
1806+
TypeParams: []*commonpb.KeyValuePair{
1807+
{Key: common.DimKey, Value: "256"}, // Different dimension
1808+
},
1809+
UserIndexParams: []*commonpb.KeyValuePair{
1810+
{Key: common.MetricTypeKey, Value: "L2"},
1811+
{Key: common.IndexTypeKey, Value: "HNSW"},
1812+
},
1813+
}
1814+
assert.False(t, checkParams(fieldIndex, req))
1815+
})
1816+
1817+
t.Run("mmap enabled should be ignored", func(t *testing.T) {
1818+
fieldIndex := &model.Index{
1819+
TypeParams: []*commonpb.KeyValuePair{
1820+
{Key: common.DimKey, Value: "128"},
1821+
},
1822+
UserIndexParams: []*commonpb.KeyValuePair{
1823+
{Key: common.MetricTypeKey, Value: "L2"},
1824+
{Key: common.IndexTypeKey, Value: "HNSW"},
1825+
},
1826+
}
1827+
req := &indexpb.CreateIndexRequest{
1828+
TypeParams: []*commonpb.KeyValuePair{
1829+
{Key: common.DimKey, Value: "128"},
1830+
{Key: common.MmapEnabledKey, Value: "true"},
1831+
},
1832+
UserIndexParams: []*commonpb.KeyValuePair{
1833+
{Key: common.MetricTypeKey, Value: "L2"},
1834+
{Key: common.IndexTypeKey, Value: "HNSW"},
1835+
},
1836+
}
1837+
assert.True(t, checkParams(fieldIndex, req))
1838+
})
1839+
}

‎tests/python_client/milvus_client/test_milvus_client_alter.py‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,98 @@ def test_milvus_client_alter_index_unsupported_value(self):
125125
properties={"mmap.enabled": value},
126126
check_task=CheckTasks.err_res, check_items=error)
127127

128+
@pytest.mark.tags(CaseLabel.L1)
129+
def test_milvus_client_create_index_idempotent_with_field_warmup(self):
130+
"""
131+
target: test create index idempotency when warmup is set at field level
132+
method: 1. create collection
133+
2. release collection
134+
3. alter field to set warmup=sync at field level (this adds warmup to field TypeParams)
135+
4. drop existing index
136+
5. create index (first time)
137+
6. create same index again (second time - should be idempotent)
138+
expected: both create_index calls should succeed (idempotent behavior)
139+
issue: https://github.com/milvus-io/milvus/issues/XXXXX
140+
note: This test verifies the fix for checkParams function in index_meta.go
141+
which was missing WarmupKey in DeleteParams, causing idempotency check to fail.
142+
When index is created, WarmupKey is removed from stored TypeParams.
143+
When checking idempotency, WarmupKey should also be filtered from request TypeParams.
144+
"""
145+
client = self._client()
146+
collection_name = cf.gen_collection_name_by_testcase_name()
147+
# 1. create collection
148+
self.create_collection(client, collection_name, default_dim, consistency_level="Strong")
149+
# 2. release collection before altering field properties
150+
self.release_collection(client, collection_name)
151+
# 3. alter field to set warmup=sync at field level
152+
# This adds warmup to field TypeParams, which will be included in CreateIndex request
153+
self.alter_collection_field(client, collection_name, field_name=default_vector_field_name,
154+
field_params={"warmup": "sync"})
155+
# 4. drop existing index
156+
self.drop_index(client, collection_name, default_vector_field_name)
157+
res = self.list_indexes(client, collection_name)[0]
158+
assert res == []
159+
# 5. prepare index params
160+
index_params = self.prepare_index_params(client)[0]
161+
index_params.add_index(field_name=default_vector_field_name, index_type="HNSW",
162+
metric_type="L2", params={"M": 8, "efConstruction": 200})
163+
# 6. create index (first time) - should succeed
164+
self.create_index(client, collection_name, index_params)
165+
idx_names, _ = self.list_indexes(client, collection_name, field_name=default_vector_field_name)
166+
assert len(idx_names) == 1
167+
# 7. create same index again (second time) - should be idempotent and succeed
168+
# Before fix: this would fail with "at most one distinct index is allowed per field"
169+
# because checkParams didn't filter WarmupKey from TypeParams comparison
170+
self.create_index(client, collection_name, index_params)
171+
idx_names_after, _ = self.list_indexes(client, collection_name, field_name=default_vector_field_name)
172+
assert len(idx_names_after) == 1
173+
assert idx_names == idx_names_after
174+
# 8. cleanup
175+
self.drop_collection(client, collection_name)
176+
177+
@pytest.mark.tags(CaseLabel.L1)
178+
def test_milvus_client_create_index_idempotent_with_collection_warmup(self):
179+
"""
180+
target: test create index idempotency when warmup is set at collection level
181+
method: 1. create collection
182+
2. release collection
183+
3. alter collection properties to set warmup.vectorField=sync
184+
4. drop existing index
185+
5. create index (first time)
186+
6. create same index again (second time - should be idempotent)
187+
expected: both create_index calls should succeed (idempotent behavior)
188+
note: This test verifies the fix for checkParams function in index_meta.go
189+
using collection-level warmup settings (warmup.vectorField)
190+
"""
191+
client = self._client()
192+
collection_name = cf.gen_collection_name_by_testcase_name()
193+
# 1. create collection
194+
self.create_collection(client, collection_name, default_dim, consistency_level="Strong")
195+
# 2. release collection before altering properties
196+
self.release_collection(client, collection_name)
197+
# 3. alter collection properties to set warmup.vectorField=sync
198+
self.alter_collection_properties(client, collection_name,
199+
properties={"warmup.vectorField": "sync"})
200+
# 4. drop existing index
201+
self.drop_index(client, collection_name, default_vector_field_name)
202+
res = self.list_indexes(client, collection_name)[0]
203+
assert res == []
204+
# 5. prepare index params
205+
index_params = self.prepare_index_params(client)[0]
206+
index_params.add_index(field_name=default_vector_field_name, index_type="HNSW",
207+
metric_type="L2", params={"M": 8, "efConstruction": 200})
208+
# 6. create index (first time) - should succeed
209+
self.create_index(client, collection_name, index_params)
210+
idx_names, _ = self.list_indexes(client, collection_name, field_name=default_vector_field_name)
211+
assert len(idx_names) == 1
212+
# 7. create same index again (second time) - should be idempotent and succeed
213+
self.create_index(client, collection_name, index_params)
214+
idx_names_after, _ = self.list_indexes(client, collection_name, field_name=default_vector_field_name)
215+
assert len(idx_names_after) == 1
216+
assert idx_names == idx_names_after
217+
# 8. cleanup
218+
self.drop_collection(client, collection_name)
219+
128220

129221
class TestMilvusClientAlterCollection(TestMilvusClientV2Base):
130222
@pytest.mark.tags(CaseLabel.L0)

0 commit comments

Comments
 (0)