Add tightening_ratio support #26

nnmehta · 2024-11-28T00:39:09Z

Add tightening_ratio support for tuning of the TIGHENTING_RATIO to allow scalable bloom objects to have a higher capacity at cost of higher false positive rate.

We allow the TIGHTENING_RATIO argument only if the command is replicated.

KarthikSubbarao · 2024-12-01T00:05:46Z

Can this PR be rebased? The digest change is merged in and looks like this PR contains commits from digest (which should be removed after rebasing)

KarthikSubbarao · 2024-12-01T00:07:28Z

src/bloom/command_handler.rs

+    let tightening_ratio = match input_args[curr_cmd_idx].to_string_lossy().parse::<f64>() {
+        Ok(num) if num > TIGHTENING_RATIO_MIN && num < TIGHTENING_RATIO_MAX => num,
+        Ok(num) if !(num > TIGHTENING_RATIO_MIN && num < TIGHTENING_RATIO_MAX) => {
+            return Err(ValkeyError::Str(utils::ERROR_RATIO_RANGE));
+        }
+        _ => {
+            return Err(ValkeyError::Str(utils::BAD_ERROR_RATIO));
+        }
+    };
+    curr_cmd_idx += 1;


This will not be provided as a command argument for the BF.RESERVE command. We can remove this

Sure will remove this

KarthikSubbarao · 2024-12-01T00:09:59Z

src/bloom/command_handler.rs

@@ -327,6 +341,18 @@ pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> Valkey
                    }
                };
            }
+            "RATIO" => {


Renaming to TIGHTENING makes this clearer

Let's add a check to only allow this argument if the command is replicated. If it is not replicated, we should not accept it as command args. This means, if the command is on a primary, it will use the constant from the configs.rs file

KarthikSubbarao · 2024-12-01T00:12:18Z

src/bloom/command_handler.rs

@@ -228,6 +230,16 @@ pub fn bloom_filter_reserve(ctx: &Context, input_args: &[ValkeyString]) -> Valke
        }


We will need to update the arity check at the beginning of the fn to handle the extra TIGHTENING argument

Will update this

KarthikSubbarao · 2024-12-01T00:14:52Z

src/configs.rs

@@ -32,6 +32,8 @@ lazy_static! {
 // Tightening ratio used during scale out for the calculation of fp_rate of every new filter within a bloom object to
 // maintain the bloom object's overall fp_rate to the configured value.
 pub const TIGHTENING_RATIO: f64 = 0.5;
+pub const TIGHTENING_RATIO_MIN: f64 = 0.001;


This should be 0, right?

Will update

KarthikSubbarao · 2024-12-01T00:22:20Z

src/wrapper/bloom_callback.rs

@@ -2,6 +2,7 @@ use crate::bloom;
 use crate::bloom::data_type::ValkeyDataType;
 use crate::bloom::utils::BloomFilterType;
 use crate::configs;
+use crate::wrapper::digest::Digest;


Since we are updating RDB Load, we need to update RDB Save to add the tightening ratio at the right place.

Will update here in rdb_save

KarthikSubbarao · 2024-12-01T00:25:07Z

src/bloom/data_type.rs

+    fn debug_digest(&self, mut dig: Digest) {
+        dig.add_long_long(self.expansion.into());
+        dig.add_string_buffer(&self.fp_rate.to_le_bytes());
+        // dig.add_string_buffer(&self.tightening_ratio.to_le_bytes());


We should add the tightening ratio to the digest. Why is this commented out?

I was testing out something else and commented this briefly, will uncomment in future revision

KarthikSubbarao · 2024-12-03T23:29:16Z

src/bloom/command_handler.rs

@@ -299,14 +298,15 @@ pub fn bloom_filter_reserve(ctx: &Context, input_args: &[ValkeyString]) -> Valke
 pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> ValkeyResult {
    let argc = input_args.len();
    // At the very least, we need: BF.INSERT <key> ITEMS <item>
-    if argc < 4 {
+    if argc < 5 {


This depends on ctx.get_flags().contains(ContextFlags::REPLICATED), right?

If replica, < 5. If primary, < 4. Can we update it?

KarthikSubbarao · 2024-12-03T23:30:25Z

src/bloom/utils.rs

@@ -94,9 +98,11 @@ impl BloomFilterType {
        // Create the bloom filter and add to the main BloomFilter object.
        let bloom = BloomFilter::new(fp_rate, capacity);
        let filters = vec![bloom];
+        let tightening_ratio = 0.5;


This will need to be passed in as arguments from new_reserved. This should not be hard coded

I had added it in this change but since you mentioned above:

not be provided as a command argument for the BF.RESERVE command

I had to remove it wherever we use new_reserved

KarthikSubbarao · 2024-12-04T18:26:56Z

src/bloom/command_handler.rs

@@ -227,7 +227,6 @@ pub fn bloom_filter_reserve(ctx: &Context, input_args: &[ValkeyString]) -> Valke
            return Err(ValkeyError::Str(utils::BAD_ERROR_RATE));
        }
    };
-    curr_cmd_idx += 1;


Let's add this back. It might have been accidentally removed in your PR

KarthikSubbarao · 2024-12-04T18:33:56Z

src/bloom/utils.rs

@@ -779,6 +794,7 @@ mod tests {

        // verify new_bf and bf
        assert_eq!(bf.fp_rate, new_bf.fp_rate);
+        assert_eq!(bf.tightening_ratio, new_bf.tightening_ratio);


In the verify_restored_items function, can we assert on this?
You can also do this assert for the false positive rate

assert_eq!( restored_bloom_filter_type.tightenting_ratio, original_bloom_filter_type.tightenting_ratio );

KarthikSubbarao · 2024-12-05T06:44:48Z

src/bloom/command_handler.rs

@@ -298,15 +302,17 @@ pub fn bloom_filter_reserve(ctx: &Context, input_args: &[ValkeyString]) -> Valke

 pub fn bloom_filter_insert(ctx: &Context, input_args: &[ValkeyString]) -> ValkeyResult {
    let argc = input_args.len();
+    let validate_size_limit = ctx.get_flags().contains(ContextFlags::REPLICATED);


Suggested change

let validate_size_limit = ctx.get_flags().contains(ContextFlags::REPLICATED);

let replicated_cmd = ctx.get_flags().contains(ContextFlags::REPLICATED);

Will update

KarthikSubbarao · 2024-12-05T06:48:50Z

src/bloom/command_handler.rs

+            "TIGHTENING" => {
+                if !validate_size_limit {
+                    return Err(ValkeyError::Str(utils::ERROR));
+                }


Suggested change

"TIGHTENING" => {

if !validate_size_limit {

return Err(ValkeyError::Str(utils::ERROR));

}

"TIGHTENING" if replicated_cmd => {

if idx >= (argc - 1) {

return Err(ValkeyError::WrongArity);

}

KarthikSubbarao · 2024-12-05T06:51:28Z

src/bloom/command_handler.rs

    // At the very least, we need: BF.INSERT <key> ITEMS <item>
-    if argc < 4 {
+    if !validate_size_limit && argc < 4 || validate_size_limit && argc < 5 {


Suggested change

if !validate_size_limit && argc < 4 || validate_size_limit && argc < 5 {

if argc < 4 {

This should handle it already. It is not required to contain tightening ratio on the replicated command - it is optional and we should handle cases where it provided and not provided.

The check above is sufficient

src/bloom/command_handler.rs

KarthikSubbarao · 2024-12-05T06:54:28Z

src/bloom/utils.rs

@@ -95,9 +100,11 @@ impl BloomFilterType {
        // Create the bloom filter and add to the main BloomFilter object.
        let bloom = BloomFilter::new(fp_rate, capacity);
        let filters = vec![bloom];
+        let tightening_ratio = 0.5;


Why is this needed? Removing it

Suggested change

let tightening_ratio = 0.5;

Missed to remove this, it is not required

KarthikSubbarao · 2024-12-05T07:03:20Z

tests/test_replication.py

@@ -34,7 +34,7 @@ def test_replication_behavior(self):
            ('BF.ADD', 'BF.ADD key item', 'BF.ADD key item1', 2),
            ('BF.MADD', 'BF.MADD key item', 'BF.MADD key item1', 2),
            ('BF.RESERVE', 'BF.RESERVE key 0.001 100000', 'BF.ADD key item1', 1),
-            ('BF.INSERT', 'BF.INSERT key items item', 'BF.INSERT key items item1', 2),
+            ('BF.INSERT', 'BF.INSERT key items item tightening 0.5', 'BF.INSERT key items item1 tightening 0.5', 2),


Suggested change

('BF.INSERT', 'BF.INSERT key items item tightening 0.5', 'BF.INSERT key items item1 tightening 0.5', 2),

('BF.INSERT', 'BF.INSERT key items item', 'BF.INSERT key items item1', 2),

This is an invalid test. The tightening ratio is coming after ITEM so it is treated as an item. It is not treated as a property. Also, we cannot use this command on the primary, since we only allow this arg during replication :D

Let's just use a normal BF.INSERT command.

Once we support the tightening ratio config, we can then (1) update the config on the primary node, (2) Use BF.INSERT (3) Validate Digest matches on the replica even though the tightening ratio config is different

For now, let us leave it as it is because it is essentially untested code (unless we use a monitor on replica during bloom creation on the primary node)

Ah got it, will revert this change accordingly

KarthikSubbarao · 2024-12-05T07:04:45Z

src/bloom/utils.rs

@@ -642,12 +672,18 @@ mod tests {
        let rand_prefix = random_prefix(7);
        // 1 in every 1000 operations is expected to be a false positive.
        let expected_fp_rate: f64 = 0.001;
+        let expected_tightening_ratio: f64 = 0.5;


Suggested change

let expected_tightening_ratio: f64 = 0.5;

let tightening_ratio: f64 = 0.5;

KarthikSubbarao · 2024-12-05T07:05:32Z

src/bloom/utils.rs

@@ -584,13 +608,19 @@ mod tests {
        let rand_prefix = random_prefix(7);
        // 1 in every 1000 operations is expected to be a false positive.
        let expected_fp_rate: f64 = 0.001;
+        let expected_tightening_ratio: f64 = 0.5;


Suggested change

let expected_tightening_ratio: f64 = 0.5;

let tightening_ratio: f64 = 0.5;

We are not asserting against this. so there is no need to call it "expected"

Will change accordingly

Signed-off-by: Nihal Mehta <[email protected]>

zackcam · 2024-12-06T06:36:24Z

src/bloom/utils.rs

@@ -22,6 +25,8 @@ pub const BAD_EXPANSION: &str = "ERR bad expansion";
 pub const BAD_CAPACITY: &str = "ERR bad capacity";
 pub const BAD_ERROR_RATE: &str = "ERR bad error rate";
 pub const ERROR_RATE_RANGE: &str = "ERR (0 < error rate range < 1)";
+pub const BAD_ERROR_RATIO: &str = "ERR bad error ratio";
+pub const ERROR_RATIO_RANGE: &str = "ERR (0 < error ratio range < 1)";


Would having a better error message i.e. something like ratio must be in the range (0, 1) or is following convention for error rate range better?

KarthikSubbarao reviewed Dec 1, 2024

View reviewed changes

nnmehta closed this Dec 3, 2024

nnmehta force-pushed the tightening_ratio branch from 6bec972 to 7c25468 Compare December 3, 2024 21:08

nnmehta reopened this Dec 3, 2024

nnmehta force-pushed the tightening_ratio branch 2 times, most recently from 99acf6f to 078534f Compare December 3, 2024 21:20

KarthikSubbarao reviewed Dec 3, 2024

View reviewed changes

KarthikSubbarao reviewed Dec 4, 2024

View reviewed changes

nnmehta force-pushed the tightening_ratio branch from c992016 to 1fb640c Compare December 4, 2024 23:25

KarthikSubbarao reviewed Dec 5, 2024

View reviewed changes

src/bloom/command_handler.rs Show resolved Hide resolved

KarthikSubbarao reviewed Dec 5, 2024

View reviewed changes

Add tightening_ratio support in BloomFilterType structure

de6ec44

Signed-off-by: Nihal Mehta <[email protected]>

nnmehta force-pushed the tightening_ratio branch from e4521ea to de6ec44 Compare December 5, 2024 21:18

KarthikSubbarao approved these changes Dec 6, 2024

View reviewed changes

zackcam reviewed Dec 6, 2024

View reviewed changes

zackcam approved these changes Dec 6, 2024

View reviewed changes

KarthikSubbarao merged commit f10a4e5 into valkey-io:unstable Dec 6, 2024
6 checks passed

nnmehta deleted the tightening_ratio branch December 6, 2024 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tightening_ratio support #26

Add tightening_ratio support #26

nnmehta commented Nov 28, 2024 •

edited

Loading

KarthikSubbarao commented Dec 1, 2024

KarthikSubbarao Dec 1, 2024 •

edited

Loading

nnmehta Dec 3, 2024

KarthikSubbarao Dec 1, 2024 •

edited

Loading

KarthikSubbarao Dec 1, 2024 •

edited

Loading

nnmehta Dec 3, 2024

KarthikSubbarao Dec 1, 2024

nnmehta Dec 3, 2024

KarthikSubbarao Dec 1, 2024

nnmehta Dec 3, 2024

KarthikSubbarao Dec 1, 2024

nnmehta Dec 3, 2024

KarthikSubbarao Dec 3, 2024

KarthikSubbarao Dec 3, 2024

nnmehta Dec 3, 2024 •

edited

Loading

KarthikSubbarao Dec 4, 2024

KarthikSubbarao Dec 4, 2024

KarthikSubbarao Dec 5, 2024

nnmehta Dec 5, 2024

KarthikSubbarao Dec 5, 2024 •

edited

Loading

KarthikSubbarao Dec 5, 2024

KarthikSubbarao Dec 5, 2024

nnmehta Dec 5, 2024

KarthikSubbarao Dec 5, 2024 •

edited

Loading

nnmehta Dec 5, 2024

KarthikSubbarao Dec 5, 2024

KarthikSubbarao Dec 5, 2024

nnmehta Dec 5, 2024

zackcam Dec 6, 2024

		@@ -228,6 +230,16 @@ pub fn bloom_filter_reserve(ctx: &Context, input_args: &[ValkeyString]) -> Valke
		}

	let validate_size_limit = ctx.get_flags().contains(ContextFlags::REPLICATED);
	let replicated_cmd = ctx.get_flags().contains(ContextFlags::REPLICATED);

	if !validate_size_limit && argc < 4 \|\| validate_size_limit && argc < 5 {
	if argc < 4 {

	('BF.INSERT', 'BF.INSERT key items item tightening 0.5', 'BF.INSERT key items item1 tightening 0.5', 2),
	('BF.INSERT', 'BF.INSERT key items item', 'BF.INSERT key items item1', 2),

	let expected_tightening_ratio: f64 = 0.5;
	let tightening_ratio: f64 = 0.5;

Add tightening_ratio support #26

Add tightening_ratio support #26

Conversation

nnmehta commented Nov 28, 2024 • edited Loading

KarthikSubbarao commented Dec 1, 2024

KarthikSubbarao Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarthikSubbarao Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

KarthikSubbarao Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmehta Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarthikSubbarao Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarthikSubbarao Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmehta commented Nov 28, 2024 •

edited

Loading

KarthikSubbarao Dec 1, 2024 •

edited

Loading

KarthikSubbarao Dec 1, 2024 •

edited

Loading

KarthikSubbarao Dec 1, 2024 •

edited

Loading

nnmehta Dec 3, 2024 •

edited

Loading

KarthikSubbarao Dec 5, 2024 •

edited

Loading

KarthikSubbarao Dec 5, 2024 •

edited

Loading