feat(zset_family): support WITHSCORE in zrevrank/zrank commands (#3921) #4001
Conversation
|
Thank you @Diskein 🙏🏼 , will take a look later this week. |
Diskein
force-pushed
the
zrank-withscore-no-templates
branch
from
527eaef to
e3f8608
Compare
src/core/sorted_map.cc
Outdated
| if (obj == nullptr) | ||
| return std::nullopt; | ||
|
|
||
| optional rank = score_tree->GetRank(obj); |
There was a problem hiding this comment.
you can pass reverse into GetRank to avoid copying the reversing logic below.
Similarly DCHECK is redundant as is it is checked by GetRank
There was a problem hiding this comment.
Yep, I've added the 'reverse' param to ScoreTree::GetRank
| @@ -1979,17 +2000,35 @@ void ZRangeGeneric(CmdArgList args, Transaction* tx, SinkReplyBuilder* builder, | |||
| } | |||
|
|
|||
| void ZRankGeneric(CmdArgList args, bool reverse, Transaction* tx, SinkReplyBuilder* builder) { | |||
| if (args.size() > 3) { | |||
There was a problem hiding this comment.
would you like to try replacing this parsing logic with facade::CmdArgParser ?
we usually replace the old parsing code with this as it's more clear this way.
There was a problem hiding this comment.
It turns out that redis does this check first. so i left this check in favor to throw the same error messages.
Providing this check by using the parser isn't obvious.
I tried to write a clean code by using parser for that and failed =D
Diskein
force-pushed
the
zrank-withscore-no-templates
branch
from
e3f8608 to
6a7b8e6
Compare
| DCHECK(rank); | ||
| return reverse ? score_map->UpperBoundSize() - *rank - 1 : *rank; |
There was a problem hiding this comment.
why was UpperBoundSize() used here? It isn't obvious, perhaps something slipped away from me.
IMHO: I'd like to keep DCHECK. It's true that score_map->GetRank shouldn't return std::nullopt in this codepath. It can do it in general though. So it wouldn't hurt.
There was a problem hiding this comment.
dense_set has the ability to expire items but it does not do it proactively, so its size method may include items that are already expired. We do not use the feature in sorted set though.
…onflydb#3921) Signed-off-by: Diskein <[email protected]>
Diskein
force-pushed
the
zrank-withscore-no-templates
branch
from
6a7b8e6 to
d27e00b
Compare
|
Looks good, thank you very much! |
Hey, it adds WITHSCORE support to zrank/zrevrank.
I actually wrote another one version with templated RankResult and constexpr'ed ifs, but i couldn't see performance differences with a simple 'boolean and if' solution. Since the other commands implemented with the last approach I've decided to go this way.
Another change it a new method in SortedMap. It could be implemented without this invention but it helps to avoid double lookup in the 'score_map'. The method returns std::pair which doesn't have self-explanatory fields with its 'first' and 'second' but I've hesitated to add a new struct for this purpose.