-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Feat/support for redis 8 vector sets #3221
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
base: main
Are you sure you want to change the base?
Feat/support for redis 8 vector sets #3221
Conversation
a6f4df4 to
c6f326a
Compare
|
@mp911de I am going to go on the next step. Pleaes, let me know if I have to switch my PR direction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went over the PR initially and left a few comments. The general direction looks neat, we should also have a reactive implementation and we have to make up our mind how we want to surface Vector sets from the Template API.
src/main/java/org/springframework/data/redis/connection/RedisVectorSetCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/org/springframework/data/redis/connection/RedisVectorSetCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/org/springframework/data/redis/connection/RedisVectorSetCommands.java
Outdated
Show resolved
Hide resolved
src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java
Outdated
Show resolved
Hide resolved
| Assert.notNull(values, "Values must not be null"); | ||
| Assert.notNull(element, "Element must not be null"); | ||
|
|
||
| // TODO: Implement when Lettuce adds native support for V.ADD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lettuce 6.7 has support for Vector sets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the API surface in 6.7 is still in experimental state.
There are some pending changes that need to be addressed before we can promote it.
See redis/lettuce#3459
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I’ll double-check and update the PR if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirmed that lettuce has still been in experimental progress in terms of Vector API. I think we should wait until it is addressed completely. Any idea?
src/main/java/org/springframework/data/redis/connection/RedisVectorSetCommands.java
Outdated
Show resolved
Hide resolved
Thank you for your comments. I am gonna address your comments. |
|
Sorry for the late response. I plan to update and re-request the PR review by this weekend. Thanks for your patience. |
c6f326a to
8438e4c
Compare
Signed-off-by: annemayor <[email protected]>
Signed-off-by: annemayor <[email protected]>
Signed-off-by: annemayor <[email protected]>
8438e4c to
c1b72e5
Compare
|
I think it is better to open new PR for support other vector APIs to make PR review more easier and comfortable. What do you guys think? |
This PR implements a partial feature to validate the code conventions and assess the impact scope. If this approach aligns with the project's direction, I will proceed with implementing the remaining functionality.
Please review and let me know if this is the right approach.
AS-IS
no support for redis 8 vector set
TO-BE
add feature vAdd of redis 8 vector set for Jedis
partially resolved #3153
Next Step