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

Fix bug preventing Qdrant points with numeric id from being consumed #1075

Conversation

craigomatic
Copy link
Contributor

Motivation and Context

Qdrant supports both string and int for the id of a ScoredPoint. This minor change allows the serialiser to handle both of these scenarios while retaining string as the type for SK.

Thank you to @AwesomeYuer for the heads up in #794

Description

Added a single attribute decorating the attribute that massages ints to string and leaves string as-is.

Contribution Checklist

…tion (Qdrant allows either string or int for this Id)
@craigomatic craigomatic added PR: ready for review All feedback addressed, ready for reviews memory connector labels May 18, 2023
@craigomatic craigomatic requested a review from tawalke May 18, 2023 20:21
@github-actions github-actions bot added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels May 18, 2023
Copy link
Contributor

@tawalke tawalke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an effective and efficient solution for this!

@craigomatic craigomatic added PR: ready to merge PR has been approved by all reviewers, and is ready to merge. and removed PR: ready for review All feedback addressed, ready for reviews labels May 18, 2023
@lemillermicrosoft lemillermicrosoft merged commit 6a1c4ef into microsoft:main May 18, 2023
18 checks passed
shawncal pushed a commit to johnoliver/semantic-kernel that referenced this pull request May 19, 2023
…icrosoft#1075)

### Motivation and Context
Qdrant supports both string and int for the id of a ScoredPoint. This
minor change allows the serialiser to handle both of these scenarios
while retaining string as the type for SK.

Thank you to @AwesomeYuer for the heads up in microsoft#794 

### Description
Added a single attribute decorating the attribute that massages ints to
string and leaves string as-is.
@AwesomeYuer
Copy link

glad to hear that

@EachShow
Copy link

EachShow commented Jun 2, 2023

when call the function "kernel.Memory.SaveInformationAsync", the ID is assigned to a GUID,
then call the function "IQdrantVectorDbClient.GetVectorByPayloadIdAsync", an error will occur

Starting in .NET 6, if you apply JsonNumberHandlingAttribute to a property that's a collection of non-number values and attempt to serialize or deserialize the property, an InvalidOperationException is thrown.

adrianwyatt pushed a commit that referenced this pull request Jun 2, 2023
### Motivation and Context
This PR fixes a regression introduced in #1075 and includes unit tests
to correctly validate that both string and integer property ids are
supported.

### Description
The changes are based on the code suggested by AwesomeYuer over in #794
- the JsonNumberHandling attribute behaved in an unexpected way and was
removed.
salmon131 pushed a commit to salmon131/semantic-kernel that referenced this pull request Jun 3, 2023
…oft#1313)

### Motivation and Context
This PR fixes a regression introduced in microsoft#1075 and includes unit tests
to correctly validate that both string and integer property ids are
supported.

### Description
The changes are based on the code suggested by AwesomeYuer over in microsoft#794
- the JsonNumberHandling attribute behaved in an unexpected way and was
removed.
@craigomatic craigomatic deleted the qdrant-support-both-int-and-string-ids branch June 15, 2023 23:26
shawncal pushed a commit to shawncal/semantic-kernel that referenced this pull request Jul 6, 2023
…oft#1313)

### Motivation and Context
This PR fixes a regression introduced in microsoft#1075 and includes unit tests
to correctly validate that both string and integer property ids are
supported.

### Description
The changes are based on the code suggested by AwesomeYuer over in microsoft#794
- the JsonNumberHandling attribute behaved in an unexpected way and was
removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel memory connector .NET Issue or Pull requests regarding .NET code PR: ready to merge PR has been approved by all reviewers, and is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants