Add support of the SORT command #2262
Conversation
Actually I'm thinking to specify a special command tag for such commands that loads all of the data structure, like this one, and HRANDFIELD.. |
There was a problem hiding this comment.
The implementation looks generally good to me.
See if @mapleFU or others have any concern.
| /// If `args.alpha` is false, `RedisSortObject.v` will be taken as double for comparison | ||
| /// If `args.alpha` is true and `args.sortby` is not empty, `RedisSortObject.v` will be taken as string for comparison | ||
| /// If `args.alpha` is true and `args.sortby` is empty, the comparison is by `RedisSortObject.obj`. |
There was a problem hiding this comment.
So why do here has std::variant and a std::string? We should use only variant or double + string composite?
There was a problem hiding this comment.
I think it is feasible to only use std::variant<std::string, double>, but this may require distinguishing between two types of std::string: BY(RedisSortObject.v) and the sort key itself(RedisSortObject.obj), resulting in unclear sorting logic.
There was a problem hiding this comment.
Sigh, I think maybe struct or other is ok, since the passed in argument ( RedisSortObject ) is defined by args. It cannot have another state ( like holding a string when !args.alpha ), and this introduce a double-checking here.
std::variant seems has same space usage as struct {double; string;}. But both is ok for me
Co-authored-by: mwish <maplewish117@gmail.com>
|
Will merge in 1-days if no negative comments |
|
Add support of the SORT command (apache#2262) Co-authored-by: 纪华裕 <jihuayu123@gmail.com> Co-authored-by: hulk <hulk.website@gmail.com> Co-authored-by: mwish <maplewish117@gmail.com>
issues: #1952