[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1116: feat: add support for Redis sintercard command

[GitBox <gi...@apache.org>]

torwig commented on code in PR #1116:
URL: https://github.com/apache/incubator-kvrocks/pull/1116#discussion_r1020872184


##########
src/types/redis_set.cc:
##########
@@ -133,6 +133,17 @@ rocksdb::Status Set::Card(const Slice &user_key, int *ret) {
   return rocksdb::Status::OK();
 }
 
+rocksdb::Status Set::InterCard(const std::vector<Slice> &keys, int64_t limit, int64_t *ret) {
+  std::vector<std::string> members;
+  rocksdb::Status s = Inter(keys, &members);
+  if (!s.ok()) return s;
+  *ret = members.size();
+  if (limit > 0 && *ret > limit) {

Review Comment:
   The Redis docs states:
   
   `By default, the command calculates the cardinality of the intersection of all given sets. When provided with the optional LIMIT argument (which defaults to 0 and means unlimited), if the intersection cardinality reaches limit partway through the computation, the algorithm will exit and yield limit as the cardinality. Such implementation ensures a significant speedup for queries where the limit is lower than the actual intersection cardinality.`
   
   I think, your implementation doesn't speed up the process of intersection, it performs an intersection as usual (full) and adjusts the result by comparing it with the `limit` value. In order to truly use the `limit` value, you should use it somewhere inside `Inter()`, passing it as a parameter or introducing a completely separate method.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org