Added Query iterator method #80

BlackGad · 2024-04-19T14:43:08Z

Added Query iterator implementation that behave in same manner as python Query operator.

It would be pleasure to see this PR merged and released.

BlackGad · 2024-04-22T12:53:59Z

Any updates on this?

roji · 2024-04-22T13:46:16Z

@BlackGad will get to it soon, hang tight.

BlackGad · 2024-06-07T12:37:54Z

so? any progress so far?

roji · 2024-06-07T12:58:51Z

@BlackGad apologies, I'll try to set aside a bit of time next week for this SDK.

BlackGad · 2024-06-20T06:32:09Z

so?)

BlackGad · 2024-07-05T06:47:50Z

I understand that this is just a side project for you @roji, but could you consider approving the request? This missing functionality is crucial for us to dump data into our internal cold storage, and it's quite cumbersome to maintain an internal fork. Additionally, I believe the community would benefit from having this helpful functionality in C#.

weianweigan · 2024-07-08T03:04:43Z

@BlackGad Thanks! I'll review and approve this PR. Perhaps @roji is currently busy.

roji

Sorry for not having had time to look at this earlier - but sometimes it can take a while as other things take priority.

I know this was already merged, but I can't say I'm very happy about it given that the PR has zero tests for the new API. I'd be against releasing a new version in this state.

Also see below for some comments which I'd like to see us fix before releasing.

Finally, can you please point to the corresponding Python code which I assume this is based on?

roji · 2024-07-10T10:50:59Z

Milvus.Client/MilvusCollection.Entity.cs

+            object? pkLastValue;
+            int processedDuringIterationCount;
+            var pkFieldsData = response.FieldsData.Single(x => x.FieldId == pkField.FieldID);
+            if (pkField.DataType == DataType.VarChar)


Use a switch here, with a default case that throws, in case Milvus add support for a 3rd PK type.

It is direct port of logic from python client

https://github.com/milvus-io/pymilvus/blob/6625af7fd0bfdf75fd596e7a9a871528cb539ef7/pymilvus/orm/iterator.py#L186

But ok, will add

In general, I don't think it makes sense to keep code a certain way just because the python implementation looks a certain way. This is a different project in a different language, and we should be doing things in the best way possible here regardless of what the Python driver looks like.

roji · 2024-07-10T10:54:07Z

Milvus.Client/MilvusCollection.Entity.cs

+            new Grpc.KeyValuePair {Key = Constants.Iterator, Value = "True"},
+            new Grpc.KeyValuePair {Key = Constants.ReduceStopForBest, Value = "True"},
+            new Grpc.KeyValuePair {Key = Constants.BatchSize, Value = batchSize.ToString(CultureInfo.InvariantCulture)},
+            new Grpc.KeyValuePair {Key = Constants.Offset, Value = 0.ToString(CultureInfo.InvariantCulture)},


nit: why not just "0"? Are you sure we even need to set Offset at all?

Same logic port here from python client

https://github.com/milvus-io/pymilvus/blob/6625af7fd0bfdf75fd596e7a9a871528cb539ef7/pymilvus/orm/iterator.py#L433

The line you refer to doesn't seem to have anything to do with this - it just checks that offset is zero; whereas here I'm asking why not just "0" as simpler code.

oh do you mean use constant?

roji · 2024-07-10T10:55:23Z

Milvus.Client/MilvusCollection.Entity.cs

+            new Grpc.KeyValuePair {Key = Constants.Limit, Value = Math.Min(batchSize, userLimit).ToString(CultureInfo.InvariantCulture)});
+
+        var processedItemsCount = 0;
+        while (!cancellationToken.IsCancellationRequested)


The cancellation token should be checked internally in InvokeAsync which is called just below - is there any reason for the additional check here? If not, it should be removed.

Because then it will be while(true) so it is defensive strategy so I will be sure that loop will broke.

I prefer to leave this. What do you think?

Adding the check doesn't ensure that the loop will be broken any more than a simple while (true) (i.e. the cancellation token mat never get triggered). The thing that guarantees that the loop will break is the yield break inside, when there are no more results. And since the cancellation token is checked within InvokeAsync anyway, I think this should be removed.

roji · 2024-07-10T10:59:12Z

Milvus.Client/MilvusCollection.Entity.cs

+        QueryParameters? parameters = null,
+        [EnumeratorCancellation] CancellationToken cancellationToken = default)
+    {
+        if ((parameters?.Offset ?? 0) != 0)


Any specific reason not to support Offset? It could simply be set on the very first call, and from that point on just continue with the batching as usual?

Same python logic port. From my tests milvus behave strange when you try to setup offset.

https://github.com/milvus-io/pymilvus/blob/6625af7fd0bfdf75fd596e7a9a871528cb539ef7/pymilvus/orm/iterator.py#L435

I'd prefer to understand better if this is an actual Milvus limitation or just an unimplemented feature at the client.

I think clients should behave the same way. I don't have time to look into it right now, but I saw some strange results when I changed the offset to a non-zero value. I need this code merged into the master branch so I can use it and not rely on a fork. Maybe we should delay this proof of concept to future releases for those who really need it?

Milvus.Client/MilvusCollection.Entity.cs

roji · 2024-07-10T11:05:51Z

Milvus.Client/MilvusCollection.Entity.cs

+            new Grpc.KeyValuePair {Key = Constants.ReduceStopForBest, Value = "True"},
+            new Grpc.KeyValuePair {Key = Constants.BatchSize, Value = batchSize.ToString(CultureInfo.InvariantCulture)},
+            new Grpc.KeyValuePair {Key = Constants.Offset, Value = 0.ToString(CultureInfo.InvariantCulture)},
+            new Grpc.KeyValuePair {Key = Constants.Limit, Value = Math.Min(batchSize, userLimit).ToString(CultureInfo.InvariantCulture)});


I'm a bit confused about Limit vs. BatchSize here... If Limit is bigger than BatchSize, will have the same value (because of the Min here). If Limit is smaller than BatchSize, BatchSize isn't reduced... What's the logic here supposed to be?

Limit and batch size must be equal here. Will fix

roji · 2024-07-10T11:08:03Z

Milvus.Client/MilvusCollection.Entity.cs

+                // If user expression is not null, we should use it
+                {userExpression: not null} => userExpression,
+                // If user expression is null and pk field is string
+                {pkField.DataType: DataType.VarChar} => $"{pkField.Name} != ''",


What is this non-empty string check - and the below < {long.MaxValue} check needed? Why not just leave Expr empty if the user didn't specify an expression?

@BlackGad what about this?

It is to guaranty order on first request when user expression is absent. Again it is port from python :)

BlackGad · 2024-07-13T07:56:57Z

Finally, can you please point to the corresponding Python code which I assume this is based on?

Here it is https://github.com/milvus-io/pymilvus/blob/6625af7fd0bfdf75fd596e7a9a871528cb539ef7/pymilvus/orm/iterator.py

BlackGad added 2 commits April 19, 2024 17:36

Added Query iterator method

3c8a33b

Added unexpected case handling

f5613dc

This was referenced Apr 19, 2024

Support array fields #77

Closed

Support iterator-based search #79

Open

weianweigan merged commit 30199ee into milvus-io:main Jul 10, 2024
2 checks passed

roji reviewed Jul 10, 2024

View reviewed changes

This was referenced Jul 13, 2024

Added Query iterator method Continuation #88

Closed

Added Query iterator method Continuation #89

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Query iterator method #80

Added Query iterator method #80

BlackGad commented Apr 19, 2024

BlackGad commented Apr 22, 2024

roji commented Apr 22, 2024

BlackGad commented Jun 7, 2024

roji commented Jun 7, 2024

BlackGad commented Jun 20, 2024

BlackGad commented Jul 5, 2024

weianweigan commented Jul 8, 2024

roji left a comment

roji Jul 10, 2024

BlackGad Jul 13, 2024

roji Jul 13, 2024

BlackGad Jul 13, 2024

roji Jul 10, 2024

BlackGad Jul 13, 2024

roji Jul 13, 2024

BlackGad Jul 13, 2024

BlackGad Jul 13, 2024

roji Jul 10, 2024

BlackGad Jul 13, 2024

roji Jul 13, 2024 •

edited

Loading

BlackGad Jul 13, 2024

roji Jul 10, 2024

BlackGad Jul 13, 2024

roji Jul 13, 2024

BlackGad Jul 13, 2024

roji Jul 10, 2024

BlackGad Jul 13, 2024

roji Jul 10, 2024

roji Jul 13, 2024 •

edited

Loading

BlackGad Jul 13, 2024

BlackGad commented Jul 13, 2024 •

edited

Loading

Added Query iterator method #80

Added Query iterator method #80

Conversation

BlackGad commented Apr 19, 2024

BlackGad commented Apr 22, 2024

roji commented Apr 22, 2024

BlackGad commented Jun 7, 2024

roji commented Jun 7, 2024

BlackGad commented Jun 20, 2024

BlackGad commented Jul 5, 2024

weianweigan commented Jul 8, 2024

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Jul 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Jul 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlackGad commented Jul 13, 2024 • edited Loading

roji Jul 13, 2024 •

edited

Loading

roji Jul 13, 2024 •

edited

Loading

BlackGad commented Jul 13, 2024 •

edited

Loading