chore: add tdigest commands #5110
Conversation
Signed-off-by: kostas <[email protected]>
| } | ||
|
|
||
| auto cb = [key, compression](Transaction* tx, EngineShard* es) -> OpResult<bool> { | ||
| auto& db_slice = tx->GetDbSlice(es->shard_id()); |
There was a problem hiding this comment.
Let's move the callback implementation from each command to Op(CommandName), as we do for other data types (e.g., json_family). I believe it's not a good approach to keep all the logic inside these static command functions, as it significantly reduces readability.
There was a problem hiding this comment.
I won't even bother for now, this is sketch -- and we do have callback within functions in other places.
In general I agree, we should keep those functions as small as possible. I am happy to polish if we ever actually merge any of this functionality upstream.
| if (!it) { | ||
| return it.status(); | ||
| } | ||
| auto* wrapper = it->it->second.GetRobjWrapper(); |
There was a problem hiding this comment.
I suggest adding a GetTdHistogram method to CompactObj, similar to the existing GetJson method, since these two lines are repeated across multiple T-Digest methods
There was a problem hiding this comment.
+1 I will add it in a follow up PR. not hat important for now
| return cmd_cntx.rb->SendError(res.status()); | ||
| } | ||
|
|
||
| double TdGetByRank(td_histogram_t* td, double total_obs, double rnk) { |
There was a problem hiding this comment.
Please move this to the anonymous namespace, along with the other methods
There was a problem hiding this comment.
same comment as above, happy to polish one we actually consider this work for merging. I will move all those to unamed namespaces after I get tests+functionility done
| const double max = td_max(td); | ||
|
|
||
| ByRankResult result; | ||
| for (auto rnk : ranks) { |
There was a problem hiding this comment.
please do result.reserve(ranks.size())
| rb->SendLong(res->mem_usage); | ||
| } | ||
|
|
||
| struct MinMax { |
There was a problem hiding this comment.
using MinMax = std::pair
There was a problem hiding this comment.
I explicitly wanted a struct here toi avoid first and second
| std::vector<size_t> ranks; | ||
| while (parser.HasNext()) { | ||
| double val = parser.Next<double>(); | ||
| ranks.push_back(val); |
There was a problem hiding this comment.
ranks is vector of size_t, and val is double. Something is wrong here.
There was a problem hiding this comment.
I think the internal implementation converts to int. I will check all of these in detail once I add tests
| const double max = td_max(td); | ||
|
|
||
| RankResult result; | ||
|
|
There was a problem hiding this comment.
please do reserve
| std::vector<size_t> values; | ||
| while (parser.HasNext()) { | ||
| double val = parser.Next<double>(); | ||
| values.push_back(val); |
| ; | ||
| auto* td = (td_histogram_t*)wrapper->inner_obj(); | ||
| Result result; | ||
| for (auto val : values) { |
The following commands are implemented: