Add TIGHTENING Ratio and FP_Rate module string configs and parse to float #31

nnmehta · 2024-12-09T20:39:33Z

Supporting False Positive config and Tightening Ratio Config.

We have already implemented feature for module configs to support validation & rejection of config sets so that we have the right values for False Positive config and Tightening Ratio Config. valkey-io/valkeymodule-rs#144

With these changes, we can fine tune the configs based on user's workloads and requirements

src/configs.rs

src/lib.rs

src/configs.rs

tests/test_replication.py

KarthikSubbarao · 2024-12-13T18:40:11Z

tests/test_basic.py

@@ -299,6 +299,22 @@ def test_debug_cmd(self):
        else:
            madd_scenario_object_digest == madd_default_object_digest

+        # validates that digest differs on bloom objects after changing the tightening_ratio
+        client.execute_command('BF.INSERT tightening_ratio ERROR 0.001 CAPACITY 1000 ITEMS item1')


Nit: For a simpler example, can we use BF.RESERVE without any item? Let's use BF.RESERVE for every bloom object creation from line 302 to 316

KarthikSubbarao · 2024-12-13T18:45:37Z

src/configs.rs

+    config_ctx: &ConfigurationContext,
+    name: &str,
+    val: &'static ValkeyGILGuard<ValkeyString>,
+) {


Change this to return an error... Right now this is not the config set handler. With the current syntax, it is the on config change handler.

Suggested change

) {

) -> Result<(), ValkeyError> {

src/configs.rs

KarthikSubbarao · 2024-12-13T18:46:33Z

src/configs.rs

+        ValkeyString::create(None, BLOOM_FP_RATE_DEFAULT.to_string())
+    );
+    pub static ref TIGHTENING_RATIO_DEFAULT_STRING: String = TIGHTENING_RATIO.to_string();
+    pub static ref TIGHTENING_RATIO_CONFIG: ValkeyGILGuard<ValkeyString> =


Suggested change

pub static ref TIGHTENING_RATIO_CONFIG: ValkeyGILGuard<ValkeyString> =

pub static ref BLOOM_TIGHTENING_RATIO: ValkeyGILGuard<ValkeyString> =

KarthikSubbarao · 2024-12-13T18:47:33Z

src/configs.rs

+            let _: Result<(), ValkeyError> = Err(ValkeyError::Str("Invalid floating-point value"));
+            return;


We need to return an error...

Suggested change

let _: Result<(), ValkeyError> = Err(ValkeyError::Str("Invalid floating-point value"));

return;

return Err(ValkeyError::Str("Invalid value: Unable to parse string as f64."));

KarthikSubbarao · 2024-12-13T18:48:53Z

src/configs.rs

+            let _: Result<(), ValkeyError> =
+                Err(ValkeyError::Str("Unknown configuration parameter"));


We need to return an error..

Suggested change

let _: Result<(), ValkeyError> =

Err(ValkeyError::Str("Unknown configuration parameter"));

return Err(ValkeyError::Str("Unknown configuration parameter"));

src/configs.rs

KarthikSubbarao · 2024-12-13T18:52:56Z

tests/test_basic.py

+        # validates that digest differs on bloom objects after changing the fp_rate
+        client.execute_command('BF.INSERT fp_rate ITEMS item1')
+        assert self.client.execute_command('CONFIG SET bf.bloom-fp-rate 0.5') == b'OK'
+        client.execute_command('BF.INSERT fp_rate2 ERROR 0.1 ITEMS item1')


Suggested change

client.execute_command('BF.INSERT fp_rate2 ERROR 0.1 ITEMS item1')

client.execute_command('BF.RESERVE fp_rate2 0.001 1000')

KarthikSubbarao · 2024-12-13T18:53:14Z

tests/test_basic.py

+        assert scenario_tightening_ratio_object_digest != scenario_tightening_ratio2_digest
+
+        # validates that digest differs on bloom objects after changing the fp_rate
+        client.execute_command('BF.INSERT fp_rate ITEMS item1')


Suggested change

client.execute_command('BF.INSERT fp_rate ITEMS item1')

client.execute_command('BF.RESERVE fp_rate 0.001 1000)

KarthikSubbarao · 2024-12-13T18:53:32Z

tests/test_basic.py

+        # validates that digest differs on bloom objects after changing the tightening_ratio
+        client.execute_command('BF.INSERT tightening_ratio ERROR 0.001 CAPACITY 1000 ITEMS item1')
+        assert self.client.execute_command('CONFIG SET bf.bloom-tightening-ratio 0.75') == b'OK'
+        client.execute_command('BF.INSERT tightening_ratio2 ERROR 0.01 CAPACITY 1000 ITEMS item1')


Suggested change

client.execute_command('BF.INSERT tightening_ratio2 ERROR 0.01 CAPACITY 1000 ITEMS item1')

client.execute_command('BF.RESERVE tightening_ratio2 0.001 1000')

KarthikSubbarao · 2024-12-13T18:54:30Z

tests/test_basic.py

@@ -299,6 +299,22 @@ def test_debug_cmd(self):
        else:
            madd_scenario_object_digest == madd_default_object_digest

+        # validates that digest differs on bloom objects after changing the tightening_ratio
+        client.execute_command('BF.INSERT tightening_ratio ERROR 0.001 CAPACITY 1000 ITEMS item1')


Suggested change

client.execute_command('BF.INSERT tightening_ratio ERROR 0.001 CAPACITY 1000 ITEMS item1')

client.execute_command('BF.RESERVE tightening_ratio 0.001 1000')

KarthikSubbarao · 2024-12-13T18:55:28Z

src/lib.rs

@@ -106,6 +108,8 @@ valkey_module! {
            ["bloom-memory-limit-per-filter", &*configs::BLOOM_MEMORY_LIMIT_PER_FILTER, configs::BLOOM_MEMORY_LIMIT_PER_FILTER_DEFAULT, configs::BLOOM_MEMORY_LIMIT_PER_FILTER_MIN, configs::BLOOM_MEMORY_LIMIT_PER_FILTER_MAX, ConfigurationFlags::DEFAULT, None],
        ],
        string: [
+            ["bloom-fp-rate", &*configs::BLOOM_FP_RATE, &*configs::BLOOM_FP_RATE_DEFAULT_STRING, ConfigurationFlags::DEFAULT, Some(Box::new(configs::on_string_config_set))],


This is not implementing a config set handler. It is implementing a config on change handler. This is how you implement a config set handler:

Suggested change

["bloom-fp-rate", &*configs::BLOOM_FP_RATE, &*configs::BLOOM_FP_RATE_DEFAULT_STRING, ConfigurationFlags::DEFAULT, Some(Box::new(configs::on_string_config_set))],

["bloom-fp-rate", &*configs::BLOOM_FP_RATE, &*configs::BLOOM_FP_RATE_DEFAULT_STRING, ConfigurationFlags::DEFAULT, None, Some(Box::new(configs::on_string_config_set))],

KarthikSubbarao · 2024-12-13T18:55:45Z

src/lib.rs

@@ -106,6 +108,8 @@ valkey_module! {
            ["bloom-memory-limit-per-filter", &*configs::BLOOM_MEMORY_LIMIT_PER_FILTER, configs::BLOOM_MEMORY_LIMIT_PER_FILTER_DEFAULT, configs::BLOOM_MEMORY_LIMIT_PER_FILTER_MIN, configs::BLOOM_MEMORY_LIMIT_PER_FILTER_MAX, ConfigurationFlags::DEFAULT, None],
        ],
        string: [
+            ["bloom-fp-rate", &*configs::BLOOM_FP_RATE, &*configs::BLOOM_FP_RATE_DEFAULT_STRING, ConfigurationFlags::DEFAULT, Some(Box::new(configs::on_string_config_set))],
+            ["bloom-tightening-ratio", &*configs::TIGHTENING_RATIO_CONFIG, &*configs::TIGHTENING_RATIO_DEFAULT_STRING, ConfigurationFlags::DEFAULT, Some(Box::new(configs::on_string_config_set))],


Currently, this is not implementing a config set handler. It is implementing a config on change handler. This is how you implement a config set handler:

Suggested change

["bloom-tightening-ratio", &*configs::TIGHTENING_RATIO_CONFIG, &*configs::TIGHTENING_RATIO_DEFAULT_STRING, ConfigurationFlags::DEFAULT, Some(Box::new(configs::on_string_config_set))],

["bloom-tightening-ratio", &*configs::TIGHTENING_RATIO_CONFIG, &*configs::TIGHTENING_RATIO_DEFAULT_STRING, ConfigurationFlags::DEFAULT, None, Some(Box::new(configs::on_string_config_set))],

KarthikSubbarao · 2024-12-13T19:01:16Z

src/configs.rs

@@ -31,6 +39,13 @@ lazy_static! {
        AtomicI64::new(BLOOM_MEMORY_LIMIT_PER_FILTER_DEFAULT);
    pub static ref BLOOM_USE_RANDOM_SEED: AtomicBool = AtomicBool::default();
    pub static ref BLOOM_DEFRAG: AtomicBool = AtomicBool::new(BLOOM_DEFRAG_DEAFULT);
+    pub static ref BLOOM_FP_RATE_DEFAULT_STRING: String = BLOOM_FP_RATE_DEFAULT.to_string();


Can you add an actual f64 Mutex above this? We need to add this and change the f64 mutex.

Suggested change

pub static ref BLOOM_FP_RATE_DEFAULT_STRING: String = BLOOM_FP_RATE_DEFAULT.to_string();

pub static ref BLOOM_FP_RATE_F64: Mutex<f64> = Mutex::new(BLOOM_FP_RATE_DEFAULT);

You also don't need BLOOM_FP_RATE_DEFAULT. We can use to_string in the config macro during creation

If I just directly use BLOOM_FP_RATE_DEFAULT rather than using the string, it throws this error

81 | / valkey_module! { 82 | | name: MODULE_NAME, 83 | | version: 1, 84 | | allocator: (valkey_module::alloc::ValkeyAlloc, valkey_module::alloc::ValkeyAlloc), ... | 111 | | ["bloom-fp-rate", &*configs::BLOOM_FP_RATE, &*configs::BLOOM_FP_RATE_DEFAULT.to_string(), ConfigurationFlags::DEFAULT, None, Some(Box::new(configs::on_string_config_set))], | | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use ... | 121 | | ] 122 | | } | | - | | | | |_temporary value is freed at the end of this statement | borrow later stored here

Can you change this &*configs::BLOOM_FP_RATE_DEFAULT.to_string() to a string slice? The macro requires a string slice type for the default value

So do you want me to use this directly:

["bloom-fp-rate", &*configs::BLOOM_FP_RATE, "0.01", ConfigurationFlags::DEFAULT, None, Some(Box::new(configs::on_string_config_set))], ["bloom-tightening-ratio", &*configs::BLOOM_TIGHTENING_RATIO, "0.5", ConfigurationFlags::DEFAULT, None, Some(Box::new(configs::on_string_config_set))],

and avoid using to_string and the string constant we have in configs?

KarthikSubbarao · 2024-12-13T19:03:37Z

src/configs.rs

+    pub static ref BLOOM_FP_RATE: ValkeyGILGuard<ValkeyString> = ValkeyGILGuard::new(
+        ValkeyString::create(None, BLOOM_FP_RATE_DEFAULT.to_string())
+    );
+    pub static ref TIGHTENING_RATIO_DEFAULT_STRING: String = TIGHTENING_RATIO.to_string();


Can you add an actual f64 Mutex above this? We need to add this and change the f64 mutex.

Suggested change

pub static ref TIGHTENING_RATIO_DEFAULT_STRING: String = TIGHTENING_RATIO.to_string();

pub static ref BLOOM_TIGHTENING_F64: Mutex<f64> = Mutex::new(TIGHTENING_RATIO);

You also don't need TIGHTENING_RATIO_DEFAULT_STRING . We can use to_string in the config macro during creation

KarthikSubbarao · 2024-12-13T19:08:05Z

src/configs.rs

+                let _: Result<(), ValkeyError> = Err(ValkeyError::Str(utils::BAD_ERROR_RATE));
+                return;
+            }
+            BLOOM_FP_RATE.set(config_ctx, v.clone());


We need to change the f64 config here...

Suggested change

BLOOM_FP_RATE.set(config_ctx, v.clone());

let mut fp_rate = BLOOM_FP_RATE_F64.lock().expect("We expect the fp_rate static to exist.");

*fp_rate = value;

KarthikSubbarao · 2024-12-13T19:09:44Z

src/configs.rs

+                let _: Result<(), ValkeyError> = Err(ValkeyError::Str(utils::BAD_ERROR_RATIO));
+                return;
+            }
+            TIGHTENING_RATIO_CONFIG.set(config_ctx, v.clone());


We need to change the f64 config here...

Suggested change

TIGHTENING_RATIO_CONFIG.set(config_ctx, v.clone());

let mut tightening = BLOOM_TIGHTENING_RATIO.lock().expect("We expect the tightening_ratio static to exist.");

*tightening = value;

KarthikSubbarao · 2024-12-17T14:30:13Z

src/bloom/utils.rs

@@ -326,7 +326,8 @@ impl BloomFilterType {

    /// Calculate the false positive rate for the Nth filter using tightening ratio.
    pub fn calculate_fp_rate(fp_rate: f64, num_filters: i32) -> Result<f64, BloomError> {
-        match fp_rate * configs::TIGHTENING_RATIO.powi(num_filters) {
+        let tightening_ratio = *configs::BLOOM_TIGHTENING_F64.lock().unwrap();


Can we change this function to accept a tightening_ratio parameter?

We should not be using a constant or config here to find the tightening ratio. We should be using what is in the bloom object itself - because tightening ratio is specific to the object at the time of its creation.

There are two places where this function is used (1) add_item (2) load_from_rdb. We need to update how this function is called from both places.

For add_item, provide the tightening ratio from the BloomFilterType structure.

For load_from_rdb, provide the tightening ratio from the f64 loaded from RDB .

Okay, sure.

I will be changing the implementation as the following diff:

--- a/src/bloom/data_type.rs +++ b/src/bloom/data_type.rs @@ -87,15 +87,16 @@ impl ValkeyDataType for BloomFilterType { let Ok(capacity) = raw::load_unsigned(rdb) else { return None; }; - let new_fp_rate = match Self::calculate_fp_rate(fp_rate, num_filters as i32) { - Ok(rate) => rate, - Err(_) => { - logging::log_warning( - "Failed to restore bloom object: Reached max number of filters", - ); - return None; - } - }; + let new_fp_rate = + match Self::calculate_fp_rate(fp_rate, num_filters as i32, tightening_ratio) { + Ok(rate) => rate, + Err(_) => { + logging::log_warning( + "Failed to restore bloom object: Reached max number of filters", + ); + return None; + } + }; if !BloomFilter::validate_size(capacity as i64, new_fp_rate) { logging::log_warning("Failed to restore bloom object: Contains a filter larger than the max allowed size limit."); return None; diff --git a/src/bloom/utils.rs b/src/bloom/utils.rs index d8f2556..d808fdc 100644 --- a/src/bloom/utils.rs +++ b/src/bloom/utils.rs @@ -265,10 +265,11 @@ impl BloomFilterType { } // Scale out by adding a new filter with capacity bounded within the u32 range. false positive rate is also // bound within the range f64::MIN_POSITIVE <= x < 1.0. - let new_fp_rate = match Self::calculate_fp_rate(self.fp_rate, num_filters) { - Ok(rate) => rate, - Err(e) => return Err(e), - }; + let new_fp_rate = + match Self::calculate_fp_rate(self.fp_rate, num_filters, self.tightening_ratio) { + Ok(rate) => rate, + Err(e) => return Err(e), + }; let new_capacity = match filter.capacity.checked_mul(self.expansion.into()) { Some(new_capacity) => new_capacity, None => { @@ -325,8 +326,12 @@ impl BloomFilterType { } /// Calculate the false positive rate for the Nth filter using tightening ratio. - pub fn calculate_fp_rate(fp_rate: f64, num_filters: i32) -> Result<f64, BloomError> { - let tightening_ratio = *configs::BLOOM_TIGHTENING_F64.lock().unwrap(); + pub fn calculate_fp_rate( + fp_rate: f64, + num_filters: i32, + tightening_ratio: f64, + ) -> Result<f64, BloomError> { match fp_rate * tightening_ratio.powi(num_filters) { x if x > f64::MIN_POSITIVE => Ok(x), _ => Err(BloomError::MaxNumScalingFilters),

KarthikSubbarao · 2024-12-18T03:00:21Z

tests/test_basic.py

@@ -299,6 +299,25 @@ def test_debug_cmd(self):
        else:
            madd_scenario_object_digest == madd_default_object_digest

+        # validates that digest differs on bloom objects after changing the tightening_ratio


Suggested change

# validates that digest differs on bloom objects after changing the tightening_ratio

# scenario 6 validates that digest differs on bloom objects after changing the tightening_ratio config

KarthikSubbarao · 2024-12-18T03:00:34Z

tests/test_basic.py

+        scenario_tightening_ratio2_digest = client.execute_command('DEBUG DIGEST-VALUE tightening_ratio2')
+        assert scenario_tightening_ratio_object_digest != scenario_tightening_ratio2_digest
+
+        # validates that digest differs on bloom objects after changing the fp_rate


Suggested change

# validates that digest differs on bloom objects after changing the fp_rate

# scenario 7 validates that digest differs on bloom objects after changing the fp_rate config

KarthikSubbarao · 2024-12-18T03:00:52Z

tests/test_basic.py

+        client.execute_command('BF.RESERVE fp_rate 0.001 1000')
+        assert self.client.execute_command('CONFIG SET bf.bloom-fp-rate 0.5') == b'OK'
+        client.execute_command('BF.RESERVE fp_rate2 0.001 1000')


We can use a BF.INSERT here without specifying the error rate in both cases

KarthikSubbarao · 2024-12-18T03:01:46Z

tests/test_basic.py

+                self.client.execute_command('CONFIG SET bf.bloom-tightening-ratio 1.75')
+                assert False
+            except ResponseError as e:
+                pass


Can we validate the error here?

src/configs.rs

KarthikSubbarao · 2024-12-18T03:05:03Z

src/configs.rs

 }

 /// Constants
 // 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_DEFAULT: f64 = 0.5;


We can change this to a string if it is not used anywhere. It will help with config creation

KarthikSubbarao · 2024-12-18T03:05:43Z

src/bloom/command_handler.rs

-    let mut tightening_ratio = configs::TIGHTENING_RATIO;
+    let mut fp_rate = *configs::BLOOM_FP_RATE_F64
+        .lock()
+        .expect("Failed to lock fp_rate");


Suggested change

.expect("Failed to lock fp_rate");

.expect("Unable to get a lock on fp_rate static");

KarthikSubbarao · 2024-12-18T16:36:36Z

src/configs.rs

@@ -31,12 +39,20 @@ lazy_static! {
        AtomicI64::new(BLOOM_MEMORY_LIMIT_PER_FILTER_DEFAULT);
    pub static ref BLOOM_USE_RANDOM_SEED: AtomicBool = AtomicBool::default();
    pub static ref BLOOM_DEFRAG: AtomicBool = AtomicBool::new(BLOOM_DEFRAG_DEAFULT);
+    pub static ref BLOOM_FP_RATE_F64: Mutex<f64> =
+        Mutex::new(BLOOM_FP_RATE_DEFAULT.parse::<f64>().unwrap());


Let's use expect instead of unwrap

Suggested change

Mutex::new(BLOOM_FP_RATE_DEFAULT.parse::<f64>().unwrap());

Mutex::new(BLOOM_FP_RATE_DEFAULT.parse::<f64>().expect("Expected valid f64 for fp rate."));

KarthikSubbarao · 2024-12-18T16:37:14Z

src/configs.rs

+    pub static ref BLOOM_FP_RATE: ValkeyGILGuard<ValkeyString> =
+        ValkeyGILGuard::new(ValkeyString::create(None, BLOOM_FP_RATE_DEFAULT));
+    pub static ref BLOOM_TIGHTENING_F64: Mutex<f64> =
+        Mutex::new(TIGHTENING_RATIO_DEFAULT.parse::<f64>().unwrap());


Suggested change

Mutex::new(TIGHTENING_RATIO_DEFAULT.parse::<f64>().unwrap());

Mutex::new(TIGHTENING_RATIO_DEFAULT.parse::<f64>().expect("Expected valid f64 for tightening ratio."));

KarthikSubbarao · 2024-12-18T16:48:01Z

src/configs.rs

+
+    match name {
+        "bloom-fp-rate" => {
+            if !(BLOOM_FP_RATE_MIN..=BLOOM_FP_RATE_MAX).contains(&value) {


This will allow invalid values for fp rate (e.g 0.0 and 1.0) which should be rejected instead.

Can you change this to something like this?

Suggested change

if !(BLOOM_FP_RATE_MIN..=BLOOM_FP_RATE_MAX).contains(&value) {

if !(BLOOM_FP_RATE_MIN > value && value < BLOOM_FP_RATE_MAX) {

I will be removing = to for the range i.e. use

if !(BLOOM_FP_RATE_MIN..BLOOM_FP_RATE_MAX).contains(&value) {

When I use this

if !(BLOOM_FP_RATE_MIN > value && value < BLOOM_FP_RATE_MAX) {

it throws the following error

error: right-hand side of `&&` operator has no effect --> src/configs.rs:91:17 | 91 | if !(BLOOM_FP_RATE_MIN > value && value < BLOOM_FP_RATE_MAX) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `BLOOM_FP_RATE_MIN > value` evaluates to true, value < BLOOM_FP_RATE_MAX` will always evaluate to true as well --> src/configs.rs:91:44

KarthikSubbarao · 2024-12-18T16:48:35Z

src/configs.rs

+            Ok(())
+        }
+        "bloom-tightening-ratio" => {
+            if !(TIGHTENING_RATIO_MIN..=TIGHTENING_RATIO_MAX).contains(&value) {


This will allow invalid values for tightening ratio (e.g 0.0 and 1.0) which should be rejected instead.

Can you change this to something like this?

Suggested change

if !(TIGHTENING_RATIO_MIN..=TIGHTENING_RATIO_MAX).contains(&value) {

if !(TIGHTENING_RATIO_MIN > value && value < TIGHTENING_RATIO_MAX) {

I will be removing = to for the range i.e. use

if !(BLOOM_TIGHTENING_RATIO_MIN..BLOOM_TIGHTENING_RATIO_MAX).contains(&value) {

When I use this

if !(BLOOM_TIGHTENING_RATIO_MIN > value && value < BLOOM_TIGHTENING_RATIO_MAX) {

it throws the following error

error: right-hand side of `&&` operator has no effect --> src/configs.rs:101:17 | 101 | if !(BLOOM_TIGHTENING_RATIO_MIN > value && value < BLOOM_TIGHTENING_RATIO_MAX) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: `if `BLOOM_TIGHTENING_RATIO_MIN > value` evaluates to true, value < BLOOM_TIGHTENING_RATIO_MAX` will always evaluate to true as well --> src/configs.rs:101:53 | 101 | if !(BLOOM_TIGHTENING_RATIO_MIN > value && value < BLOOM_TIGHTENING_RATIO_MAX) {

This check is incorrect as it allows 0.0 which is an invalid fp rate.

Correct check:

if !(value > BLOOM_TIGHTENING_RATIO_MIN && value < BLOOM_TIGHTENING_RATIO_MAX) { ```

KarthikSubbarao · 2024-12-18T16:48:58Z

src/configs.rs

 }

 /// Constants
 // 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_DEFAULT: &str = "0.5";
 pub const TIGHTENING_RATIO_MIN: f64 = 0.0;


Suggested change

pub const TIGHTENING_RATIO_MIN: f64 = 0.0;

pub const BLOOM_TIGHTENING_RATIO_MIN: f64 = 0.0;

KarthikSubbarao · 2024-12-18T16:49:10Z

src/configs.rs

 }

 /// Constants
 // 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_DEFAULT: &str = "0.5";
 pub const TIGHTENING_RATIO_MIN: f64 = 0.0;
 pub const TIGHTENING_RATIO_MAX: f64 = 1.0;


Suggested change

pub const TIGHTENING_RATIO_MAX: f64 = 1.0;

pub const BLOOM_TIGHTENING_RATIO_MAX: f64 = 1.0;

KarthikSubbarao · 2024-12-18T16:52:19Z

tests/test_basic.py

+    #validates config set correctly and invalid values are rejected
+    def test_on_string_config_set(self):


Suggested change

#validates config set correctly and invalid values are rejected

def test_on_string_config_set(self):

def test_bloom_string_config_set(self):

"""

This is a test that validates the bloom string configuration set logic.

"""

KarthikSubbarao · 2024-12-18T16:53:42Z

src/configs.rs

+
+    let value = match value_str.parse::<f64>() {
+        Ok(v) => v,
+        Err(_) => {
+            return Err(ValkeyError::Str("Invalid floating-point value"));
+        }
+    };
+


We can remove the extra new lines here to help with readability

Suggested change

let value = match value_str.parse::<f64>() {

Ok(v) => v,

Err(_) => {

return Err(ValkeyError::Str("Invalid floating-point value"));

}

};

let value = match value_str.parse::<f64>() {

Ok(v) => v,

Err(_) => {

return Err(ValkeyError::Str("Invalid floating-point value"));

}

};

KarthikSubbarao · 2024-12-18T17:05:57Z

src/configs.rs

 // 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_DEFAULT: &str = "0.5";
 pub const TIGHTENING_RATIO_MIN: f64 = 0.0;


Can we move this block of code up to line 24? This is no longer a constant - it is a configuration, so we can include it in the section of configurations

Will move above

Signed-off-by: Nihal Mehta <nnmehta@amazon.com>

nnmehta force-pushed the unstable branch from 0e75a7f to adba9e5 Compare December 9, 2024 20:45

KarthikSubbarao assigned nnmehta Dec 10, 2024

nnmehta closed this Dec 10, 2024

nnmehta force-pushed the unstable branch from adba9e5 to a5ec8de Compare December 10, 2024 22:50

nnmehta reopened this Dec 12, 2024

nnmehta force-pushed the unstable branch from bcf243c to 7268bec Compare December 12, 2024 20:33

YueTang-Vanessa reviewed Dec 12, 2024

View reviewed changes