Iterators and range_filter error (when no range filter is specified)

PiercarloSlavazza commented 4 months ago

I started to use the search iterators.

I am using:

milvus sdk java: 2.4.0
milvus: 2.3.3 and v2.4.0-rc.1

I create the SearchIteratorParam in the following way:

SearchIteratorParam.newBuilder()
                .withCollectionName(metadata.getCollectionName())
                .withMetricType(MetricType.IP)
                .withOutFields(Arrays.asList(
                        METADATA_FIELD_ID_COURT.getKeyId(),
                        METADATA_FIELD_ID_PDD_SETTING_JUDGEMENTS_YEARS.getKeyId(),
                        METADATA_FIELD_ID_SENTENCE_UUID.getKeyId(),
                        METADATA_FIELD_ID_TAGS.getKeyId(),
                        METADATA_FIELD_ID_TEXT_TYPE.getKeyId(),
                        METADATA_FIELD_ID_TEXT_UUID_PK.getKeyId(),
                        METADATA_FIELD_ID_YEAR.getKeyId()
                ))
                .withFloatVectors(singletonList(embeddings.getVector().stream().map(Double::floatValue).toList()))
                .withBatchSize(500L)
                .withVectorFieldName(METADATA_FIELD_ID_EMBEDDINGS.getKeyId())

and afterwards I experience the following weird behaviour:

on machine n. 1 (macOS), with Milvus 2.3.3 ⇒ everything works fine
on machine n. 2 (Ubuntu) with Milvus v2.4.0-rc.1, I get the following error: io.milvus.v2.exception.MilvusClientException: range_filter must be less than radius for L2/HAMMING/JACCARD, range_filter:0.853851, radius:0.809344: invalid parameter

This is very strange because:

I used IP
I do not set any radius/rangefilter parameter - so I suppose that they come from some weird defaults
even if I switch on machine n. 2 to Milvus 2.3.3, I still get the error (maybe defaults are written somewhere in the index/database?)

Moreover, if I try to set radius/rangefilter so that the defaults do not kick in, such as in

                .withParams("""
                        {
                            "radius": 1.0,
                            "range_filter" : 1.0
                        }""")

on machine n. 1 (milvus 2.3.3) I get the error: lass java.lang.Double cannot be cast to class java.lang.Float (java.lang.Double and java.lang.Float are in module java.base of loader 'bootstrap')
on machine n. 2 (milvus v2.4.0-rc.1), nothing changes, I still get "range_filter must be less than radius"

I have only found this potentially related issue: https://github.com/milvus-io/milvus/issues/29305

Any hint?

Thank you.

PiercarloSlavazza commented 4 months ago

Tried Milvus 2.4.0 as well - still getting the error.

PiercarloSlavazza commented 4 months ago

Just in case someone will experience this very same issue, I managed to solve it by:

creating a brand new instance of Milvus v 2.3.3
connecting via milvus sdk v2.4.0

This wasy the iterators are working fine.

I assume that sdk 2.4.0 iterators have been tested with Milvus 2.4.0 - so I think that it is something on my side, but I wasn't able to track it down (apart going back to Milvus 2.3.3)…

yhmo commented 4 months ago

A valid range search param for L2 metric should be:

String params = "{\"radius\": 1.0, \"range_filter\": 0.5}";
SearchIteratorParam.Builder searchIteratorParamBuilder = SearchIteratorParam.newBuilder()
                ......
                .withParams(params)
                .withMetricType(MetricType.L2);

With L2 metric type, the value of range_filter should be less than radius.
With IP metric type, the value of radius should be less than range_filter.

range_search drawio

PiercarloSlavazza commented 4 months ago

Thanks for your clarification, but my search does not use L2 - it actually uses .withMetricType(MetricType.IP) (see 1st comment).

yhmo commented 4 months ago

But you set "radius"=1.0 and "range_filter"=1.0, the two parameters cannot be equal, neither with IP nor L2.

With IP metric type, the value of radius should be less than range_filter.

String params = "{\"radius\": 0.5, \"range_filter\": 0.8}";
SearchIteratorParam.Builder searchIteratorParamBuilder = SearchIteratorParam.newBuilder()
                ......
                .withParams(params)
                .withMetricType(MetricType.IP);

PiercarloSlavazza commented 4 months ago

I see - thank you for pointing it.

However:

I resorted to setting the params only later on, in order to try to circumvent the issue that occurred in first place: I will try again following your suggestion
so, please consider that the problem occurred in the first place when I tried to execute the search without having specified any parameter: it alwasy worked, I works with Milvus 2.3.3, it doesn't with Milvus 2.4.* - maybe it is by design, are able to confirm it?
lastly, even if it is everything by design, the error message is at least misleading: I used IP, and the error message I got was referring to L2

xiaofan-luan commented 4 months ago

I thought it is because you don't specify metrics when you create index. and by default it is L2. ignore the metrics when search, it only helps on verify the build index metrics

PiercarloSlavazza commented 4 months ago

Guys, I finally found the bugs.

There are - according to my investigations - two (unrelated) bugs:

Bug n.1

In statement that starts at this line: https://github.com/milvus-io/milvus-sdk-java/blob/bf981d037c487aadc35b2adeb072485331d63dd4/src/main/java/io/milvus/orm/iterator/SearchIterator.java#L221 the metric type is NOT initialized using the searchIteratorParam.getMetricType().

This affects ONLY the iterator.next calls subsequent to the first one: the SDK computes correctly the new params, BUT, given that the metric type is not re-initialized AND I am using IP which is NOT the default, the server thinks that the intended metric is L2 (the default), and therefore it complains that the range and radius paramters are inverted - which indeed are NOT, but only if the correct metric type is reported to the server.

Bug n. 2

This bug is actually unrelated to bug n.1 - it just happened to occur while I was trying hard to find a workaround to bug n. 1 (well, before I understood the very nature of that first issue, of course).

In this line: https://github.com/milvus-io/milvus-sdk-java/blob/bf981d037c487aadc35b2adeb072485331d63dd4/src/main/java/io/milvus/orm/iterator/SearchIterator.java#L143 the result of params.get(RADIUS) is cast to a Float - but this leads to a casting error because params.get(RADIUS) is read as a Double, which of course cannot be just cast to a Float. BTW the parameter is read as a Double because this is parsed via JacksonUtils.fromJson(searchIteratorParam.getParams(), new TypeReference<Map<String, Object>>(){}), and Jackson wil always read a real number as a Double in this case, and this behaviour is not - to my knowledge - configurable.

So, if you confirm my findings, I will report the Bug. n. 2 in another issue.

I would like to highlight that bug n. 1 is blocking for whoever wants to use iterators with IP metric in Java.

xiaofan-luan commented 4 months ago

Guys, I finally found the bugs.

There are - according to my investigations - two (unrelated) bugs:

Bug n.1

In statement that starts at this line:

https://github.com/milvus-io/milvus-sdk-java/blob/bf981d037c487aadc35b2adeb072485331d63dd4/src/main/java/io/milvus/orm/iterator/SearchIterator.java#L221

the metric type is NOT initialized using the searchIteratorParam.getMetricType(). This affects ONLY the iterator.next calls subsequent to the first one: the SDK computes correctly the new params, BUT, given that the metric type is not re-initialized AND I am using IP which is NOT the default, the server thinks that the intended metric is L2 (the default), and therefore it complains that the range and radius paramters are inverted - which indeed are NOT, but only if the correct metric type is reported to the server.

Bug n. 2

This bug is actually unrelated to bug n.1 - it just happened to occur while I was trying hard to find a workaround to bug n. 1 (well, before I understood the very nature of that first issue, of course).

In this line:

https://github.com/milvus-io/milvus-sdk-java/blob/bf981d037c487aadc35b2adeb072485331d63dd4/src/main/java/io/milvus/orm/iterator/SearchIterator.java#L143

the result of params.get(RADIUS) is cast to a Float - but this leads to a casting error because params.get(RADIUS) is read as a Double, which of course cannot be just cast to a Float. BTW the parameter is read as a Double because this is parsed via JacksonUtils.fromJson(searchIteratorParam.getParams(), new TypeReference<Map<String, Object>>(){}), and Jackson wil always read a real number as a Double in this case, and this behaviour is not - to my knowledge - configurable. So, if you confirm my findings, I will report the Bug. n. 2 in another issue.

I would like to highlight that bug n. 1 is blocking for whoever wants to use iterators with IP metric in Java.

Thanks for reporting the bugs. We will fix bug 1 as soon as possible and maybe you want to take bug 2? Questions: what's the reason of cast could causing a problem?

PiercarloSlavazza commented 4 months ago

Hi @xiaofan-luan ,

We will fix bug 1 as soon as possible

Actually I already attached a PR fixing bug n. 1 - the fix was trivial, but I am a bit in the rush and wasn't even able to run the unit tests, let alone adding a new one… so I would like to ask if someone could please add the related unit test, thank you.

and maybe you want to take bug 2?

I will do the fix - ditto for the unit tests.

what's the reason of cast could causing a problem?

I am not sure I am getting your point… reason of what, exactly? The cast from Double to Float is forbidden by the VM, if done as it has been done in the code base (the root of all evil is of course in first place having used a Map of Objects… imho choices like this nullify the whole point of having a type system, and in the end lead to "undetectable" bugs like this one) - the way to go is Double.floatValue(), so in this case ((Double) params.get(RADIUS)).floatValue().

xiaofan-luan commented 4 months ago

pr looks good to me, please fix the DCO so I can merge the PR, thanks for the contribution.

yhmo commented 4 months ago

I just tried the IteratorExample.java, I got the exception about the cast:

Exception in thread "main" java.lang.RuntimeException: class java.lang.Double cannot be cast to class java.lang.Float (java.lang.Double and java.lang.Float are in module java.base of loader 'bootstrap')
    at io.milvus.CommonUtils.handleResponseStatus(CommonUtils.java:34)
    at io.milvus.IteratorExample.getSearchIterator(IteratorExample.java:308)

I agree the executeNextSearch() should pass the metricType to SearchParam, it is a bug. Wait @lentitude2tk to condirm.

xiaofan-luan commented 4 months ago

Hi @xiaofan-luan ,

We will fix bug 1 as soon as possible

Actually I already attached a PR fixing bug n. 1 - the fix was trivial, but I am a bit in the rush and wasn't even able to run the unit tests, let alone adding a new one… so I would like to ask if someone could please add the related unit test, thank you.

and maybe you want to take bug 2?

I will do the fix - ditto for the unit tests.

what's the reason of cast could causing a problem?

I am not sure I am getting your point… reason of what, exactly? The cast from Double to Float is forbidden by the VM, if done as it has been done in the code base (the root of all evil is of course in first place having used a Map of Objects… imho choices like this nullify the whole point of having a type system, and in the end lead to "undetectable" bugs like this one) - the way to go is Double.floatValue(), so in this case ((Double) params.get(RADIUS)).floatValue().

Thanks for the clarification . I think the reason is not cast double to int, but to convert a object to primitive directly. make sense to me

PiercarloSlavazza commented 4 months ago

I think the reason is not cast double to int, but to convert a object to primitive directly. make sense to me

You are wrong - look at this code from JShell - no primitives here, and the error message is very clear:

jshell> (Float) new Double(0.1)
|  Warning:
|  Double(double) in java.lang.Double has been deprecated and marked for removal
|  (Float) new Double(0.1)
|          ^-------------^
|  Error:
|  incompatible types: java.lang.Double cannot be converted to java.lang.Float
|  (Float) new Double(0.1)
|          ^-------------^

yhmo commented 4 months ago

@PiercarloSlavazza Just confirm, the metricType for search() has been deprecated from v2.3 on the server side. Metric type is determined by the CreateIndexParam when you call createIndex(). So, we can pass None to SearchParam, just let the server get the metric type from index.

So, if you want to use MetricType.IP, you can specify it to the index:

R<RpcStatus> response = milvusClient.createIndex(CreateIndexParam.newBuilder()
                .withCollectionName(COLLECTION_NAME)
                .withFieldName(VECTOR_FIELD)
                .withIndexName(INDEX_NAME)
                .withIndexType(INDEX_TYPE)
                .withMetricType(MetricType.IP)
                .withExtraParam(INDEX_PARAM)
                .withSyncMode(Boolean.TRUE)
                .build());

And we don't need to change the code of SearchIterator.

PiercarloSlavazza commented 4 months ago

Hi,

added signoff to the PR
added patch for the cast bug (tracked in in its own issue #887 )
@yhmo I already have the call to .withMetricType(MetricType.IP) in my index initialization - so the patches are actually needed

yhmo commented 4 months ago

@PiercarloSlavazza The pr has been merged. Could you cherry-pick it to the 2.3 branch and master branch?

PiercarloSlavazza commented 4 months ago

@yhmo

done for master
2.3 has issues: cherry picking my patch (and the patch that fixes the casting issue) cause a conflict - but I am not able to resolve it with confidence because it's about code that I do not know what is doing…

yhmo commented 4 months ago

The code logic is a bit different between 2.3 and 2.4 and cause the conflict. I have merged the pr to 2.3 branch.

milvus-io / milvus-sdk-java