torwig commented on code in PR #1681:
URL: https://github.com/apache/kvrocks/pull/1681#discussion_r1298267829
##########
src/types/redis_list.cc:
##########
@@ -392,6 +392,67 @@ rocksdb::Status List::Range(const Slice &user_key, int
start, int stop, std::vec
return rocksdb::Status::OK();
}
+rocksdb::Status List::Pos(const Slice &user_key, const Slice &elem, const
PosSpec &spec,
+ std::vector<int64_t> *indexes) {
+ assert(spec.rank != 0);
Review Comment:
I'm not sure whether we need those assertions because our CI and tests are
using the `RelWithDebugInfo` build which doesn't see these checks.
##########
src/types/redis_list.cc:
##########
@@ -392,6 +392,67 @@ rocksdb::Status List::Range(const Slice &user_key, int
start, int stop, std::vec
return rocksdb::Status::OK();
}
+rocksdb::Status List::Pos(const Slice &user_key, const Slice &elem, const
PosSpec &spec,
+ std::vector<int64_t> *indexes) {
+ assert(spec.rank != 0);
+ assert(!spec.count.has_value() || spec.count >= 0);
+ assert(spec.max_len >= 0);
+ indexes->clear();
+
+ std::string ns_key = AppendNamespacePrefix(user_key);
+ ListMetadata metadata(false);
+ rocksdb::Status s = GetMetadata(ns_key, &metadata);
+ if (!s.ok()) return s;
+
+ // A negative rank means start from the tail.
+ int64_t rank = spec.rank;
+ uint64_t start = metadata.head;
+ bool reversed = false;
+ if (rank < 0) {
+ rank = -rank;
+ start = metadata.tail - 1;
+ reversed = true;
+ }
+
+ std::string buf;
+ PutFixed64(&buf, start);
+ std::string start_key = InternalKey(ns_key, buf, metadata.version,
storage_->IsSlotIdEncoded()).Encode();
+ std::string prefix = InternalKey(ns_key, "", metadata.version,
storage_->IsSlotIdEncoded()).Encode();
+ std::string next_version_prefix = InternalKey(ns_key, "", metadata.version +
1, storage_->IsSlotIdEncoded()).Encode();
+
+ std::vector<uint64_t> to_delete_indexes;
Review Comment:
I see that this variable is not used.
##########
src/types/redis_list.cc:
##########
@@ -392,6 +392,67 @@ rocksdb::Status List::Range(const Slice &user_key, int
start, int stop, std::vec
return rocksdb::Status::OK();
}
+rocksdb::Status List::Pos(const Slice &user_key, const Slice &elem, const
PosSpec &spec,
+ std::vector<int64_t> *indexes) {
+ assert(spec.rank != 0);
+ assert(!spec.count.has_value() || spec.count >= 0);
+ assert(spec.max_len >= 0);
+ indexes->clear();
+
+ std::string ns_key = AppendNamespacePrefix(user_key);
+ ListMetadata metadata(false);
+ rocksdb::Status s = GetMetadata(ns_key, &metadata);
+ if (!s.ok()) return s;
+
+ // A negative rank means start from the tail.
+ int64_t rank = spec.rank;
+ uint64_t start = metadata.head;
+ bool reversed = false;
+ if (rank < 0) {
+ rank = -rank;
+ start = metadata.tail - 1;
+ reversed = true;
+ }
+
+ std::string buf;
+ PutFixed64(&buf, start);
+ std::string start_key = InternalKey(ns_key, buf, metadata.version,
storage_->IsSlotIdEncoded()).Encode();
+ std::string prefix = InternalKey(ns_key, "", metadata.version,
storage_->IsSlotIdEncoded()).Encode();
+ std::string next_version_prefix = InternalKey(ns_key, "", metadata.version +
1, storage_->IsSlotIdEncoded()).Encode();
+
+ std::vector<uint64_t> to_delete_indexes;
+ rocksdb::ReadOptions read_options = storage_->DefaultScanOptions();
+ LatestSnapShot ss(storage_);
+ read_options.snapshot = ss.GetSnapShot();
+ rocksdb::Slice upper_bound(next_version_prefix);
+ read_options.iterate_upper_bound = &upper_bound;
+ rocksdb::Slice lower_bound(prefix);
+ read_options.iterate_lower_bound = &lower_bound;
+
+ auto list_len = static_cast<int64_t>(metadata.size);
+ int64_t max_len = spec.max_len;
+ int64_t count = spec.count.value_or(-1);
+ int64_t index = 0, matches = 0;
+
+ auto iter = util::UniqueIterator(storage_, read_options);
+ iter->Seek(start_key);
+ while (iter->Valid() && iter->key().starts_with(prefix) && (max_len == 0 ||
index < max_len)) {
+ if (iter->value() == elem) {
+ matches++;
+ if (matches >= rank) {
+ int64_t pos = !reversed ? index : list_len - index - 1;
+ indexes->push_back(pos);
+ if (count && matches - rank + 1 >= count) {
Review Comment:
I would recommend explicitly comparing the `count` variable with `0` because
its type is `int64_t` not `std::optional`.
##########
src/types/redis_list.cc:
##########
@@ -392,6 +392,67 @@ rocksdb::Status List::Range(const Slice &user_key, int
start, int stop, std::vec
return rocksdb::Status::OK();
}
+rocksdb::Status List::Pos(const Slice &user_key, const Slice &elem, const
PosSpec &spec,
+ std::vector<int64_t> *indexes) {
+ assert(spec.rank != 0);
+ assert(!spec.count.has_value() || spec.count >= 0);
+ assert(spec.max_len >= 0);
+ indexes->clear();
+
+ std::string ns_key = AppendNamespacePrefix(user_key);
+ ListMetadata metadata(false);
+ rocksdb::Status s = GetMetadata(ns_key, &metadata);
+ if (!s.ok()) return s;
+
+ // A negative rank means start from the tail.
+ int64_t rank = spec.rank;
+ uint64_t start = metadata.head;
+ bool reversed = false;
+ if (rank < 0) {
+ rank = -rank;
+ start = metadata.tail - 1;
+ reversed = true;
+ }
+
+ std::string buf;
+ PutFixed64(&buf, start);
+ std::string start_key = InternalKey(ns_key, buf, metadata.version,
storage_->IsSlotIdEncoded()).Encode();
+ std::string prefix = InternalKey(ns_key, "", metadata.version,
storage_->IsSlotIdEncoded()).Encode();
+ std::string next_version_prefix = InternalKey(ns_key, "", metadata.version +
1, storage_->IsSlotIdEncoded()).Encode();
+
+ std::vector<uint64_t> to_delete_indexes;
+ rocksdb::ReadOptions read_options = storage_->DefaultScanOptions();
+ LatestSnapShot ss(storage_);
+ read_options.snapshot = ss.GetSnapShot();
+ rocksdb::Slice upper_bound(next_version_prefix);
+ read_options.iterate_upper_bound = &upper_bound;
+ rocksdb::Slice lower_bound(prefix);
+ read_options.iterate_lower_bound = &lower_bound;
+
+ auto list_len = static_cast<int64_t>(metadata.size);
+ int64_t max_len = spec.max_len;
+ int64_t count = spec.count.value_or(-1);
+ int64_t index = 0, matches = 0;
Review Comment:
I would rename `index` to `offset` (or something similar) because if the
`rank` is negative this variable doesn't hold the exact index of the element.
##########
src/commands/cmd_list.cc:
##########
@@ -680,23 +681,88 @@ class CommandBLMove : public Commander,
void unblockOnSrc() { svr_->UnblockOnKey(args_[1], conn_); }
};
-REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandBLPop>("blpop", -3, "write
no-script", 1, -2, 1),
- MakeCmdAttr<CommandBRPop>("brpop", -3, "write
no-script", 1, -2, 1),
- MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only",
1, 1, 1),
- MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1,
1, 1),
- MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1,
1),
- MakeCmdAttr<CommandLMove>("lmove", 5, "write", 1, 2,
1),
- MakeCmdAttr<CommandBLMove>("blmove", 6, "write", 1, 2,
1),
- MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1,
1), //
- MakeCmdAttr<CommandLPush>("lpush", -3, "write", 1, 1,
1),
- MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1,
1, 1),
- MakeCmdAttr<CommandLRange>("lrange", 4, "read-only",
1, 1, 1),
- MakeCmdAttr<CommandLRem>("lrem", 4, "write", 1, 1, 1),
- MakeCmdAttr<CommandLSet>("lset", 4, "write", 1, 1, 1),
- MakeCmdAttr<CommandLTrim>("ltrim", 4, "write", 1, 1,
1),
- MakeCmdAttr<CommandRPop>("rpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPopLPUSH>("rpoplpush", 3, "write",
1, 2, 1),
- MakeCmdAttr<CommandRPush>("rpush", -3, "write", 1, 1,
1),
- MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1,
1, 1), )
+class CommandLPOS : public Commander {
Review Comment:
I think we can name the command `CommandLPos` without capitalizing `POS`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
