-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(digest): Digest wrappers #102
Open
hahnandrew
wants to merge
12
commits into
valkey-io:main
Choose a base branch
from
hahnandrew:feat/module-digest
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
2e0dbcc
feat(digest): Added ValkeyModule_GetKeyNameFromDigest and ValkeyModul…
hahnandrew 2e4aa8e
Added wrappers for following operations:
hahnandrew 02dc383
Changed `get_key_name` return type and updated syntax for digest methods
hahnandrew 7d31c52
(test): Added digest callback implementation
hahnandrew ca41b3a
(tests): Add alloc module tests
hahnandrew 46a1540
tests: changed test name for clarity
hahnandrew 3b8a083
tests: Added DEBUG digest test
hahnandrew c76de04
test: Add DIGEST-VALUE cases
hahnandrew acc22c8
Test suite docs update
hahnandrew a9d6755
Merge branch 'main' into feat/module-digest
hahnandrew cf4f655
Merge branch 'valkey-io:main' into feat/module-digest
hahnandrew 99941ff
temp HEAD, do not checkout
hahnandrew File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
use std::collections::HashMap; | ||
use std::os::raw::c_void; | ||
use std::ptr::null; | ||
use raw::KeyType; | ||
use valkey_module::alloc::ValkeyAlloc; | ||
use valkey_module::native_types::ValkeyType; | ||
use valkey_module::digest::Digest; | ||
use valkey_module::key::{ValkeyKey, ValkeyKeyWritable}; | ||
use valkey_module::{raw, valkey_module, Context, NextArg, ValkeyResult, ValkeyString}; | ||
|
||
#[derive(Debug)] | ||
struct MyType { | ||
data: String, | ||
} | ||
|
||
#[derive(Debug)] | ||
struct Memblock { | ||
dictionary: HashMap<i32, MyType>, | ||
} | ||
|
||
impl Memblock { | ||
fn new() -> Self { | ||
Memblock { | ||
dictionary: HashMap::new(), | ||
} | ||
} | ||
|
||
fn insert(&mut self, key: i32, value: MyType) { | ||
self.dictionary.insert(key, value); | ||
} | ||
|
||
fn get(&self, key: &i32) -> Option<&MyType> { | ||
self.dictionary.get(key) | ||
} | ||
} | ||
|
||
static MY_VALKEY_TYPE: ValkeyType = ValkeyType::new( | ||
"mytype123", | ||
0, | ||
raw::RedisModuleTypeMethods { | ||
version: raw::REDISMODULE_TYPE_METHOD_VERSION as u64, | ||
rdb_load: None, | ||
rdb_save: None, | ||
aof_rewrite: None, | ||
free: Some(free), | ||
digest: Some(digest), | ||
|
||
// Currently unused by Redis | ||
mem_usage: None, | ||
|
||
// Aux data | ||
aux_load: None, | ||
aux_save: None, | ||
aux_save2: None, | ||
aux_save_triggers: 0, | ||
|
||
free_effort: None, | ||
unlink: None, | ||
copy: None, | ||
defrag: None, | ||
|
||
copy2: None, | ||
free_effort2: None, | ||
mem_usage2: None, | ||
unlink2: None, | ||
}, | ||
); | ||
|
||
unsafe extern "C" fn free(value: *mut c_void) { | ||
drop(Box::from_raw(value.cast::<MyType>())); | ||
} | ||
|
||
unsafe extern "C" fn digest(md: *mut raw::RedisModuleDigest, value: *mut c_void) { | ||
// let mut dig = Digest::new(md); | ||
// let val = &*(value.cast::<MyType>()); | ||
|
||
// dig.add_long_long(val.data.parse::<i64>().unwrap()); | ||
// let keyname = dig.get_key_name(); | ||
// let dbid = dig.get_db_id(); | ||
// assert!(!keyname.is_empty()); | ||
// assert!(dbid != i32::MIN && dbid != i32::MAX, "dbid should not be i32::MIN or i32::MAX, got: {}", dbid); | ||
// dig.end_sequence(); | ||
} | ||
|
||
fn alloc_set(ctx: &Context, args: Vec<ValkeyString>) -> ValkeyResult { | ||
let mut args = args.into_iter().skip(1); | ||
let key = args.next_arg()?; | ||
let size = args.next_i64()?; | ||
|
||
ctx.log_debug(format!("key: {key}, size: {size}").as_str()); | ||
|
||
let key = ctx.open_key_writable(&key); | ||
|
||
if let Some(value) = key.get_value::<MyType>(&MY_VALKEY_TYPE)? { | ||
value.data = "B".repeat(size as usize); | ||
} else { | ||
let value = MyType { | ||
data: "A".repeat(size as usize), | ||
}; | ||
|
||
key.set_value(&MY_VALKEY_TYPE, value)?; | ||
} | ||
Ok(size.into()) | ||
} | ||
|
||
fn alloc_get(ctx: &Context, args: Vec<ValkeyString>) -> ValkeyResult { | ||
let mut args = args.into_iter().skip(1); | ||
let key = args.next_arg()?; | ||
|
||
let key = ctx.open_key(&key); | ||
|
||
let value = match key.get_value::<MyType>(&MY_VALKEY_TYPE)? { | ||
Some(value) => value.data.as_str().into(), | ||
None => ().into(), | ||
}; | ||
|
||
Ok(value) | ||
} | ||
|
||
////////////////////////////////////////////////////// | ||
|
||
valkey_module! { | ||
name: "alloc", | ||
version: 1, | ||
allocator: (ValkeyAlloc, ValkeyAlloc), | ||
data_types: [ | ||
MY_VALKEY_TYPE, | ||
], | ||
commands: [ | ||
["alloc.set", alloc_set, "write", 1, 1, 1], | ||
["alloc.get", alloc_get, "readonly", 1, 1, 1], | ||
], | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
use std::collections::HashMap; | ||
use std::os::raw::c_void; | ||
use std::ptr::null; | ||
use raw::KeyType; | ||
use valkey_module::alloc::ValkeyAlloc; | ||
use valkey_module::native_types::ValkeyType; | ||
use valkey_module::digest::Digest; | ||
use valkey_module::key::{ValkeyKey, ValkeyKeyWritable}; | ||
use valkey_module::{raw, valkey_module, Context, NextArg, ValkeyResult, ValkeyString}; | ||
|
||
#[derive(Debug)] | ||
struct MyType { | ||
data: String, | ||
} | ||
|
||
// #[derive(Debug)] | ||
// struct Memblock { | ||
// dictionary: HashMap<i32, MyType>, | ||
// } | ||
|
||
// impl Memblock { | ||
// fn new() -> Self { | ||
// Memblock { | ||
// dictionary: HashMap::new(), | ||
// } | ||
// } | ||
|
||
// fn insert(&mut self, key: i32, value: MyType) { | ||
// self.dictionary.insert(key, value); | ||
// } | ||
|
||
// fn get(&self, key: &i32) -> Option<&MyType> { | ||
// self.dictionary.get(key) | ||
// } | ||
// } | ||
|
||
static MY_VALKEY_TYPE: ValkeyType = ValkeyType::new( | ||
"mytype123", | ||
0, | ||
raw::RedisModuleTypeMethods { | ||
version: raw::REDISMODULE_TYPE_METHOD_VERSION as u64, | ||
rdb_load: None, | ||
rdb_save: None, | ||
aof_rewrite: None, | ||
free: Some(free), | ||
digest: Some(digest), | ||
|
||
// Currently unused by Redis | ||
mem_usage: None, | ||
|
||
// Aux data | ||
aux_load: None, | ||
aux_save: None, | ||
aux_save2: None, | ||
aux_save_triggers: 0, | ||
|
||
free_effort: None, | ||
unlink: None, | ||
copy: None, | ||
defrag: None, | ||
|
||
copy2: None, | ||
free_effort2: None, | ||
mem_usage2: None, | ||
unlink2: None, | ||
}, | ||
); | ||
|
||
unsafe extern "C" fn free(value: *mut c_void) { | ||
drop(Box::from_raw(value.cast::<MyType>())); | ||
} | ||
|
||
unsafe extern "C" fn digest(md: *mut raw::RedisModuleDigest, value: *mut c_void) { | ||
let mut dig = Digest::new(md); | ||
let val = &*(value.cast::<MyType>()); | ||
dig.add_string_buffer(&val.data.as_bytes()); | ||
dig.end_sequence(); | ||
} | ||
|
||
// unsafe extern "C" fn digest(md: *mut raw::RedisModuleDigest, value: *mut c_void) { | ||
// let mut dig = Digest::new(md); | ||
// let val = &*(value.cast::<MyType>()); | ||
|
||
// dig.add_long_long(val.data.parse::<i64>().unwrap()); | ||
// let keyname = dig.get_key_name(); | ||
// let dbid = dig.get_db_id(); | ||
// assert!(!keyname.is_empty()); | ||
// assert!(dbid != i32::MIN && dbid != i32::MAX, "dbid should not be i32::MIN or i32::MAX, got: {}", dbid); | ||
// dig.end_sequence(); | ||
// } | ||
|
||
fn alloc_set(ctx: &Context, args: Vec<ValkeyString>) -> ValkeyResult { | ||
let mut args = args.into_iter().skip(1); | ||
let key = args.next_arg()?; | ||
let size = args.next_i64()?; | ||
|
||
ctx.log_debug(format!("key: {key}, size: {size}").as_str()); | ||
|
||
let key = ctx.open_key_writable(&key); | ||
|
||
if let Some(value) = key.get_value::<MyType>(&MY_VALKEY_TYPE)? { | ||
value.data = "B".repeat(size as usize); | ||
} else { | ||
let value = MyType { | ||
data: "A".repeat(size as usize), | ||
}; | ||
|
||
key.set_value(&MY_VALKEY_TYPE, value)?; | ||
} | ||
Ok(size.into()) | ||
} | ||
|
||
fn alloc_get(ctx: &Context, args: Vec<ValkeyString>) -> ValkeyResult { | ||
let mut args = args.into_iter().skip(1); | ||
let key = args.next_arg()?; | ||
|
||
let key = ctx.open_key(&key); | ||
|
||
let value = match key.get_value::<MyType>(&MY_VALKEY_TYPE)? { | ||
Some(value) => value.data.as_str().into(), | ||
None => ().into(), | ||
}; | ||
|
||
Ok(value) | ||
} | ||
|
||
////////////////////////////////////////////////////// | ||
|
||
valkey_module! { | ||
name: "alloc", | ||
version: 1, | ||
allocator: (ValkeyAlloc, ValkeyAlloc), | ||
data_types: [ | ||
MY_VALKEY_TYPE, | ||
], | ||
commands: [ | ||
["alloc.set", alloc_set, "write", 1, 1, 1], | ||
["alloc.get", alloc_get, "readonly", 1, 1, 1], | ||
], | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
so we exercise
add_string_buffer
andend_sequence
via Integration Tests. How can we add tests to also testget_key_name
,get_db_id
andadd_long_long
?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 calling this out. I'm thinking that we can follow core and integration test
get_key_name
,get_db_id
andadd_long_long
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.
@KarthikSubbarao - your thoughts?
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.
Yes, it makes sense to test all the new functionality that being added, so I agree with your comment, @dmitrypol .
@hahnandrew That makes sense. We can follow the core and have the Digest callback of the example data type use all the new wrapper Digest APIs added.
Here is how the core tests the Module APIs (using the core example module).
Core Unit Tests written in TCL: https://github.com/valkey-io/valkey/blob/2df56d87c0ebe802f38e8922bb2ea1e4ca9cfa76/tests/unit/moduleapi/datatype2.tcl#L183
Core Module: https://github.com/valkey-io/valkey/blob/b0f23df16522e91a769c75646166045ae70e8d4e/tests/modules/datatype2.c#L614
For the
get_db_id
, notice how they useSELECT
to switch data bases. We can do the same and validate that the APIs work as expected