Skip to content
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

Add the support of the BF.MADD command #1758

Merged
merged 18 commits into from
Sep 19, 2023
Merged

Conversation

zncleon
Copy link
Contributor

@zncleon zncleon commented Sep 15, 2023

This PR introduce the bloom BF.MADD command.

[1] https://redis.io/commands/bf.madd/

@zncleon
Copy link
Contributor Author

zncleon commented Sep 15, 2023

I add the getBFDataList function to get all the bloomfilter data at once. And update the bloomCheck interface for only check the data.

@zncleon
Copy link
Contributor Author

zncleon commented Sep 15, 2023

About BF.MADD command, it will be likely to return error reply in a array. But I have no way to check it in gocase, for the string type is inconsistent to the error type. It seems have not enough API to transform it. How should I solve it ?

cc @mapleFU

@mapleFU
Copy link
Member

mapleFU commented Sep 15, 2023

Emmm nice catch. Since MADD in RedisBloom is not atomic, and just a helper...

You can try to use raw-text to check the test reponse in gocase, or cast it to any already-existing pattern( I think it's ok that not fully-consistent with RedisBloom)

@zncleon

This comment was marked as resolved.

@mapleFU
Copy link
Member

mapleFU commented Sep 16, 2023

You can maintain a protect/private constructor, and when build from non-owned, you can use a unique_ptr<const BlockSplit>CreateNonOwned(), and it can only call constant fn.

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance this looks ok to me, but I think we can add some further optimizations

src/commands/cmd_bloom_filter.cc Outdated Show resolved Hide resolved
src/types/redis_bloom_chain.cc Show resolved Hide resolved
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

src/commands/cmd_bloom_filter.cc Show resolved Hide resolved
src/types/redis_bloom_chain.cc Outdated Show resolved Hide resolved
src/types/redis_bloom_chain.cc Outdated Show resolved Hide resolved
src/types/redis_bloom_chain.cc Show resolved Hide resolved
}

std::string bloom_chain_metadata_bytes;
metadata.Encode(&bloom_chain_metadata_bytes);
batch->Put(metadata_cf_handle_, ns_key, bloom_chain_metadata_bytes);

batch->Put(bf_key_list.back(), bf_data_list.back());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add a "modified bf indices", rather than this. Assume we trigger "scaling", the logic here would be trickey, because it only put the final bf_data_list. And when no data has been added, here will also write a batch.

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but I wonder would we produce some flacky test if random string is used?

tests/gocase/unit/type/bloom/bloom_test.go Outdated Show resolved Hide resolved
@PragmaTwice PragmaTwice requested a review from mapleFU September 19, 2023 05:32
@PragmaTwice PragmaTwice merged commit 3092081 into apache:unstable Sep 19, 2023
26 checks passed
@PragmaTwice
Copy link
Member

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants