-
Notifications
You must be signed in to change notification settings - Fork 2k
Refactor QdrantObjectFactory to allow null metadata values #3483
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
Conversation
|
@WOONBE Thanks for the PR! Could you add test(s) to validate the fix please? |
@ilayaperumalg Regarding the unit test for this change, I couldn't find an existing test class for QdrantObjectFactory where this new test case would naturally fit. |
|
@WOONBE Please feel free to add a new test class for the unit test. You could also add a test case in our ITs. Thanks again for looking into this! |
@ilayaperumalg Thank you for the guidance! I've pushed a new commit that adds a test class with a unit test to cover this fix. Please let me know if there are any improvements or if the format needs any adjustments. I'd be happy to make further changes. |
Signed-off-by: WOONBE <[email protected]>
Signed-off-by: WOONBE <[email protected]>
Signed-off-by: WOONBE <[email protected]>
Signed-off-by: WOONBE <[email protected]>
|
@ilayaperumalg My apologies, it seems I missed running the code formatter before submitting the initial PR. I saw the "Formatting violations" error in the PR checks for the test file I added. I have now run mvn spring-javaformat:apply and pushed the updated code to resolve this. Aside from this, please let me know if there's anything else I might have missed or any other feedback you have. |
|
@WOONBE Thank you once again for the PR and following it up with the test update. |
related issue : #3420
When retrieving documents from a Qdrant vector store, a NullPointerException was being thrown if the metadata payload for any document contained a field with a null value.
I fix the issue by replacing the stream-based Collectors.toMap() implementation with a more robust and explicit for-each loop.
I have confirmed the fix by performing tests in my local environment.