Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: go-sdk can't use upsert. #33835

Closed
1 task done
Seaiii opened this issue Jun 13, 2024 · 20 comments
Closed
1 task done

[Bug]: go-sdk can't use upsert. #33835

Seaiii opened this issue Jun 13, 2024 · 20 comments
Assignees
Labels
stale indicates no udpates for 30 days triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@Seaiii
Copy link

Seaiii commented Jun 13, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Environment

- Milvus version:
- Deployment mode(standalone or cluster):
- MQ type(rocksmq, pulsar or kafka):    
- SDK version(e.g. pymilvus v2.0.0rc2):
- OS(Ubuntu or CentOS): 
- CPU/Memory: 
- GPU: 
- Others:

Current Behavior

There are three fields in the library collection.
I am also passing three fields using the Upsert method.
But it says "the number of fields is less than needed: invalid parameter[expected=3][actual=4]"

Expected Behavior

No response

Steps To Reproduce

No response

Milvus Log

No response

Anything else?

No response

@Seaiii Seaiii added kind/bug Issues or changes related a bug needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 13, 2024
@yanliang567
Copy link
Contributor

@Seaiii could you please share your collection schema and code snippet of upsert? Also share the versions of milvus and go sdks would be helpful.

/assign @Seaiii
/unassign

@sre-ci-robot sre-ci-robot assigned Seaiii and unassigned yanliang567 Jun 13, 2024
@yanliang567 yanliang567 added triage/needs-information Indicates an issue needs more information in order to work on it. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 13, 2024
@Seaiii
Copy link
Author

Seaiii commented Jun 13, 2024

@Seaiii could you please share your collection schema and code snippet of upsert? Also share the versions of milvus and go sdks would be helpful.

embeddings := entity.NewColumnFloatVector("embedding", 1024, [][]float32{embedding})
contents := entity.NewColumnVarChar("content", []string{action.Content})
id := entity.NewColumnInt64("id", []int64{action.Id})
_, err := MilvusClient.milvus.Upsert(context.Background(), collectionName, "", id, embeddings, contents)

embeddings are vectorised values.
content is the varchar content
id is the self-incremented primary key id

milvus version:2.4.1
milvus-sdk-go/v2 v2.4.0

image

@sunby
Copy link
Contributor

sunby commented Jun 13, 2024

@Seaiii Upsert operations does not support collections with autoID enabled.

@yanliang567 yanliang567 removed the kind/bug Issues or changes related a bug label Jun 13, 2024
@xiaofan-luan
Copy link
Collaborator

thought @smellthemoon is working on it

@Seaiii
Copy link
Author

Seaiii commented Jun 13, 2024

@Seaiii Upsert operations does not support collections with autoID enabled.

Is this one for sure? In that case I still need to maintain a self-incrementing id myself.
But the error message that milvus replied to me was not due to autoID

@smellthemoon
Copy link
Contributor

thought @smellthemoon is working on it

pr has hang for a long time. related with #30342, reviewed by @czs007 . I will fix the conflict after review.

@smellthemoon
Copy link
Contributor

Is this one for sure? In that case I still need to maintain a self-incrementing id myself.
But the error message that milvus replied to me was not due to autoID

yes, I will make the error msg more clear.
/assign

@Seaiii
Copy link
Author

Seaiii commented Jun 14, 2024

autoID

Ok, so when your PR is merged, autoID will be able to use upsert too right?

@yanliang567
Copy link
Contributor

autoID

Ok, so when your PR is merged, autoID will be able to use upsert too right?

As you already set autoid for primary key, could you please share more info about why do you still need upsert to keep the same primary key? The pr will not be merged until it is determined that upsert for autoid is really meaningful to user scenarios. @Seaiii

@Seaiii
Copy link
Author

Seaiii commented Jun 14, 2024

autoID

We need the primary key id to self-increment to determine uniqueness. Also we need to update each piece of information. That's all.
If we don't set the primary key id to increment, we need to maintain our own

@yanliang567
Copy link
Contributor

autoID

We need the primary key id to self-increment to determine uniqueness. Also we need to update each piece of information. That's all. If we don't set the primary key id to increment, we need to maintain our own

so in your case to keep the order of entities, you want the same primary key after doing upsert?is it acceptable for you if we set a new primary key(a new incremented or a bigger id) for the updated entity after doing upsert?

@Seaiii
Copy link
Author

Seaiii commented Jun 14, 2024

so in your case to keep the order of entities, you want the same primary key after doing upsert?is it acceptable for you if we set a new primary key(a new incremented or a bigger id) for the updated entity after doing upsert?

If a new primary key is added cutting larger values is not necessary.
This is because without the upsert interface, we would delete and then create for "updates". It's also a bigger primary key, and the sorting will be messed up.
The effect we want to see is that the primary key remains the same after the update.

@yanliang567
Copy link
Contributor

okay, thank you for your feedbacks. We will rethink our design of support for upsert with autoid.
I'd close this issue as the following up support will be trakced in pr above.

@xiaofan-luan
Copy link
Collaborator

action.Id

how did you know action.Id? did you store it some where else?

@Seaiii
Copy link
Author

Seaiii commented Jun 14, 2024

how did you know action.Id? did you store it some where else?

Get all the action.Id for display via the query method.
If I want to modify any of them, I do know what this action.Id is

Copy link

stale bot commented Jul 14, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label Jul 14, 2024
@xiaofan-luan
Copy link
Collaborator

@congqixia
is this still issue?

@stale stale bot removed the stale indicates no udpates for 30 days label Jul 16, 2024
Copy link

stale bot commented Aug 18, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label Aug 18, 2024
@yanliang567
Copy link
Contributor

@congqixia @ThreadDao was this fixed

@stale stale bot removed the stale indicates no udpates for 30 days label Aug 19, 2024
Copy link

stale bot commented Sep 20, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label Sep 20, 2024
@stale stale bot closed this as completed Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale indicates no udpates for 30 days triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
Development

No branches or pull requests

5 participants