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.MEXISTS and BF.CARD Command #1756

Merged
merged 10 commits into from
Sep 14, 2023

Conversation

zncleon
Copy link
Contributor

@zncleon zncleon commented Sep 12, 2023

This PR introduce the bloom BF.MEXISTS and BF.CARD command.

[1] https://redis.io/commands/bf.mexists/
[2] https://redis.io/commands/bf.card/

src/commands/cmd_bloom_filter.cc Show resolved Hide resolved
src/types/redis_bloom_chain.cc Outdated Show resolved Hide resolved
@PragmaTwice PragmaTwice requested a review from mapleFU September 13, 2023 04:20
@PragmaTwice PragmaTwice linked an issue Sep 13, 2023 that may be closed by this pull request
11 tasks
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 looks ok to me

src/types/redis_bloom_chain.cc Outdated Show resolved Hide resolved
src/types/redis_bloom_chain.cc Outdated Show resolved Hide resolved
tests/gocase/unit/type/bloom/bloom_test.go Show resolved Hide resolved
@@ -40,6 +40,15 @@ std::string Integer(T data) {
return ":" + std::to_string(data) + CRLF;
}

template <typename T, std::enable_if_t<std::is_integral_v<T>, int> = 0>
std::string MultiInteger(const std::vector<T> &multi_data) {
Copy link
Member

Choose a reason for hiding this comment

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

A quick question: would other Redis response can be help with MultiInteger?

Copy link
Member

@mapleFU mapleFU Sep 13, 2023

Choose a reason for hiding this comment

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

(Oh and @PragmaTwice and can we support a vector<bool> version to convert vector<bool> to 0/1 response?)

Copy link
Member

Choose a reason for hiding this comment

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

not sure since resp3 supports boolean.

Copy link
Contributor Author

@zncleon zncleon Sep 13, 2023

Choose a reason for hiding this comment

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

It's seems only here(BF.MExists and BF.MAdd) and SMIsMember can be help with MultiInteger. Should we give up MultiInteger?

Copy link
Member

Choose a reason for hiding this comment

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

You can put it in src/commands/cmd_bloom_filter.cc in anonymous namespace as a helper first. If you find any other user would use it, you can move it out.

Copy link
Member

@PragmaTwice PragmaTwice Sep 14, 2023

Choose a reason for hiding this comment

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

this function looks ok to me.

mapleFU
mapleFU previously approved these changes Sep 13, 2023
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.

LGTM

Co-authored-by: Twice <twice@apache.org>
@mapleFU mapleFU merged commit 83aaa75 into apache:unstable Sep 14, 2023
26 checks passed
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