feat(tdigest): add tdigest algorithm and storage encoding implementations #2741
Conversation
|
Hi Team, Sorry for delaying this feature's implementation. I kept the commit log since we will squash merging. Please give me some suggestions and help take a review. There are still many parts need to be improved, and I will be very happy to try my best for any improvement. 😊 Best Regards, |
PragmaTwice
changed the title
LindaSummer
force-pushed
the
feature/tdigest-for-first-pr
branch
from
26d8348 to
f0f2f3d
Compare
src/storage/redis_metadata.h
Outdated
| uint64_t total_observations = 0; | ||
| uint64_t merge_times = 0; |
There was a problem hiding this comment.
We can add a comment for that one
src/types/redis_tdigest.cc
Outdated
| return iter_ != centroids_.cend(); | ||
| } | ||
| bool Prev() { | ||
| if (Valid()) { |
There was a problem hiding this comment.
assume it's at end, what's it expected behavior here?
There was a problem hiding this comment.
Hi @mapleFU ,
At the beginning of tdigest centroids iteration, we have confirmed the value must be inside the range of (min_, max_).
So, if iteration reaches the cend(), it means that the precheck logic doesn't cover all cases or the centroids may be corrupted.
Best Regards,
Edward
src/types/redis_tdigest.cc
Outdated
| if (status.ok()) { | ||
| return {}; | ||
| } | ||
|
|
||
| if (!status.IsNotFound()) { | ||
| return status; | ||
| } |
There was a problem hiding this comment.
Can we merge these two lines together?
There was a problem hiding this comment.
Hi @mapleFU ,
Thanks for your suggestion! 😊
We need to return both status and the existence of same tdigest before creation.
I have refactored the std::optional with an explicit flag exists with docstring.
Best Regards,
Edward
|
Hi @mapleFU , Sorry to interrupt you again. 😊 Have a nice day! Best Regards, |
| } | ||
| return iter_ != centroids_.cend(); | ||
| } | ||
| bool Prev() { |
There was a problem hiding this comment.
Can you add a comment to explain the Prev() requires to be valid? This function call is ok to be but a bit weird. Since if the iter is the cend(), for std::vector, it's not "Valid" but it prev is valid?
I've read the caller in tdigest.cc, this interface is ok since it prevent to call Prev() if !Valid(), but this requires some comment
There was a problem hiding this comment.
Hi @mapleFU ,
Thanks for your suggestion! 😊
I have added comment for this function.
Best Regards,
Edward
src/types/redis_tdigest.cc
Outdated
|
|
||
| namespace redis { | ||
|
|
||
| // It should be replaced by a iteration of the rocksdb iterator |
There was a problem hiding this comment.
Can you add a todo and create an issue for this?
src/types/redis_tdigest.cc
Outdated
| InternalKey ikey(key, storage_->IsSlotIdEncoded()); | ||
| auto subkey = ikey.GetSubKey(); | ||
| auto type_flg = static_cast<uint8_t>(SegmentType::kGuardFlag); | ||
| GetFixed8(&subkey, &type_flg); |
There was a problem hiding this comment.
Have we checked the return value of Get{} api here?
There was a problem hiding this comment.
Hi @mapleFU , Thanks very much for your warm help and patience! 😊 Best Regards, |
|
Let's merge first and then add lots of go tests |
|
|
Thank you for your contribution! |
Issue
Proposal
Proposed Changes
create,addandquantile.create,addandquantile.TODO List
DummyCentroidsto an iteration way for efficiency.