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.
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: support count min sketch data structure and most commands #2524
base: unstable
Are you sure you want to change the base?
feat: support count min sketch data structure and most commands #2524
Changes from 37 commits
c9626df
bb48c5b
b4359b5
a1b904a
8cab9c9
ecbaa44
bf41c60
3fb9f1f
a92dc08
e59e2b7
b8b0bf2
dd1c4f5
509d5ab
f19e364
c1c1e7f
f21792c
1e4e747
a71d7f7
202a2f5
7743f7b
ede5481
551f3c8
a0222d7
9a2fe26
de0c2ad
69d6ea4
db73151
0cdfb07
02347df
606f9b5
0445b3d
48e9bc2
f577146
ab277e9
f2f1288
ca60e13
df7a7b2
e64056f
bc81447
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@git-hulk @PragmaTwice
I'm thinking about handling the
array
. Since regarding the cmsketch as a string is ok to me, but parsing it to anstd::vector
is a bit weird to me. Can we take aBuffer
and do zero copying when doing this?Like:
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.
Yeah it can be like how we handle the JSON data structure.
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.
But here I'm more interested in whether it's better to be a single key or putting the array in subkeys.
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.
Actually I think this in single key is ok since we merely "only query metadata" if not calling "info"
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.
What's the limit of the size of such an array? Do we need to consider to split it?
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.
The default engine doesn't handle this well. There're some blog engine which could do this, and we can enable the blob in kvrocks here.
Perhaps we can limit the size to 1MB firstly? 🤔
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.
Looks good to me.
Also yeah we should not put the array inside the metadata.
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.
I may try a round tonight
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.
I'll adjust it to 1MB limit
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 do we want to hold the array in a buffer on storage, and do operations on that?