-
Notifications
You must be signed in to change notification settings - Fork 630
Add support for vector set commands #1673
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?
Conversation
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 still need to go over most of the PR, but moving serde to a non-optional dependency is a no-go. This will have to be made into an optional feature - preferably, the same feature as the JSON module, since I'd like to cut down on the number of features in the crate.
I suggest targetting the 1.0.x branch, and renaming the JSON feature to account for all extra functionality.
redis/Cargo.toml
Outdated
serde = { version = "1.0.219", optional = true } | ||
serde_json = { version = "1.0.140", optional = true } | ||
# Needed for RedisJSON Support and vector sets | ||
serde = { version = "1.0.219" } |
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.
please keep those optional. Making these non-optional significantly increases compilation times.
Thank you for the feedback! Would it make sense to keep the JSON feature separate so it can still be enabled on its own, without pulling in the other extras? I’m happy to consolidate if that’s the preferred direction — just wanted to check in case keeping them separate would offer more flexibility for users. The target branch will be updated once the other proposed changes are implemented. |
It makes sense to introduce this as a separate feature, but this crate uses flag-frenzy to check for potentially problematic feature combinations, which means that introducing new features makes the CI run slower, so I try to avoid that. |
The plan is to implement new Redis 8 features, but based on my understanding, none of the planned additions are expected to introduce dependencies on |
19d0c4c
to
77fe7e8
Compare
@nihohit The vector sets feature is now optional. However, if this isn’t quite the approach you had in mind, please feel free to let me know. |
- Added support for all Redis vector set commands: - VADD, VCARD, VDIM, VEMB, VGETATTR, VINFO, VLINKS, VRANDMEMBER, VREM, VSETATTR, VSIM - Developed unit tests to verify the correct behavior of each command - Ensured compliance with Redis vector set specifications
These changes improve code clarity and maintainability by eliminating redundant generic type specifications where they are unnecessary.
77fe7e8
to
7ca5847
Compare
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.
commented on the new functions. Still need to review types and tests.
@@ -91,7 +91,7 @@ rustls-native-certs = { version = "0.8", optional = true } | |||
tokio-rustls = { version = "0.26", optional = true, default-features = false } | |||
futures-rustls = { version = "0.26", optional = true, default-features = false } | |||
|
|||
# Only needed for RedisJSON Support | |||
# Needed for RedisJSON Support |
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.
Needed for modules support
@@ -118,6 +118,7 @@ default = ["acl", "streams", "geospatial", "script", "keep-alive", "num-bigint"] | |||
acl = [] | |||
geospatial = [] | |||
json = ["dep:serde", "serde/derive", "dep:serde_json"] | |||
vector-sets = ["json"] |
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.
why not "dep:serde", "serde/derive", "dep:serde_json"
?
/// [Redis Docs](https://redis.io/commands/VADD) | ||
#[cfg(feature = "vector-sets")] | ||
#[cfg_attr(docsrs, doc(cfg(feature = "vector-sets")))] | ||
fn vadd_extended<K: ToRedisArgs, E: ToRedisArgs>(key: K, input: VectorAddInput<'a>, element: E, options: &'a VAddOptions) -> (bool) { |
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.
please be consistent with the naming scheme - we use _options
, not _extended
.
/// [Redis Docs](https://redis.io/commands/VEMB) | ||
#[cfg(feature = "vector-sets")] | ||
#[cfg_attr(docsrs, doc(cfg(feature = "vector-sets")))] | ||
fn vemb_raw<K: ToRedisArgs, E: ToRedisArgs>(key: K, element: E) -> Generic { |
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.
it's preferable to use an option type here, in case that more values will be added to the command in the future.
#[cfg_attr(docsrs, doc(cfg(feature = "vector-sets")))] | ||
fn vsetattr<K: ToRedisArgs, E: ToRedisArgs, J: Serialize>(key: K, element: E, json_object: &'a J) -> (bool) { | ||
let attributes_json = match serde_json::to_value(json_object) { | ||
Ok(serde_json::Value::String(s)) if s.is_empty() => "".to_string(), |
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.
what happens on the Err
case? why not fail then?
Also, why bother with the if s.is_empty()
check? what happens if it is removed?
/// [Redis Docs](https://redis.io/commands/VINFO) | ||
#[cfg(feature = "vector-sets")] | ||
#[cfg_attr(docsrs, doc(cfg(feature = "vector-sets")))] | ||
fn vinfo<K: ToRedisArgs>(key: K) -> Generic { |
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.
doesn't this return a HashMap<String, Value>
?
/// [Redis Docs](https://redis.io/commands/VRANDMEMBER) | ||
#[cfg(feature = "vector-sets")] | ||
#[cfg_attr(docsrs, doc(cfg(feature = "vector-sets")))] | ||
fn vrandmember<K: ToRedisArgs>(key: K) -> (Option<Vec<String>>) { |
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.
Why return a vec? and why have both this and vrandmember_multiple?
/// [Redis Docs](https://redis.io/commands/VSIM) | ||
#[cfg(feature = "vector-sets")] | ||
#[cfg_attr(docsrs, doc(cfg(feature = "vector-sets")))] | ||
fn vsim_extended<K: ToRedisArgs>(key: K, input: VectorSimilaritySearchInput<'a>, options: &'a VSimOptions) -> Generic { |
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.
again, _options
.
#[cfg(feature = "vector-sets")] | ||
#[cfg_attr(docsrs, doc(cfg(feature = "vector-sets")))] | ||
#[derive(Clone, Copy)] | ||
pub enum VectorQuantization { |
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.
why are these here and not in commands/mod.rs
?
#[cfg(feature = "vector-sets")] | ||
#[cfg_attr(docsrs, doc(cfg(feature = "vector-sets")))] | ||
#[derive(Clone)] | ||
pub enum EmbeddingInput<'a> { |
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.
same for all types below
Add Support for Redis Vector Set Commands
This Pull Request implements the full suite of Redis Vector Set commands as defined in the Redis documentation, along with unit tests to verify correctness and expected behavior.
The following commands have been implemented:
VADD
– Add a new element to a vector set.VCARD
– Return the number of elements in a vector set.VDIM
– Return the dimensionality of a vector set.VEMB
– Return the approximate vector associated with a given element in the vector set.VGETATTR
– Retrieve attribute values associated with an element of a vector set.VINFO
– Return information about a vector set.VLINKS
– Return the neighbors of a specified element in a vector set.VRANDMEMBER
– Return one or more random elements from a vector set.VREM
– Remove an element from a vector set.VSETATTR
– Associate a JSON object with an element in a vector set.VSIM
– Return elements similar to a given vector or element.Command aliases:
VDELATTR
- A utility command that can be used to remove the associated attributes of an element in a vector set. It is an alias forVSETATTR
with an empty string.🧪 Tests
Each command is covered by unit tests that verify:
The tests serve both as validation and usage examples for future contributors.