Feat/support for redis 8 vector sets #3221
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
Signed-off-by: annemayor <[email protected]>
Signed-off-by: annemayor <[email protected]>
AnneMayor
force-pushed
the
feat/support-for-redis-8-vector-sets
branch
from
a6f4df4 to
c6f326a
Compare
mp911de
added
type: enhancement
|
@mp911de I am going to go on the next step. Pleaes, let me know if I have to switch my PR direction. |
| * @param options the options for the command | ||
| * @return true if the element was added, false if it already existed | ||
| */ | ||
| Boolean vAdd(byte @NonNull [] key, byte @NonNull [] values, byte @NonNull [] element, VAddOptions options); |
There was a problem hiding this comment.
values should be named vector to indicate better what this API method is about. VAddOptions can be explicitly @Nullable
| * @param options the options for the command | ||
| * @return true if the element was added, false if it already existed | ||
| */ | ||
| Boolean vAdd(byte @NonNull [] key, double @NonNull [] values, byte @NonNull [] element, VAddOptions options); |
There was a problem hiding this comment.
Please rename values to vector. We do have a specific Vector type in Spring Data Commons and it would make sense to use that one as well as separate method. In fact, this vAdd method could be turned into a default method using Vector.unsafe(double[]) and delegate to the method accepting Vector.
| this.maxConnections = builder.maxConnections; | ||
| } | ||
|
|
||
| public static Builder builder() { |
There was a problem hiding this comment.
We typically don't use builders (see XAddOptions).
| return target; | ||
| } | ||
|
|
||
| private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); |
There was a problem hiding this comment.
Let's not introduce a mandatory dependency on Jackson.
| 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.
Lettuce 6.7 has support for Vector sets.
There was a problem hiding this comment.
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.
Thanks for the feedback! I’ll double-check and update the PR if needed.
| private final boolean cas; | ||
| private final QuantizationType quantization; | ||
| private final @Nullable Integer efBuildFactor; | ||
| private final @Nullable Map<String, Object> attributes; |
There was a problem hiding this comment.
Let's keep attributes opaque and make it a String without us being involved in JSON serialization for the time being.
Thank you for your comments. I am gonna address your comments. |
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