You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "julic20s (via GitHub)" <gi...@apache.org> on 2023/11/20 16:46:37 UTC
[PR] Support for the BITFIELD command [kvrocks]
julic20s opened a new pull request, #1901:
URL: https://github.com/apache/kvrocks/pull/1901
(no comment)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1403482350
##########
src/storage/batch_extractor.cc:
##########
@@ -232,6 +232,10 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t column_family_id, const Slic
first_seen_ = false;
}
break;
+ case kRedisCmdBitfield:
Review Comment:
Could you tell me how to run only one test instead of all go test? I wrote the tests and I tried `./x.py test go build redis-cli -run <test function name>`, but no effect. Thanks.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1820366326
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [20 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1401930809
##########
src/types/redis_bitmap.cc:
##########
@@ -538,6 +545,281 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+namespace {
+using SegmentCache = std::pair<std::string, std::string>;
+
+// used to simplify accessing bitfield that occupies multiple continuous segments.
+// you can access multiple segments like accessing in one big segments.
+class BitmapSegmentsView {
+ public:
+ BitmapSegmentsView() = default;
+ explicit BitmapSegmentsView(std::vector<SegmentCache *> segments, bool expand_in_advance = true)
+ : segments_(std::move(segments)) {
+ if (expand_in_advance && segments_.size() > 1) {
+ // expand segments during iteration (not in advance) maybe increase allocation times.
+ // we assume that all end of each segment except last one will be accessed.
+ // note: segments_.size() is unsigned.
+ for (size_t i = 0; i < segments_.size() - 1; ++i) {
+ ExpandBitmapSegment(&segments_[i]->second, kBitmapSegmentBytes);
+ }
+ }
+ }
+
+ char &operator[](uint32_t index) noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ const char &operator[](uint32_t index) const noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ SegmentCache &GetSegment(uint32_t index) noexcept { return *segments_[index]; }
+
+ const SegmentCache &GetSegment(uint32_t index) const noexcept { return *segments_[index]; }
+
+ size_t LastSegmentSize() const noexcept { return segments_.back()->second.size(); }
+
+ size_t SegmentsCount() const noexcept { return segments_.size(); }
+
+ auto SegmentBegin() noexcept { return segments_.begin(); }
+
+ auto SegmentEnd() noexcept { return segments_.end(); }
+
+ private:
+ std::vector<SegmentCache *> segments_;
+};
+} // namespace
+
+template <bool ReadOnly>
+rocksdb::Status Bitmap::bitfield(const Slice &user_key, const std::vector<BitfieldOperation> &ops,
+ std::vector<std::string> *rets) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
+
+ LockGuard guard;
+ if constexpr (!ReadOnly) {
+ guard = LockGuard(storage_->GetLockManager(), ns_key);
+ }
+
+ BitmapMetadata metadata;
+ std::string raw_value;
+ auto s = GetMetadata(ns_key, &metadata, &raw_value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+
+ if (metadata.Type() == RedisType::kRedisString) {
+ if constexpr (ReadOnly) {
+ s = BitmapString::BitfieldReadOnly(ns_key, raw_value, ops, rets);
+ } else {
+ s = BitmapString(storage_, namespace_).Bitfield(ns_key, &raw_value, ops, rets);
+ }
+ return s;
+ }
+
+ if (metadata.Type() != RedisType::kRedisBitmap) {
+ return rocksdb::Status::InvalidArgument("The value is not a bitmap or string.");
+ }
+
+ std::unordered_map<uint32_t, SegmentCache> seg_cache;
+ auto get_segment = [&](uint32_t index, SegmentCache **res) {
+ auto [seg_itor, no_cache] = seg_cache.try_emplace(index);
+ auto &[sub_key, value] = seg_itor->second;
+ if (no_cache) {
+ auto sub_sub_key = std::to_string(index * kBitmapSegmentBytes);
+ sub_key = InternalKey(ns_key, sub_sub_key, metadata.version, storage_->IsSlotIdEncoded()).Encode();
+ rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(), sub_key, &value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+ }
+ *res = &seg_itor->second;
+ return rocksdb::Status::OK();
+ };
+
+ auto init_view = [&get_segment](uint32_t offset, uint32_t bits, BitmapSegmentsView *view) {
+ uint32_t first_segment = offset / kBitmapSegmentBits;
+ uint32_t last_segment = (offset + bits - 1) / kBitmapSegmentBits;
+
+ std::vector<SegmentCache *> segments(last_segment - first_segment + 1);
+ for (auto &seg : segments) {
+ auto seg_status = get_segment(first_segment++, &seg);
+ if (!seg_status.ok()) {
+ return seg_status;
+ }
+ }
+ *view = BitmapSegmentsView(std::move(segments));
+ return rocksdb::Status::OK();
+ };
+
+ // get bitfield value from entire bitmap
+ auto get = [&init_view](uint32_t offset, BitfieldEncoding enc, uint64_t *res) {
+ BitmapSegmentsView view;
+ auto view_status = init_view(offset, enc.Bits(), &view);
+ if (!view_status.ok()) {
+ return view_status;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ if (enc.IsSigned()) {
+ *res = GetSignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ } else {
+ *res = GetUnsignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ if constexpr (ReadOnly) {
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ return rocksdb::Status::InvalidArgument("Write bitfield in read-only mode.");
+ }
+
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_old_value));
+ } else {
+ ret = redis::Integer(unsigned_old_value);
+ }
+
+ rets->push_back(std::move(ret));
+ }
+ return rocksdb::Status::OK();
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ bitfieldWriteAheadLog(batch, ops);
+
+ // set bitfield value among segments
+ auto set = [&](uint32_t offset, BitfieldEncoding enc, BitfieldOverflowBehavior overflow, uint64_t value) {
+ BitmapSegmentsView view;
+ s = init_view(offset, enc.Bits(), &view);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ SetBitfield(view, first_bit_segment_offset, enc.Bits(), value);
+
+ for (auto segment = view.SegmentBegin(); segment != view.SegmentEnd(); ++segment) {
+ batch->Put((*segment)->first, (*segment)->second);
+ }
+
+ uint64_t segments_cnt = view.SegmentsCount();
+ uint64_t used_size = (offset / 8) + (segments_cnt - 1) * kBitmapSegmentBytes + view.LastSegmentSize();
+ if (metadata.size < used_size) {
+ metadata.size = used_size;
+ std::string bytes;
+ metadata.Encode(&bytes);
+ batch->Put(metadata_cf_handle_, ns_key, bytes);
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint64_t unsigned_new_value = 0;
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value)) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ s = set(op.offset, op.encoding, op.overflow, unsigned_new_value);
+ if (!s.ok()) {
+ return s;
+ }
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_new_value));
+ } else {
+ ret = redis::Integer(unsigned_new_value);
+ }
+ } else {
+ ret = redis::NilString();
+ }
Review Comment:
Maybe it can be `std::optional<struct { uint64_t v; bool signed; }>`, if you are willing to change it.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1837474580
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1400125723
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <stdexcept>
+#include <cstdint>
+#include <limits>
+#include <type_traits>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+struct BitfieldEncoding {
+ bool is_signed;
+ // 1 ~ 63 if unsigned, 1 ~ 64 if signed.
Review Comment:
Should we `static_assert(sizeof(...))` for this?
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <stdexcept>
+#include <cstdint>
+#include <limits>
+#include <type_traits>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+struct BitfieldEncoding {
+ bool is_signed;
+ // 1 ~ 63 if unsigned, 1 ~ 64 if signed.
+ uint8_t bits;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding;
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+
+// safe cast from unsigned to signed.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+union SafeInt64 {
Review Comment:
Would we better just using `uint64_t` and maintaining a group of `SafeOp` rather than `SafeInt64`?
##########
src/types/redis_bitmap.cc:
##########
@@ -538,6 +545,302 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+namespace {
+union IntResult {
Review Comment:
ditto. Seems this has duplicate IntResult...
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1820388074
@julic20s Can use `./x.py format` to format your code before pushing.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1820866701
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1400839029
##########
src/types/redis_bitmap.cc:
##########
@@ -538,6 +545,281 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+namespace {
+using SegmentCache = std::pair<std::string, std::string>;
+
+// used to simplify accessing bitfield that occupies multiple continuous segments.
+// you can access multiple segments like accessing in one big segments.
+class BitmapSegmentsView {
+ public:
+ BitmapSegmentsView() = default;
+ explicit BitmapSegmentsView(std::vector<SegmentCache *> segments, bool expand_in_advance = true)
+ : segments_(std::move(segments)) {
+ if (expand_in_advance && segments_.size() > 1) {
+ // expand segments during iteration (not in advance) maybe increase allocation times.
+ // we assume that all end of each segment except last one will be accessed.
+ // note: segments_.size() is unsigned.
+ for (size_t i = 0; i < segments_.size() - 1; ++i) {
+ ExpandBitmapSegment(&segments_[i]->second, kBitmapSegmentBytes);
+ }
+ }
+ }
+
+ char &operator[](uint32_t index) noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ const char &operator[](uint32_t index) const noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ SegmentCache &GetSegment(uint32_t index) noexcept { return *segments_[index]; }
+
+ const SegmentCache &GetSegment(uint32_t index) const noexcept { return *segments_[index]; }
+
+ size_t LastSegmentSize() const noexcept { return segments_.back()->second.size(); }
+
+ size_t SegmentsCount() const noexcept { return segments_.size(); }
+
+ auto SegmentBegin() noexcept { return segments_.begin(); }
+
+ auto SegmentEnd() noexcept { return segments_.end(); }
+
+ private:
+ std::vector<SegmentCache *> segments_;
+};
+} // namespace
+
+template <bool ReadOnly>
+rocksdb::Status Bitmap::bitfield(const Slice &user_key, const std::vector<BitfieldOperation> &ops,
+ std::vector<std::string> *rets) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
+
+ LockGuard guard;
+ if constexpr (!ReadOnly) {
+ guard = LockGuard(storage_->GetLockManager(), ns_key);
+ }
+
+ BitmapMetadata metadata;
+ std::string raw_value;
+ auto s = GetMetadata(ns_key, &metadata, &raw_value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+
+ if (metadata.Type() == RedisType::kRedisString) {
+ if constexpr (ReadOnly) {
+ s = BitmapString::BitfieldReadOnly(ns_key, raw_value, ops, rets);
+ } else {
+ s = BitmapString(storage_, namespace_).Bitfield(ns_key, &raw_value, ops, rets);
+ }
+ return s;
+ }
+
+ if (metadata.Type() != RedisType::kRedisBitmap) {
+ return rocksdb::Status::InvalidArgument("The value is not a bitmap or string.");
+ }
+
+ std::unordered_map<uint32_t, SegmentCache> seg_cache;
+ auto get_segment = [&](uint32_t index, SegmentCache **res) {
+ auto [seg_itor, no_cache] = seg_cache.try_emplace(index);
+ auto &[sub_key, value] = seg_itor->second;
+ if (no_cache) {
+ auto sub_sub_key = std::to_string(index * kBitmapSegmentBytes);
+ sub_key = InternalKey(ns_key, sub_sub_key, metadata.version, storage_->IsSlotIdEncoded()).Encode();
+ rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(), sub_key, &value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+ }
+ *res = &seg_itor->second;
+ return rocksdb::Status::OK();
+ };
+
+ auto init_view = [&get_segment](uint32_t offset, uint32_t bits, BitmapSegmentsView *view) {
+ uint32_t first_segment = offset / kBitmapSegmentBits;
+ uint32_t last_segment = (offset + bits - 1) / kBitmapSegmentBits;
+
+ std::vector<SegmentCache *> segments(last_segment - first_segment + 1);
+ for (auto &seg : segments) {
+ auto seg_status = get_segment(first_segment++, &seg);
+ if (!seg_status.ok()) {
+ return seg_status;
+ }
+ }
+ *view = BitmapSegmentsView(std::move(segments));
+ return rocksdb::Status::OK();
+ };
+
+ // get bitfield value from entire bitmap
+ auto get = [&init_view](uint32_t offset, BitfieldEncoding enc, uint64_t *res) {
+ BitmapSegmentsView view;
+ auto view_status = init_view(offset, enc.Bits(), &view);
+ if (!view_status.ok()) {
+ return view_status;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ if (enc.IsSigned()) {
+ *res = GetSignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ } else {
+ *res = GetUnsignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ if constexpr (ReadOnly) {
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ return rocksdb::Status::InvalidArgument("Write bitfield in read-only mode.");
+ }
+
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_old_value));
+ } else {
+ ret = redis::Integer(unsigned_old_value);
+ }
+
+ rets->push_back(std::move(ret));
+ }
+ return rocksdb::Status::OK();
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ bitfieldWriteAheadLog(batch, ops);
+
+ // set bitfield value among segments
+ auto set = [&](uint32_t offset, BitfieldEncoding enc, BitfieldOverflowBehavior overflow, uint64_t value) {
+ BitmapSegmentsView view;
+ s = init_view(offset, enc.Bits(), &view);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ SetBitfield(view, first_bit_segment_offset, enc.Bits(), value);
+
+ for (auto segment = view.SegmentBegin(); segment != view.SegmentEnd(); ++segment) {
+ batch->Put((*segment)->first, (*segment)->second);
+ }
+
+ uint64_t segments_cnt = view.SegmentsCount();
+ uint64_t used_size = (offset / 8) + (segments_cnt - 1) * kBitmapSegmentBytes + view.LastSegmentSize();
+ if (metadata.size < used_size) {
+ metadata.size = used_size;
+ std::string bytes;
+ metadata.Encode(&bytes);
+ batch->Put(metadata_cf_handle_, ns_key, bytes);
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint64_t unsigned_new_value = 0;
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value)) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ s = set(op.offset, op.encoding, op.overflow, unsigned_new_value);
+ if (!s.ok()) {
+ return s;
+ }
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_new_value));
+ } else {
+ ret = redis::Integer(unsigned_new_value);
+ }
+ } else {
+ ret = redis::NilString();
+ }
Review Comment:
I think it would be better to return `vector<optional<integer>>` here rather than vector of RESP strings here, in case that we can reuse the result.
##########
src/types/redis_bitmap_string.cc:
##########
@@ -204,4 +205,113 @@ int64_t BitmapString::RawBitpos(const uint8_t *c, int64_t count, bool bit) {
return res;
}
+namespace {
+class ReadOnlyStringBitmapView {
+ public:
+ explicit ReadOnlyStringBitmapView(std::string_view str) noexcept : str_(str) {}
+
+ char operator[](uint32_t index) const noexcept {
+ if (index >= str_.size()) {
+ return 0;
+ }
+ return str_[index];
+ }
+
+ private:
+ std::string_view str_;
+};
+
+class ReadWriteStringBitmapView {
+ public:
+ explicit ReadWriteStringBitmapView(std::string *str) : str_(str) {}
+
+ char &operator[](uint32_t index) {
+ if (index >= str_->size()) {
+ str_->append((index + 1) - str_->size(), 0);
+ }
+ return (*str_)[index];
+ }
+
+ const char &operator[](uint32_t index) const {
+ if (index >= str_->size()) {
+ str_->append((index + 1) - str_->size(), 0);
+ }
+ return (*str_)[index];
+ }
+
+ private:
+ std::string *str_;
+};
+} // namespace
+
+rocksdb::Status BitmapString::Bitfield(const Slice &ns_key, std::string *raw_value,
+ const std::vector<BitfieldOperation> &ops, std::vector<std::string> *rets) {
+ auto header_offset = Metadata::GetOffsetAfterExpire((*raw_value)[0]);
+ std::string string_value = raw_value->substr(header_offset);
+ ReadWriteStringBitmapView view(&string_value);
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ uint64_t unsigned_old_value = 0;
+ if (op.encoding.IsSigned()) {
+ unsigned_old_value = GetSignedBitfield(view, op.offset, op.encoding.Bits());
+ } else {
+ unsigned_old_value = GetUnsignedBitfield(view, op.offset, op.encoding.Bits());
+ }
+
+ uint64_t unsigned_new_value = 0;
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value)) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ SetBitfield(view, op.offset, op.encoding.Bits(), unsigned_new_value);
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_new_value));
+ } else {
+ ret = redis::Integer(unsigned_new_value);
+ }
+ } else {
+ ret = redis::NilString();
+ }
Review Comment:
ditto
##########
src/types/redis_bitmap_string.cc:
##########
@@ -204,4 +205,113 @@ int64_t BitmapString::RawBitpos(const uint8_t *c, int64_t count, bool bit) {
return res;
}
+namespace {
+class ReadOnlyStringBitmapView {
+ public:
+ explicit ReadOnlyStringBitmapView(std::string_view str) noexcept : str_(str) {}
+
+ char operator[](uint32_t index) const noexcept {
+ if (index >= str_.size()) {
+ return 0;
+ }
+ return str_[index];
+ }
+
+ private:
+ std::string_view str_;
+};
+
+class ReadWriteStringBitmapView {
+ public:
+ explicit ReadWriteStringBitmapView(std::string *str) : str_(str) {}
+
+ char &operator[](uint32_t index) {
+ if (index >= str_->size()) {
+ str_->append((index + 1) - str_->size(), 0);
Review Comment:
```suggestion
str_->resize(index + 1);
```
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1826483875
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [15 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1401930809
##########
src/types/redis_bitmap.cc:
##########
@@ -538,6 +545,281 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+namespace {
+using SegmentCache = std::pair<std::string, std::string>;
+
+// used to simplify accessing bitfield that occupies multiple continuous segments.
+// you can access multiple segments like accessing in one big segments.
+class BitmapSegmentsView {
+ public:
+ BitmapSegmentsView() = default;
+ explicit BitmapSegmentsView(std::vector<SegmentCache *> segments, bool expand_in_advance = true)
+ : segments_(std::move(segments)) {
+ if (expand_in_advance && segments_.size() > 1) {
+ // expand segments during iteration (not in advance) maybe increase allocation times.
+ // we assume that all end of each segment except last one will be accessed.
+ // note: segments_.size() is unsigned.
+ for (size_t i = 0; i < segments_.size() - 1; ++i) {
+ ExpandBitmapSegment(&segments_[i]->second, kBitmapSegmentBytes);
+ }
+ }
+ }
+
+ char &operator[](uint32_t index) noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ const char &operator[](uint32_t index) const noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ SegmentCache &GetSegment(uint32_t index) noexcept { return *segments_[index]; }
+
+ const SegmentCache &GetSegment(uint32_t index) const noexcept { return *segments_[index]; }
+
+ size_t LastSegmentSize() const noexcept { return segments_.back()->second.size(); }
+
+ size_t SegmentsCount() const noexcept { return segments_.size(); }
+
+ auto SegmentBegin() noexcept { return segments_.begin(); }
+
+ auto SegmentEnd() noexcept { return segments_.end(); }
+
+ private:
+ std::vector<SegmentCache *> segments_;
+};
+} // namespace
+
+template <bool ReadOnly>
+rocksdb::Status Bitmap::bitfield(const Slice &user_key, const std::vector<BitfieldOperation> &ops,
+ std::vector<std::string> *rets) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
+
+ LockGuard guard;
+ if constexpr (!ReadOnly) {
+ guard = LockGuard(storage_->GetLockManager(), ns_key);
+ }
+
+ BitmapMetadata metadata;
+ std::string raw_value;
+ auto s = GetMetadata(ns_key, &metadata, &raw_value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+
+ if (metadata.Type() == RedisType::kRedisString) {
+ if constexpr (ReadOnly) {
+ s = BitmapString::BitfieldReadOnly(ns_key, raw_value, ops, rets);
+ } else {
+ s = BitmapString(storage_, namespace_).Bitfield(ns_key, &raw_value, ops, rets);
+ }
+ return s;
+ }
+
+ if (metadata.Type() != RedisType::kRedisBitmap) {
+ return rocksdb::Status::InvalidArgument("The value is not a bitmap or string.");
+ }
+
+ std::unordered_map<uint32_t, SegmentCache> seg_cache;
+ auto get_segment = [&](uint32_t index, SegmentCache **res) {
+ auto [seg_itor, no_cache] = seg_cache.try_emplace(index);
+ auto &[sub_key, value] = seg_itor->second;
+ if (no_cache) {
+ auto sub_sub_key = std::to_string(index * kBitmapSegmentBytes);
+ sub_key = InternalKey(ns_key, sub_sub_key, metadata.version, storage_->IsSlotIdEncoded()).Encode();
+ rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(), sub_key, &value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+ }
+ *res = &seg_itor->second;
+ return rocksdb::Status::OK();
+ };
+
+ auto init_view = [&get_segment](uint32_t offset, uint32_t bits, BitmapSegmentsView *view) {
+ uint32_t first_segment = offset / kBitmapSegmentBits;
+ uint32_t last_segment = (offset + bits - 1) / kBitmapSegmentBits;
+
+ std::vector<SegmentCache *> segments(last_segment - first_segment + 1);
+ for (auto &seg : segments) {
+ auto seg_status = get_segment(first_segment++, &seg);
+ if (!seg_status.ok()) {
+ return seg_status;
+ }
+ }
+ *view = BitmapSegmentsView(std::move(segments));
+ return rocksdb::Status::OK();
+ };
+
+ // get bitfield value from entire bitmap
+ auto get = [&init_view](uint32_t offset, BitfieldEncoding enc, uint64_t *res) {
+ BitmapSegmentsView view;
+ auto view_status = init_view(offset, enc.Bits(), &view);
+ if (!view_status.ok()) {
+ return view_status;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ if (enc.IsSigned()) {
+ *res = GetSignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ } else {
+ *res = GetUnsignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ if constexpr (ReadOnly) {
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ return rocksdb::Status::InvalidArgument("Write bitfield in read-only mode.");
+ }
+
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_old_value));
+ } else {
+ ret = redis::Integer(unsigned_old_value);
+ }
+
+ rets->push_back(std::move(ret));
+ }
+ return rocksdb::Status::OK();
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ bitfieldWriteAheadLog(batch, ops);
+
+ // set bitfield value among segments
+ auto set = [&](uint32_t offset, BitfieldEncoding enc, BitfieldOverflowBehavior overflow, uint64_t value) {
+ BitmapSegmentsView view;
+ s = init_view(offset, enc.Bits(), &view);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ SetBitfield(view, first_bit_segment_offset, enc.Bits(), value);
+
+ for (auto segment = view.SegmentBegin(); segment != view.SegmentEnd(); ++segment) {
+ batch->Put((*segment)->first, (*segment)->second);
+ }
+
+ uint64_t segments_cnt = view.SegmentsCount();
+ uint64_t used_size = (offset / 8) + (segments_cnt - 1) * kBitmapSegmentBytes + view.LastSegmentSize();
+ if (metadata.size < used_size) {
+ metadata.size = used_size;
+ std::string bytes;
+ metadata.Encode(&bytes);
+ batch->Put(metadata_cf_handle_, ns_key, bytes);
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint64_t unsigned_new_value = 0;
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value)) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ s = set(op.offset, op.encoding, op.overflow, unsigned_new_value);
+ if (!s.ok()) {
+ return s;
+ }
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_new_value));
+ } else {
+ ret = redis::Integer(unsigned_new_value);
+ }
+ } else {
+ ret = redis::NilString();
+ }
Review Comment:
Maybe it can be `std::optional<struct X>`, where `struct X { uint64_t v; bool signed; std::string to_string(){ ... } }`, if you are willing to change it.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1820827772
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1826767347
I am trying to use rocksdb::PinnableSlice to improve Bitmap::SegmentCacheStore. But I failed because of some strange error. When I use `std::unordered_map<uint32_t, rocksdb::PinnableSlice> cache_` and get its iterator, the iterator compare function is failed to instantiate (such as: `it != cache_.end()`), which makes me can not travel `cache_` in `BatchForFlush()`. `rocksdb::Cleanable` has a bug in current rocksdb version(It's copy constructor deleted, but the param of it is not marked const).
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1820395538
> > @julic20s Can use `./x.py format` to format your code before pushing.
>
> Sorry, I didn't see it at first, I just used shortcut formatting in the ide
ooh, that's fine. You can also configure clang-format in IDE if supported.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1413066822
##########
src/types/redis_bitmap_string.cc:
##########
@@ -204,4 +205,88 @@ int64_t BitmapString::RawBitpos(const uint8_t *c, int64_t count, bool bit) {
return res;
}
+rocksdb::Status BitmapString::Bitfield(const Slice &ns_key, std::string *raw_value,
+ const std::vector<BitfieldOperation> &ops,
+ std::vector<std::optional<BitfieldValue>> *rets) {
+ auto header_offset = Metadata::GetOffsetAfterExpire((*raw_value)[0]);
+ std::string string_value = raw_value->substr(header_offset);
+ for (BitfieldOperation op : ops) {
+ // [first_byte, last_byte)
+ uint32_t first_byte = op.offset / 8;
+ uint32_t last_byte = (op.offset + op.encoding.Bits() - 1) / 8 + 1;
+
+ // expand string if need.
+ if (string_value.size() < last_byte) {
+ string_value.resize(last_byte);
+ }
+
+ ArrayBitfieldBitmap bitfield(first_byte);
+ auto str = reinterpret_cast<const uint8_t *>(string_value.data() + first_byte);
+ auto s = bitfield.Set(first_byte, last_byte - first_byte, str);
+
+ uint64_t unsigned_old_value = 0;
+ if (op.encoding.IsSigned()) {
+ unsigned_old_value = bitfield.GetSignedBitfield(op.offset, op.encoding.Bits()).GetValue();
+ } else {
+ unsigned_old_value = bitfield.GetUnsignedBitfield(op.offset, op.encoding.Bits()).GetValue();
+ }
+
+ uint64_t unsigned_new_value = 0;
+ auto &ret = rets->emplace_back();
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value).GetValue()) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ // never failed.
+ s = bitfield.SetBitfield(op.offset, op.encoding.Bits(), unsigned_new_value);
+ auto dst = reinterpret_cast<uint8_t *>(string_value.data()) + first_byte;
+ s = bitfield.Get(first_byte, last_byte - first_byte, dst);
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ ret = {op.encoding, unsigned_new_value};
+ }
+ }
+
+ raw_value->resize(header_offset);
+ raw_value->append(string_value);
+ auto batch = storage_->GetWriteBatchBase();
+ WriteBatchLogData log_data(kRedisString);
+ batch->PutLogData(log_data.Encode());
+ batch->Put(metadata_cf_handle_, ns_key, *raw_value);
+
+ return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
+}
+
+rocksdb::Status BitmapString::BitfieldReadOnly(const Slice &ns_key, const std::string &raw_value,
+ const std::vector<BitfieldOperation> &ops,
+ std::vector<std::optional<BitfieldValue>> *rets) {
+ std::string_view string_value = raw_value;
+ string_value = string_value.substr(Metadata::GetOffsetAfterExpire(string_value[0]));
+
+ for (BitfieldOperation op : ops) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ return rocksdb::Status::InvalidArgument("Write bitfield in read-only mode.");
+ }
+
+ uint32_t first_byte = op.offset / 8;
+ uint32_t last_byte = (op.offset + op.encoding.Bits() - 1) / 8 + 1;
+
+ ArrayBitfieldBitmap bitfield(first_byte);
+ auto s = bitfield.Set(first_byte, last_byte - first_byte,
+ reinterpret_cast<const uint8_t *>(string_value.data() + first_byte));
+
+ if (op.encoding.IsSigned()) {
+ int64_t value = bitfield.GetSignedBitfield(op.offset, op.encoding.Bits()).GetValue();
+ rets->emplace_back() = {op.encoding, static_cast<uint64_t>(value)};
Review Comment:
```suggestion
rets->emplace_back(std::in_place, op.encoding, static_cast<uint64_t>(value));
```
##########
src/types/redis_bitmap_string.cc:
##########
@@ -204,4 +205,88 @@ int64_t BitmapString::RawBitpos(const uint8_t *c, int64_t count, bool bit) {
return res;
}
+rocksdb::Status BitmapString::Bitfield(const Slice &ns_key, std::string *raw_value,
+ const std::vector<BitfieldOperation> &ops,
+ std::vector<std::optional<BitfieldValue>> *rets) {
+ auto header_offset = Metadata::GetOffsetAfterExpire((*raw_value)[0]);
+ std::string string_value = raw_value->substr(header_offset);
+ for (BitfieldOperation op : ops) {
+ // [first_byte, last_byte)
+ uint32_t first_byte = op.offset / 8;
+ uint32_t last_byte = (op.offset + op.encoding.Bits() - 1) / 8 + 1;
+
+ // expand string if need.
+ if (string_value.size() < last_byte) {
+ string_value.resize(last_byte);
+ }
+
+ ArrayBitfieldBitmap bitfield(first_byte);
+ auto str = reinterpret_cast<const uint8_t *>(string_value.data() + first_byte);
+ auto s = bitfield.Set(first_byte, last_byte - first_byte, str);
+
+ uint64_t unsigned_old_value = 0;
+ if (op.encoding.IsSigned()) {
+ unsigned_old_value = bitfield.GetSignedBitfield(op.offset, op.encoding.Bits()).GetValue();
+ } else {
+ unsigned_old_value = bitfield.GetUnsignedBitfield(op.offset, op.encoding.Bits()).GetValue();
+ }
+
+ uint64_t unsigned_new_value = 0;
+ auto &ret = rets->emplace_back();
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value).GetValue()) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ // never failed.
+ s = bitfield.SetBitfield(op.offset, op.encoding.Bits(), unsigned_new_value);
+ auto dst = reinterpret_cast<uint8_t *>(string_value.data()) + first_byte;
+ s = bitfield.Get(first_byte, last_byte - first_byte, dst);
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ ret = {op.encoding, unsigned_new_value};
+ }
+ }
+
+ raw_value->resize(header_offset);
+ raw_value->append(string_value);
+ auto batch = storage_->GetWriteBatchBase();
+ WriteBatchLogData log_data(kRedisString);
+ batch->PutLogData(log_data.Encode());
+ batch->Put(metadata_cf_handle_, ns_key, *raw_value);
+
+ return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
+}
+
+rocksdb::Status BitmapString::BitfieldReadOnly(const Slice &ns_key, const std::string &raw_value,
+ const std::vector<BitfieldOperation> &ops,
+ std::vector<std::optional<BitfieldValue>> *rets) {
+ std::string_view string_value = raw_value;
+ string_value = string_value.substr(Metadata::GetOffsetAfterExpire(string_value[0]));
+
+ for (BitfieldOperation op : ops) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ return rocksdb::Status::InvalidArgument("Write bitfield in read-only mode.");
+ }
+
+ uint32_t first_byte = op.offset / 8;
+ uint32_t last_byte = (op.offset + op.encoding.Bits() - 1) / 8 + 1;
+
+ ArrayBitfieldBitmap bitfield(first_byte);
+ auto s = bitfield.Set(first_byte, last_byte - first_byte,
+ reinterpret_cast<const uint8_t *>(string_value.data() + first_byte));
+
+ if (op.encoding.IsSigned()) {
+ int64_t value = bitfield.GetSignedBitfield(op.offset, op.encoding.Bits()).GetValue();
+ rets->emplace_back() = {op.encoding, static_cast<uint64_t>(value)};
+ } else {
+ uint64_t value = bitfield.GetUnsignedBitfield(op.offset, op.encoding.Bits()).GetValue();
+ rets->emplace_back() = {op.encoding, value};
Review Comment:
ditto
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1820864940
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1820390644
> @julic20s Can use `./x.py format` to format your code before pushing.
Sorry, I didn't see it at first, I just used shortcut formatting in the ide
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1403363941
##########
src/types/redis_bitmap.cc:
##########
@@ -538,6 +546,320 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+// SegmentCacheStore is used to read segments from storage.
+class Bitmap::SegmentCacheStore {
+ public:
+ SegmentCacheStore(engine::Storage *storage, rocksdb::ColumnFamilyHandle *metadata_cf_handle,
+ std::string namespace_key, const Metadata &bitmap_metadata)
+ : storage_(storage),
+ metadata_cf_handle_(metadata_cf_handle),
+ ns_key_(std::move(namespace_key)),
+ metadata_(bitmap_metadata) {}
+
+ // Get a read-only segment by given index
+ rocksdb::Status Get(uint32_t index, const std::string **cache) {
+ std::string *res = nullptr;
+ auto s = get(index, false, &res);
+ if (s.ok()) {
+ *cache = res;
+ }
+ return s;
+ }
+
+ // Get a segment by given index, and mark it dirty.
+ rocksdb::Status GetMut(uint32_t index, std::string **cache) { return get(index, true, cache); }
+
+ // Add all dirty segments into write batch.
+ void BatchForFlush(ObserverOrUniquePtr<rocksdb::WriteBatchBase> &batch) {
+ uint64_t used_size = 0;
+ for (auto &[index, content] : cache_) {
+ if (content.first) {
+ InternalKey sub_key(ns_key_, getSegmentSubKey(index), metadata_.version, storage_->IsSlotIdEncoded());
+ batch->Put(sub_key.Encode(), content.second);
+ used_size = std::max(used_size, index * kBitmapSegmentBytes + content.second.size());
+ }
+ }
+ if (used_size > metadata_.size) {
+ metadata_.size = used_size;
+ std::string bytes;
+ metadata_.Encode(&bytes);
+ batch->Put(metadata_cf_handle_, ns_key_, bytes);
+ }
+ }
+
+ private:
+ rocksdb::Status get(uint32_t index, bool set_dirty, std::string **cache) {
+ auto [seg_itor, no_cache] = cache_.try_emplace(index);
+ auto &[is_dirty, str] = seg_itor->second;
+
+ if (no_cache) {
+ is_dirty = false;
+ InternalKey sub_key(ns_key_, getSegmentSubKey(index), metadata_.version, storage_->IsSlotIdEncoded());
+ rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(), sub_key.Encode(), &str);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+ }
+
+ is_dirty |= set_dirty;
+ *cache = &str;
+ return rocksdb::Status::OK();
+ }
+
+ static std::string getSegmentSubKey(uint32_t index) { return std::to_string(index * kBitmapSegmentBytes); }
+
+ engine::Storage *storage_;
+ rocksdb::ColumnFamilyHandle *metadata_cf_handle_;
+ std::string ns_key_;
+ Metadata metadata_;
+ // Segment index -> [is_dirty, segment_cache_string]
+ std::unordered_map<uint32_t, std::pair<bool, std::string>> cache_;
+};
+
+namespace {
+
+// Copy a range of bytes from entire bitmap and store them into ArrayBitfieldBitmap.
+static rocksdb::Status CopySegmentsBytesToBitfield(Bitmap::SegmentCacheStore &store, uint32_t byte_offset,
+ uint32_t bytes, ArrayBitfieldBitmap *bitfield) {
+ bitfield->SetOffset(byte_offset);
+ bitfield->Reset();
+
+ uint32_t segment_index = byte_offset / kBitmapSegmentBytes;
+ int64_t remain_bytes = bytes;
+ // the byte_offset in current segment.
+ int segment_byte_offset = static_cast<int>(byte_offset % kBitmapSegmentBytes);
+ for (; remain_bytes > 0; ++segment_index) {
+ const std::string *cache = nullptr;
+ auto cache_status = store.Get(segment_index, &cache);
+ if (!cache_status.ok()) {
+ return cache_status;
+ }
+
+ int cache_size = static_cast<int>(cache->size());
+ auto copyable = std::max(0, cache_size - segment_byte_offset);
+ auto copy_count = std::min(static_cast<int>(remain_bytes), copyable);
+ auto src = reinterpret_cast<const uint8_t *>(cache->data() + segment_byte_offset);
+ auto status = bitfield->Set(byte_offset, copy_count, src);
+ if (!status) {
+ return rocksdb::Status::InvalidArgument();
+ }
+
+ // next segment will copy from its front.
+ byte_offset = (segment_index + 1) * kBitmapSegmentBytes;
+ // maybe negative, but still correct.
+ remain_bytes -= kBitmapSegmentBytes - segment_byte_offset;
+ segment_byte_offset = 0;
+ }
+
+ return rocksdb::Status::OK();
+}
+
+static rocksdb::Status GetBitfieldInteger(const ArrayBitfieldBitmap &bitfield, uint32_t offset, BitfieldEncoding enc,
+ uint64_t *res) {
+ if (enc.IsSigned()) {
+ auto status = bitfield.GetSignedBitfield(offset, enc.Bits());
+ if (!status) {
+ return rocksdb::Status::InvalidArgument();
+ }
+ *res = status.GetValue();
+ } else {
+ auto status = bitfield.GetUnsignedBitfield(offset, enc.Bits());
+ if (!status) {
+ return rocksdb::Status::InvalidArgument();
+ }
+ *res = status.GetValue();
+ }
+ return rocksdb::Status::OK();
+}
+
+static rocksdb::Status CopyBitfieldBytesToSegments(Bitmap::SegmentCacheStore &store,
+ const ArrayBitfieldBitmap &bitfield, uint32_t byte_offset,
+ uint32_t bytes) {
+ uint32_t segment_index = byte_offset / kBitmapSegmentBytes;
+ int segment_byte_offset = static_cast<int>(byte_offset % kBitmapSegmentBytes);
+ auto remain_bytes = static_cast<int32_t>(bytes);
+ for (; remain_bytes > 0; ++segment_index) {
+ std::string *cache = nullptr;
+ auto cache_status = store.GetMut(segment_index, &cache);
+ if (!cache_status.ok()) {
+ return cache_status;
+ }
+
+ auto copy_count = std::min(remain_bytes, static_cast<int32_t>(kBitmapSegmentBytes - segment_byte_offset));
+ if (static_cast<int>(cache->size()) < segment_byte_offset + copy_count) {
+ cache->resize(segment_byte_offset + copy_count);
+ }
+
+ auto status = bitfield.Get(byte_offset, copy_count, cache->data() + segment_byte_offset);
+ if (!status) {
+ return rocksdb::Status::InvalidArgument();
+ }
+
+ // next segment will copy from its front.
+ byte_offset = (segment_index + 1) * kBitmapSegmentBytes;
+ // maybe negative, but still correct.
+ remain_bytes -= static_cast<int32_t>(kBitmapSegmentBytes - segment_byte_offset);
+ segment_byte_offset = 0;
+ }
+ return rocksdb::Status::OK();
+}
+
+} // namespace
+
+template <bool ReadOnly>
+rocksdb::Status Bitmap::bitfield(const Slice &user_key, const std::vector<BitfieldOperation> &ops,
+ std::vector<std::optional<BitfieldValue>> *rets) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
+
+ std::optional<LockGuard> guard;
+ if constexpr (!ReadOnly) {
+ guard = LockGuard(storage_->GetLockManager(), ns_key);
+ }
+
+ BitmapMetadata metadata;
+ std::string raw_value;
+ auto s = GetMetadata(ns_key, &metadata, &raw_value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+
+ if (metadata.Type() == RedisType::kRedisString) {
+ if constexpr (ReadOnly) {
+ s = BitmapString::BitfieldReadOnly(ns_key, raw_value, ops, rets);
+ } else {
+ s = BitmapString(storage_, namespace_).Bitfield(ns_key, &raw_value, ops, rets);
+ }
+ return s;
+ }
+
+ if (metadata.Type() != RedisType::kRedisBitmap) {
+ return rocksdb::Status::InvalidArgument("The value is not a bitmap or string.");
+ }
+
+ // We firstly do the bitfield operation by fetching segments into memory.
+ // Use SegmentCacheStore to record dirty segments. (if not read-only mode)
+ SegmentCacheStore cache(storage_, metadata_cf_handle_, ns_key, metadata);
+ runBitfieldOperationsWithCache<ReadOnly>(cache, ops, rets);
+
+ if constexpr (ReadOnly) {
+ return rocksdb::Status::OK();
+ }
+
+ // Write changes into storage.
+ auto batch = storage_->GetWriteBatchBase();
+ bitfieldWriteAheadLog(batch, ops);
+ cache.BatchForFlush(batch);
+ return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
+}
+
+template <bool ReadOnly>
+rocksdb::Status Bitmap::runBitfieldOperationsWithCache(SegmentCacheStore &cache,
+ const std::vector<BitfieldOperation> &ops,
+ std::vector<std::optional<BitfieldValue>> *rets) {
+ ArrayBitfieldBitmap tmp_bitfield;
+ for (BitfieldOperation op : ops) {
+ // found all bytes that contents the bitfield.
+ uint32_t first_byte = op.offset / 8;
+ uint32_t last_bytes = (op.offset + op.encoding.Bits() - 1) / 8 + 1;
+ uint32_t bytes = last_bytes - first_byte;
+
+ ArrayBitfieldBitmap bitfield;
+ auto segment_status = CopySegmentsBytesToBitfield(cache, first_byte, bytes, &bitfield);
+ if (!segment_status.ok()) {
+ return segment_status;
+ }
+
+ // Covert the bitfield from a buffer to an integer.
+ uint64_t unsigned_old_value = 0;
+ auto s = GetBitfieldInteger(bitfield, op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ if constexpr (ReadOnly) {
+ rets->emplace_back() = {op.encoding, unsigned_old_value};
+ continue;
+ }
+
+ auto &ret = rets->emplace_back();
+ uint64_t unsigned_new_value = 0;
+ // BitfieldOp failed only when the length or bits not illegal.
Review Comment:
There is a typo. Should be `BitfieldOp failed only when the length or bits illegal.`
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1822274781
The mutex pool in lock manager may be reallocated by MultiGet(). Then the old mutex pointers in LockGuard all be invalid...
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1405347883
##########
src/types/redis_bitmap.cc:
##########
@@ -187,17 +205,7 @@ rocksdb::Status Bitmap::SetBit(const Slice &user_key, uint32_t offset, bool new_
uint32_t byte_index = (offset / 8) % kBitmapSegmentBytes;
uint64_t used_size = index + byte_index + 1;
uint64_t bitmap_size = std::max(used_size, metadata.size);
- if (byte_index >= value.size()) { // expand the bitmap
- size_t expand_size = 0;
- if (byte_index >= value.size() * 2) {
- expand_size = byte_index - value.size() + 1;
- } else if (value.size() * 2 > kBitmapSegmentBytes) {
- expand_size = kBitmapSegmentBytes - value.size();
- } else {
- expand_size = value.size();
- }
- value.append(expand_size, 0);
- }
+ ExpandBitmapSegment(&value, byte_index + 1);
Review Comment:
@mapleFU byte_index is the last index will be used. It can be explained as `expand_size = (byte_index + 1) /* new size */ - value.size() /* old size */`;
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1835275097
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1826225528
done
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1405345188
##########
src/types/redis_bitmap.cc:
##########
@@ -539,6 +547,321 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+// SegmentCacheStore is used to read segments from storage.
+class Bitmap::SegmentCacheStore {
+ public:
+ SegmentCacheStore(engine::Storage *storage, rocksdb::ColumnFamilyHandle *metadata_cf_handle,
+ std::string namespace_key, const Metadata &bitmap_metadata)
+ : storage_(storage),
+ metadata_cf_handle_(metadata_cf_handle),
+ ns_key_(std::move(namespace_key)),
+ metadata_(bitmap_metadata) {}
+
+ // Get a read-only segment by given index
+ rocksdb::Status Get(uint32_t index, const std::string **cache) {
+ std::string *res = nullptr;
+ auto s = get(index, false, &res);
Review Comment:
```suggestion
auto s = get(index, /*set_dirty=*/false, &res);
```
##########
src/types/redis_bitmap.cc:
##########
@@ -539,6 +547,321 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+// SegmentCacheStore is used to read segments from storage.
+class Bitmap::SegmentCacheStore {
+ public:
+ SegmentCacheStore(engine::Storage *storage, rocksdb::ColumnFamilyHandle *metadata_cf_handle,
+ std::string namespace_key, const Metadata &bitmap_metadata)
+ : storage_(storage),
+ metadata_cf_handle_(metadata_cf_handle),
+ ns_key_(std::move(namespace_key)),
+ metadata_(bitmap_metadata) {}
+
+ // Get a read-only segment by given index
+ rocksdb::Status Get(uint32_t index, const std::string **cache) {
+ std::string *res = nullptr;
+ auto s = get(index, false, &res);
+ if (s.ok()) {
+ *cache = res;
+ }
+ return s;
+ }
+
+ // Get a segment by given index, and mark it dirty.
+ rocksdb::Status GetMut(uint32_t index, std::string **cache) { return get(index, true, cache); }
Review Comment:
```suggestion
rocksdb::Status GetMut(uint32_t index, std::string **cache) { return get(index, /*set_dirty=*/true, cache); }
```
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1822322909
```cpp
LockGuard(LockGuard &&guard) : lock_(guard.lock_) { guard.lock_ = nullptr; } // correct constructor
LockGuard &operator=(LockGuard &&other) = default; // uncorrect assignment.
```
😭
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1837503429
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.5% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1400177749
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <stdexcept>
+#include <cstdint>
+#include <limits>
+#include <type_traits>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+struct BitfieldEncoding {
+ bool is_signed;
+ // 1 ~ 63 if unsigned, 1 ~ 64 if signed.
Review Comment:
The code comment incomprehensible. I will update it after.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1400162162
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <stdexcept>
+#include <cstdint>
+#include <limits>
+#include <type_traits>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+struct BitfieldEncoding {
+ bool is_signed;
+ // 1 ~ 63 if unsigned, 1 ~ 64 if signed.
Review Comment:
> Should we `static_assert(sizeof(...))` for this?
I don't get it. Should I add likes static_assert(sizeof(BitfieldEncoding) == 2)?
The comment in code means redis only support i1~i64 and u1~u63 in bitfield.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1401930809
##########
src/types/redis_bitmap.cc:
##########
@@ -538,6 +545,281 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+namespace {
+using SegmentCache = std::pair<std::string, std::string>;
+
+// used to simplify accessing bitfield that occupies multiple continuous segments.
+// you can access multiple segments like accessing in one big segments.
+class BitmapSegmentsView {
+ public:
+ BitmapSegmentsView() = default;
+ explicit BitmapSegmentsView(std::vector<SegmentCache *> segments, bool expand_in_advance = true)
+ : segments_(std::move(segments)) {
+ if (expand_in_advance && segments_.size() > 1) {
+ // expand segments during iteration (not in advance) maybe increase allocation times.
+ // we assume that all end of each segment except last one will be accessed.
+ // note: segments_.size() is unsigned.
+ for (size_t i = 0; i < segments_.size() - 1; ++i) {
+ ExpandBitmapSegment(&segments_[i]->second, kBitmapSegmentBytes);
+ }
+ }
+ }
+
+ char &operator[](uint32_t index) noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ const char &operator[](uint32_t index) const noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ SegmentCache &GetSegment(uint32_t index) noexcept { return *segments_[index]; }
+
+ const SegmentCache &GetSegment(uint32_t index) const noexcept { return *segments_[index]; }
+
+ size_t LastSegmentSize() const noexcept { return segments_.back()->second.size(); }
+
+ size_t SegmentsCount() const noexcept { return segments_.size(); }
+
+ auto SegmentBegin() noexcept { return segments_.begin(); }
+
+ auto SegmentEnd() noexcept { return segments_.end(); }
+
+ private:
+ std::vector<SegmentCache *> segments_;
+};
+} // namespace
+
+template <bool ReadOnly>
+rocksdb::Status Bitmap::bitfield(const Slice &user_key, const std::vector<BitfieldOperation> &ops,
+ std::vector<std::string> *rets) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
+
+ LockGuard guard;
+ if constexpr (!ReadOnly) {
+ guard = LockGuard(storage_->GetLockManager(), ns_key);
+ }
+
+ BitmapMetadata metadata;
+ std::string raw_value;
+ auto s = GetMetadata(ns_key, &metadata, &raw_value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+
+ if (metadata.Type() == RedisType::kRedisString) {
+ if constexpr (ReadOnly) {
+ s = BitmapString::BitfieldReadOnly(ns_key, raw_value, ops, rets);
+ } else {
+ s = BitmapString(storage_, namespace_).Bitfield(ns_key, &raw_value, ops, rets);
+ }
+ return s;
+ }
+
+ if (metadata.Type() != RedisType::kRedisBitmap) {
+ return rocksdb::Status::InvalidArgument("The value is not a bitmap or string.");
+ }
+
+ std::unordered_map<uint32_t, SegmentCache> seg_cache;
+ auto get_segment = [&](uint32_t index, SegmentCache **res) {
+ auto [seg_itor, no_cache] = seg_cache.try_emplace(index);
+ auto &[sub_key, value] = seg_itor->second;
+ if (no_cache) {
+ auto sub_sub_key = std::to_string(index * kBitmapSegmentBytes);
+ sub_key = InternalKey(ns_key, sub_sub_key, metadata.version, storage_->IsSlotIdEncoded()).Encode();
+ rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(), sub_key, &value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+ }
+ *res = &seg_itor->second;
+ return rocksdb::Status::OK();
+ };
+
+ auto init_view = [&get_segment](uint32_t offset, uint32_t bits, BitmapSegmentsView *view) {
+ uint32_t first_segment = offset / kBitmapSegmentBits;
+ uint32_t last_segment = (offset + bits - 1) / kBitmapSegmentBits;
+
+ std::vector<SegmentCache *> segments(last_segment - first_segment + 1);
+ for (auto &seg : segments) {
+ auto seg_status = get_segment(first_segment++, &seg);
+ if (!seg_status.ok()) {
+ return seg_status;
+ }
+ }
+ *view = BitmapSegmentsView(std::move(segments));
+ return rocksdb::Status::OK();
+ };
+
+ // get bitfield value from entire bitmap
+ auto get = [&init_view](uint32_t offset, BitfieldEncoding enc, uint64_t *res) {
+ BitmapSegmentsView view;
+ auto view_status = init_view(offset, enc.Bits(), &view);
+ if (!view_status.ok()) {
+ return view_status;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ if (enc.IsSigned()) {
+ *res = GetSignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ } else {
+ *res = GetUnsignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ if constexpr (ReadOnly) {
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ return rocksdb::Status::InvalidArgument("Write bitfield in read-only mode.");
+ }
+
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_old_value));
+ } else {
+ ret = redis::Integer(unsigned_old_value);
+ }
+
+ rets->push_back(std::move(ret));
+ }
+ return rocksdb::Status::OK();
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ bitfieldWriteAheadLog(batch, ops);
+
+ // set bitfield value among segments
+ auto set = [&](uint32_t offset, BitfieldEncoding enc, BitfieldOverflowBehavior overflow, uint64_t value) {
+ BitmapSegmentsView view;
+ s = init_view(offset, enc.Bits(), &view);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ SetBitfield(view, first_bit_segment_offset, enc.Bits(), value);
+
+ for (auto segment = view.SegmentBegin(); segment != view.SegmentEnd(); ++segment) {
+ batch->Put((*segment)->first, (*segment)->second);
+ }
+
+ uint64_t segments_cnt = view.SegmentsCount();
+ uint64_t used_size = (offset / 8) + (segments_cnt - 1) * kBitmapSegmentBytes + view.LastSegmentSize();
+ if (metadata.size < used_size) {
+ metadata.size = used_size;
+ std::string bytes;
+ metadata.Encode(&bytes);
+ batch->Put(metadata_cf_handle_, ns_key, bytes);
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint64_t unsigned_new_value = 0;
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value)) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ s = set(op.offset, op.encoding, op.overflow, unsigned_new_value);
+ if (!s.ok()) {
+ return s;
+ }
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_new_value));
+ } else {
+ ret = redis::Integer(unsigned_new_value);
+ }
+ } else {
+ ret = redis::NilString();
+ }
Review Comment:
Maybe it can be `std::optional<struct { uint64_t v; bool signed; std::string to_string(){ ... } }>`, if you are willing to change it.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1405380898
##########
src/types/redis_bitmap.cc:
##########
@@ -187,17 +205,7 @@ rocksdb::Status Bitmap::SetBit(const Slice &user_key, uint32_t offset, bool new_
uint32_t byte_index = (offset / 8) % kBitmapSegmentBytes;
uint64_t used_size = index + byte_index + 1;
uint64_t bitmap_size = std::max(used_size, metadata.size);
- if (byte_index >= value.size()) { // expand the bitmap
- size_t expand_size = 0;
- if (byte_index >= value.size() * 2) {
- expand_size = byte_index - value.size() + 1;
- } else if (value.size() * 2 > kBitmapSegmentBytes) {
- expand_size = kBitmapSegmentBytes - value.size();
- } else {
- expand_size = value.size();
- }
- value.append(expand_size, 0);
- }
+ ExpandBitmapSegment(&value, byte_index + 1);
Review Comment:
Got it, my bad
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1826767652
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1822352779
@julic20s BTW, would you like to take anonymous functions(get/set/get_segment) from Bitmap::bitfield into named functions. This function is a bit hard to read for now.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1405380991
##########
src/common/bitfield_util.cc:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include "bitfield_util.h"
+
+namespace detail {
+
+StatusOr<bool> SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst) {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kSigned, bits)) {
+ return Status::NotOK;
+ }
+
+ auto max = std::numeric_limits<int64_t>::max();
+ if (bits != 64) {
+ max = (static_cast<int64_t>(1) << (bits - 1)) - 1;
+ }
+ int64_t min = -max - 1;
+
+ int64_t signed_value = CastToSignedWithoutBitChanges(value);
+ int64_t max_incr = CastToSignedWithoutBitChanges(static_cast<uint64_t>(max) - value);
+ int64_t min_incr = min - signed_value;
+
+ auto wrap = [=]() {
+ uint64_t res = value + static_cast<uint64_t>(incr);
+ if (bits < 64) {
+ auto mask = std::numeric_limits<uint64_t>::max() << bits;
+ if ((res & (1 << (bits - 1))) != 0) {
+ res |= mask;
+ } else {
+ res &= ~mask;
+ }
+ }
+ return res;
+ };
+
Review Comment:
We can keep the logic here, and we may need to paste the code link to redis
I think later we may use `checked_add` to simplify this logic
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1837502019
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.5% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1837498179
@PragmaTwice @mapleFU I think we can go ahead to merge this PR if overall looks good.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1401423417
##########
src/types/redis_bitmap.cc:
##########
@@ -538,6 +545,281 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+namespace {
+using SegmentCache = std::pair<std::string, std::string>;
+
+// used to simplify accessing bitfield that occupies multiple continuous segments.
+// you can access multiple segments like accessing in one big segments.
+class BitmapSegmentsView {
+ public:
+ BitmapSegmentsView() = default;
+ explicit BitmapSegmentsView(std::vector<SegmentCache *> segments, bool expand_in_advance = true)
+ : segments_(std::move(segments)) {
+ if (expand_in_advance && segments_.size() > 1) {
+ // expand segments during iteration (not in advance) maybe increase allocation times.
+ // we assume that all end of each segment except last one will be accessed.
+ // note: segments_.size() is unsigned.
+ for (size_t i = 0; i < segments_.size() - 1; ++i) {
+ ExpandBitmapSegment(&segments_[i]->second, kBitmapSegmentBytes);
+ }
+ }
+ }
+
+ char &operator[](uint32_t index) noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ const char &operator[](uint32_t index) const noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ SegmentCache &GetSegment(uint32_t index) noexcept { return *segments_[index]; }
+
+ const SegmentCache &GetSegment(uint32_t index) const noexcept { return *segments_[index]; }
+
+ size_t LastSegmentSize() const noexcept { return segments_.back()->second.size(); }
+
+ size_t SegmentsCount() const noexcept { return segments_.size(); }
+
+ auto SegmentBegin() noexcept { return segments_.begin(); }
+
+ auto SegmentEnd() noexcept { return segments_.end(); }
+
+ private:
+ std::vector<SegmentCache *> segments_;
+};
+} // namespace
+
+template <bool ReadOnly>
+rocksdb::Status Bitmap::bitfield(const Slice &user_key, const std::vector<BitfieldOperation> &ops,
+ std::vector<std::string> *rets) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
+
+ LockGuard guard;
+ if constexpr (!ReadOnly) {
+ guard = LockGuard(storage_->GetLockManager(), ns_key);
+ }
+
+ BitmapMetadata metadata;
+ std::string raw_value;
+ auto s = GetMetadata(ns_key, &metadata, &raw_value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+
+ if (metadata.Type() == RedisType::kRedisString) {
+ if constexpr (ReadOnly) {
+ s = BitmapString::BitfieldReadOnly(ns_key, raw_value, ops, rets);
+ } else {
+ s = BitmapString(storage_, namespace_).Bitfield(ns_key, &raw_value, ops, rets);
+ }
+ return s;
+ }
+
+ if (metadata.Type() != RedisType::kRedisBitmap) {
+ return rocksdb::Status::InvalidArgument("The value is not a bitmap or string.");
+ }
+
+ std::unordered_map<uint32_t, SegmentCache> seg_cache;
+ auto get_segment = [&](uint32_t index, SegmentCache **res) {
+ auto [seg_itor, no_cache] = seg_cache.try_emplace(index);
+ auto &[sub_key, value] = seg_itor->second;
+ if (no_cache) {
+ auto sub_sub_key = std::to_string(index * kBitmapSegmentBytes);
+ sub_key = InternalKey(ns_key, sub_sub_key, metadata.version, storage_->IsSlotIdEncoded()).Encode();
+ rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(), sub_key, &value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+ }
+ *res = &seg_itor->second;
+ return rocksdb::Status::OK();
+ };
+
+ auto init_view = [&get_segment](uint32_t offset, uint32_t bits, BitmapSegmentsView *view) {
+ uint32_t first_segment = offset / kBitmapSegmentBits;
+ uint32_t last_segment = (offset + bits - 1) / kBitmapSegmentBits;
+
+ std::vector<SegmentCache *> segments(last_segment - first_segment + 1);
+ for (auto &seg : segments) {
+ auto seg_status = get_segment(first_segment++, &seg);
+ if (!seg_status.ok()) {
+ return seg_status;
+ }
+ }
+ *view = BitmapSegmentsView(std::move(segments));
+ return rocksdb::Status::OK();
+ };
+
+ // get bitfield value from entire bitmap
+ auto get = [&init_view](uint32_t offset, BitfieldEncoding enc, uint64_t *res) {
+ BitmapSegmentsView view;
+ auto view_status = init_view(offset, enc.Bits(), &view);
+ if (!view_status.ok()) {
+ return view_status;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ if (enc.IsSigned()) {
+ *res = GetSignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ } else {
+ *res = GetUnsignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ if constexpr (ReadOnly) {
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ return rocksdb::Status::InvalidArgument("Write bitfield in read-only mode.");
+ }
+
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_old_value));
+ } else {
+ ret = redis::Integer(unsigned_old_value);
+ }
+
+ rets->push_back(std::move(ret));
+ }
+ return rocksdb::Status::OK();
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ bitfieldWriteAheadLog(batch, ops);
+
+ // set bitfield value among segments
+ auto set = [&](uint32_t offset, BitfieldEncoding enc, BitfieldOverflowBehavior overflow, uint64_t value) {
+ BitmapSegmentsView view;
+ s = init_view(offset, enc.Bits(), &view);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ SetBitfield(view, first_bit_segment_offset, enc.Bits(), value);
+
+ for (auto segment = view.SegmentBegin(); segment != view.SegmentEnd(); ++segment) {
+ batch->Put((*segment)->first, (*segment)->second);
+ }
+
+ uint64_t segments_cnt = view.SegmentsCount();
+ uint64_t used_size = (offset / 8) + (segments_cnt - 1) * kBitmapSegmentBytes + view.LastSegmentSize();
+ if (metadata.size < used_size) {
+ metadata.size = used_size;
+ std::string bytes;
+ metadata.Encode(&bytes);
+ batch->Put(metadata_cf_handle_, ns_key, bytes);
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint64_t unsigned_new_value = 0;
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value)) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ s = set(op.offset, op.encoding, op.overflow, unsigned_new_value);
+ if (!s.ok()) {
+ return s;
+ }
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_new_value));
+ } else {
+ ret = redis::Integer(unsigned_new_value);
+ }
+ } else {
+ ret = redis::NilString();
+ }
Review Comment:
Should I use std::variant<nullptr_t, uint64_t, int64_t> or define a new type to handle this? There are 3 of types could store in result list (null, unsigned and signed). These two way above can store each result in 16 bytes. But it doesn't seem intuitive enough. (variant is unreadable. new type is hard to reuse, which fits for the
`Bitmap::Bitfield` function only)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1821002079
Thanks @julic20s . This patch is huge, and I need some time to review the whole patch. I just submit some small comments.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1822029595
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [8 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1400174130
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <stdexcept>
+#include <cstdint>
+#include <limits>
+#include <type_traits>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+struct BitfieldEncoding {
+ bool is_signed;
+ // 1 ~ 63 if unsigned, 1 ~ 64 if signed.
Review Comment:
Oh I misunderstanding this
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1829132618
Sure, I'm now becoming very busy on workdays, I'll finish review these few days.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1826208469
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [15 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1826806036
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice merged PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1825887556
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [15 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1405329621
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,306 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+#include "status.h"
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
Review Comment:
though noexcept is not widely used in kvrocks, since you mark `GetType()` and other as `noexcept`, should this function be `noexcept` as well?
##########
src/common/bitfield_util.cc:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include "bitfield_util.h"
+
+namespace detail {
+
+StatusOr<bool> SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst) {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kSigned, bits)) {
+ return Status::NotOK;
+ }
+
+ auto max = std::numeric_limits<int64_t>::max();
+ if (bits != 64) {
+ max = (static_cast<int64_t>(1) << (bits - 1)) - 1;
+ }
+ int64_t min = -max - 1;
+
+ int64_t signed_value = CastToSignedWithoutBitChanges(value);
+ int64_t max_incr = CastToSignedWithoutBitChanges(static_cast<uint64_t>(max) - value);
+ int64_t min_incr = min - signed_value;
+
+ auto wrap = [=]() {
+ uint64_t res = value + static_cast<uint64_t>(incr);
+ if (bits < 64) {
+ auto mask = std::numeric_limits<uint64_t>::max() << bits;
+ if ((res & (1 << (bits - 1))) != 0) {
+ res |= mask;
+ } else {
+ res &= ~mask;
+ }
+ }
+ return res;
+ };
+
Review Comment:
I tried hard to understand the logic here. The logic is same as redis logic, it tries to:
1. check whether `signed_value` itself overflows
2. if bits < 64, it would be ok to check `max_incr` without caring about underflow
3. do something like `checked_add/checked_minus` and then compare
Do you think just `checked_add` or `checked_sub` using int64_t by `incr` first, and then if that not overflow/underflow, comparing with min-max. Would that be more understandable?
##########
src/common/bitfield_util.cc:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include "bitfield_util.h"
+
+namespace detail {
+
+StatusOr<bool> SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst) {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kSigned, bits)) {
+ return Status::NotOK;
+ }
+
+ auto max = std::numeric_limits<int64_t>::max();
+ if (bits != 64) {
+ max = (static_cast<int64_t>(1) << (bits - 1)) - 1;
+ }
+ int64_t min = -max - 1;
+
+ int64_t signed_value = CastToSignedWithoutBitChanges(value);
+ int64_t max_incr = CastToSignedWithoutBitChanges(static_cast<uint64_t>(max) - value);
+ int64_t min_incr = min - signed_value;
+
+ auto wrap = [=]() {
Review Comment:
Expect extract `wrap` as a function here. Since it's logic can be test easily.
##########
src/common/bitfield_util.cc:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include "bitfield_util.h"
+
+namespace detail {
+
+StatusOr<bool> SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst) {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kSigned, bits)) {
+ return Status::NotOK;
+ }
+
+ auto max = std::numeric_limits<int64_t>::max();
+ if (bits != 64) {
+ max = (static_cast<int64_t>(1) << (bits - 1)) - 1;
+ }
+ int64_t min = -max - 1;
+
+ int64_t signed_value = CastToSignedWithoutBitChanges(value);
+ int64_t max_incr = CastToSignedWithoutBitChanges(static_cast<uint64_t>(max) - value);
+ int64_t min_incr = min - signed_value;
+
+ auto wrap = [=]() {
+ uint64_t res = value + static_cast<uint64_t>(incr);
+ if (bits < 64) {
+ auto mask = std::numeric_limits<uint64_t>::max() << bits;
+ if ((res & (1 << (bits - 1))) != 0) {
+ res |= mask;
+ } else {
+ res &= ~mask;
+ }
+ }
+ return res;
+ };
+
+ if (signed_value > max || (bits != 64 && incr > max_incr) || (signed_value >= 0 && incr >= 0 && incr > max_incr)) {
+ if (overflow == BitfieldOverflowBehavior::kWrap) {
+ *dst = wrap();
+ } else {
+ *dst = max;
+ }
+ return true;
+ } else if (signed_value < min || (bits != 64 && incr < min_incr) ||
+ (signed_value < 0 && incr < 0 && incr < min_incr)) {
+ if (overflow == BitfieldOverflowBehavior::kWrap) {
+ *dst = wrap();
+ } else {
+ *dst = min;
+ }
+ return true;
+ }
+
+ *dst = signed_value + incr;
+ return false;
+}
+
+// return true if overflow.
+StatusOr<bool> UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst) {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kUnsigned, bits)) {
+ return Status::NotOK;
+ }
+ auto max = (static_cast<uint64_t>(1) << bits) - 1;
+ int64_t max_incr = CastToSignedWithoutBitChanges(max - value);
+ int64_t min_incr = CastToSignedWithoutBitChanges((~value) + 1);
+
+ auto wrap = [=]() {
+ uint64_t mask = std::numeric_limits<uint64_t>::max() << bits;
+ uint64_t res = value + incr;
+ res &= ~mask;
+ return res;
+ };
+
+ if (value > max || (incr > 0 && incr > max_incr)) {
+ if (overflow == BitfieldOverflowBehavior::kWrap) {
Review Comment:
Should we `DCHECK` the behavior is not `kFail`?
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,306 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+#include "status.h"
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ static StatusOr<BitfieldEncoding> Create(Type type, uint8_t bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ return Status::NotOK;
Review Comment:
Nit: with a reason to help debugging?
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,306 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+#include "status.h"
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ static StatusOr<BitfieldEncoding> Create(Type type, uint8_t bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ return Status::NotOK;
+ }
+ BitfieldEncoding enc;
+ enc.type_ = type;
+ enc.bits_ = bits;
+ return enc;
+ }
+
+ Type GetType() const noexcept { return type_; }
+
+ bool IsSigned() const noexcept { return type_ == Type::kSigned; }
+
+ bool IsUnsigned() const noexcept { return type_ == Type::kUnsigned; }
+
+ uint8_t Bits() const noexcept { return bits_; }
+
+ Status SetBitsCount(uint8_t new_bits) {
+ if (!IsSupportedBitLengths(type_, new_bits)) {
+ return Status::NotOK;
+ }
+ bits_ = new_bits;
+ return Status::OK();
+ }
+
+ Status SetType(Type new_type) {
+ if (!IsSupportedBitLengths(new_type, bits_)) {
+ return Status::NotOK;
+ }
+ type_ = new_type;
+ return Status::OK();
+ }
+
+ std::string ToString() const noexcept { return (IsSigned() ? "i" : "u") + std::to_string(static_cast<int>(bits_)); }
+
+ private:
+ BitfieldEncoding() = default;
+
+ Type type_;
+ uint8_t bits_;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding{BitfieldEncoding::Create(BitfieldEncoding::Type::kSigned, 32).GetValue()};
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+} // namespace detail
+
+// safe cast from unsigned to signed, without any bit changes.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+inline int64_t CastToSignedWithoutBitChanges(uint64_t x) {
+ int64_t res = 0;
+ memcpy(&res, &x, sizeof(res));
+ return res;
+}
+
+class BitfieldValue {
+ public:
+ BitfieldValue(BitfieldEncoding encoding, uint64_t value) noexcept : encoding_(encoding), value_(value) {}
+
+ template <class T, std::enable_if_t<std::is_integral_v<T>, int> = 0>
+ bool operator==(T rhs) const {
+ return value_ == static_cast<uint64_t>(rhs);
+ }
+
+ template <class T>
+ friend bool operator==(T lhs, const BitfieldValue &rhs) {
+ return rhs == lhs;
+ }
+
+ BitfieldEncoding Encoding() const noexcept { return encoding_; }
+
+ uint64_t Value() const noexcept { return value_; }
+
+ private:
+ BitfieldEncoding encoding_;
+ uint64_t value_;
+};
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+inline StatusOr<bool> BitfieldPlus(uint64_t value, int64_t incr, BitfieldEncoding enc,
Review Comment:
This function needs more detailed document. It means like check "(value, enc) + incr"
##########
src/types/redis_bitmap.cc:
##########
@@ -187,17 +205,7 @@ rocksdb::Status Bitmap::SetBit(const Slice &user_key, uint32_t offset, bool new_
uint32_t byte_index = (offset / 8) % kBitmapSegmentBytes;
uint64_t used_size = index + byte_index + 1;
uint64_t bitmap_size = std::max(used_size, metadata.size);
- if (byte_index >= value.size()) { // expand the bitmap
- size_t expand_size = 0;
- if (byte_index >= value.size() * 2) {
- expand_size = byte_index - value.size() + 1;
- } else if (value.size() * 2 > kBitmapSegmentBytes) {
- expand_size = kBitmapSegmentBytes - value.size();
- } else {
- expand_size = value.size();
- }
- value.append(expand_size, 0);
- }
+ ExpandBitmapSegment(&value, byte_index + 1);
Review Comment:
@git-hulk Hmmm I'm a bit confused why original `expand_size = byte_index - value.size() + 1;` has a `+1` here?
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,306 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+#include "status.h"
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ static StatusOr<BitfieldEncoding> Create(Type type, uint8_t bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ return Status::NotOK;
+ }
+ BitfieldEncoding enc;
+ enc.type_ = type;
+ enc.bits_ = bits;
+ return enc;
+ }
+
+ Type GetType() const noexcept { return type_; }
+
+ bool IsSigned() const noexcept { return type_ == Type::kSigned; }
+
+ bool IsUnsigned() const noexcept { return type_ == Type::kUnsigned; }
+
+ uint8_t Bits() const noexcept { return bits_; }
+
+ Status SetBitsCount(uint8_t new_bits) {
+ if (!IsSupportedBitLengths(type_, new_bits)) {
+ return Status::NotOK;
+ }
+ bits_ = new_bits;
+ return Status::OK();
+ }
+
+ Status SetType(Type new_type) {
+ if (!IsSupportedBitLengths(new_type, bits_)) {
+ return Status::NotOK;
+ }
+ type_ = new_type;
+ return Status::OK();
+ }
+
+ std::string ToString() const noexcept { return (IsSigned() ? "i" : "u") + std::to_string(static_cast<int>(bits_)); }
+
+ private:
+ BitfieldEncoding() = default;
+
+ Type type_;
+ uint8_t bits_;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding{BitfieldEncoding::Create(BitfieldEncoding::Type::kSigned, 32).GetValue()};
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+} // namespace detail
+
+// safe cast from unsigned to signed, without any bit changes.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+inline int64_t CastToSignedWithoutBitChanges(uint64_t x) {
+ int64_t res = 0;
+ memcpy(&res, &x, sizeof(res));
+ return res;
+}
+
+class BitfieldValue {
+ public:
+ BitfieldValue(BitfieldEncoding encoding, uint64_t value) noexcept : encoding_(encoding), value_(value) {}
+
+ template <class T, std::enable_if_t<std::is_integral_v<T>, int> = 0>
+ bool operator==(T rhs) const {
+ return value_ == static_cast<uint64_t>(rhs);
+ }
+
+ template <class T>
+ friend bool operator==(T lhs, const BitfieldValue &rhs) {
+ return rhs == lhs;
+ }
+
+ BitfieldEncoding Encoding() const noexcept { return encoding_; }
+
+ uint64_t Value() const noexcept { return value_; }
+
+ private:
+ BitfieldEncoding encoding_;
+ uint64_t value_;
+};
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+inline StatusOr<bool> BitfieldPlus(uint64_t value, int64_t incr, BitfieldEncoding enc,
+ BitfieldOverflowBehavior overflow, uint64_t *dst) {
+ if (enc.IsSigned()) {
+ return detail::SignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+ }
+ return detail::UnsignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+}
+
+// return true if successful. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+inline StatusOr<bool> BitfieldOp(BitfieldOperation op, uint64_t old_value, uint64_t *new_value) {
+ if (op.type == BitfieldOperation::Type::kGet) {
+ *new_value = old_value;
+ return true;
+ }
+
+ bool overflow = false;
+ if (op.type == BitfieldOperation::Type::kSet) {
+ auto plus_res = BitfieldPlus(op.value, 0, op.encoding, op.overflow, new_value);
+ if (!plus_res) {
+ return plus_res;
+ }
Review Comment:
`GET_OR_RET` can used to simplify this code
##########
src/types/redis_bitmap.cc:
##########
@@ -38,6 +38,24 @@ const char kErrBitmapStringOutOfRange[] =
"The size of the bitmap string exceeds the "
"configuration item max-bitmap-to-string-mb";
+// min_bytes can not more than kBitmapSegmentBytes
+void ExpandBitmapSegment(std::string *segment, size_t min_bytes) {
+ assert(min_bytes <= kBitmapSegmentBytes);
+
+ auto old_size = segment->size();
+ if (min_bytes > old_size) {
+ size_t new_size = 0;
+ if (min_bytes > old_size * 2) {
+ new_size = min_bytes;
+ } else if (old_size * 2 > kBitmapSegmentBytes) {
+ new_size = kBitmapSegmentBytes;
+ } else {
+ new_size = old_size * 2;
+ }
+ segment->resize(new_size);
Review Comment:
explicit write `resize(new_size, 0)` might better here
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,306 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+#include "status.h"
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ static StatusOr<BitfieldEncoding> Create(Type type, uint8_t bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ return Status::NotOK;
+ }
+ BitfieldEncoding enc;
+ enc.type_ = type;
+ enc.bits_ = bits;
+ return enc;
+ }
+
+ Type GetType() const noexcept { return type_; }
+
+ bool IsSigned() const noexcept { return type_ == Type::kSigned; }
+
+ bool IsUnsigned() const noexcept { return type_ == Type::kUnsigned; }
+
+ uint8_t Bits() const noexcept { return bits_; }
+
+ Status SetBitsCount(uint8_t new_bits) {
+ if (!IsSupportedBitLengths(type_, new_bits)) {
+ return Status::NotOK;
+ }
+ bits_ = new_bits;
+ return Status::OK();
+ }
+
+ Status SetType(Type new_type) {
+ if (!IsSupportedBitLengths(new_type, bits_)) {
+ return Status::NotOK;
+ }
+ type_ = new_type;
+ return Status::OK();
+ }
+
+ std::string ToString() const noexcept { return (IsSigned() ? "i" : "u") + std::to_string(static_cast<int>(bits_)); }
+
+ private:
+ BitfieldEncoding() = default;
+
+ Type type_;
+ uint8_t bits_;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding{BitfieldEncoding::Create(BitfieldEncoding::Type::kSigned, 32).GetValue()};
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+} // namespace detail
+
+// safe cast from unsigned to signed, without any bit changes.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+inline int64_t CastToSignedWithoutBitChanges(uint64_t x) {
+ int64_t res = 0;
+ memcpy(&res, &x, sizeof(res));
+ return res;
+}
+
+class BitfieldValue {
+ public:
+ BitfieldValue(BitfieldEncoding encoding, uint64_t value) noexcept : encoding_(encoding), value_(value) {}
+
+ template <class T, std::enable_if_t<std::is_integral_v<T>, int> = 0>
+ bool operator==(T rhs) const {
+ return value_ == static_cast<uint64_t>(rhs);
+ }
+
+ template <class T>
+ friend bool operator==(T lhs, const BitfieldValue &rhs) {
+ return rhs == lhs;
+ }
+
+ BitfieldEncoding Encoding() const noexcept { return encoding_; }
+
+ uint64_t Value() const noexcept { return value_; }
+
+ private:
+ BitfieldEncoding encoding_;
+ uint64_t value_;
+};
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+inline StatusOr<bool> BitfieldPlus(uint64_t value, int64_t incr, BitfieldEncoding enc,
+ BitfieldOverflowBehavior overflow, uint64_t *dst) {
+ if (enc.IsSigned()) {
+ return detail::SignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+ }
+ return detail::UnsignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+}
+
+// return true if successful. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+inline StatusOr<bool> BitfieldOp(BitfieldOperation op, uint64_t old_value, uint64_t *new_value) {
+ if (op.type == BitfieldOperation::Type::kGet) {
+ *new_value = old_value;
+ return true;
+ }
+
+ bool overflow = false;
+ if (op.type == BitfieldOperation::Type::kSet) {
+ auto plus_res = BitfieldPlus(op.value, 0, op.encoding, op.overflow, new_value);
+ if (!plus_res) {
+ return plus_res;
+ }
+ overflow = plus_res.GetValue();
+ } else {
+ auto plus_res = BitfieldPlus(old_value, op.value, op.encoding, op.overflow, new_value);
+ if (!plus_res) {
+ return plus_res;
+ }
+ overflow = plus_res.GetValue();
+ }
+
+ return op.overflow != BitfieldOverflowBehavior::kFail || !overflow;
+}
+
+// Use a small buffer to store a range of bytes on a bitmap.
+// If you try to visit other place on bitmap. It will failed.
+class ArrayBitfieldBitmap {
+ public:
+ // offset is the byte offset of view in entire bitmap.
+ explicit ArrayBitfieldBitmap(uint32_t offset = 0) noexcept : offset_(offset) { Reset(); }
+
+ ArrayBitfieldBitmap(const ArrayBitfieldBitmap &) = delete;
+ ArrayBitfieldBitmap &operator=(const ArrayBitfieldBitmap &) = delete;
+ ~ArrayBitfieldBitmap() = default;
+
+ // Change the position this represents.
+ void SetOffset(uint64_t offset) noexcept { offset_ = offset; }
+
+ void Reset() { memset(buf_, 0, sizeof(buf_)); }
+
+ Status Set(uint32_t offset, uint32_t bytes, const uint8_t *src) {
+ if (isOutOfBound(offset, bytes)) {
+ return Status::NotOK;
+ }
+ offset -= offset_;
+ memcpy(buf_ + offset, src, bytes);
+ return Status::OK();
+ }
+
+ Status Get(uint32_t offset, uint32_t bytes, uint8_t *dst) const {
+ if (isOutOfBound(offset, bytes)) {
+ return Status::NotOK;
+ }
+ offset -= offset_;
+ memcpy(dst, buf_ + offset, bytes);
+ return Status::OK();
+ }
+
+ StatusOr<uint64_t> GetUnsignedBitfield(uint64_t offset, uint64_t bits) const {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kUnsigned, bits)) {
+ return Status::NotOK;
+ }
+ return getBitfield(offset, bits);
+ }
+
+ StatusOr<int64_t> GetSignedBitfield(uint64_t offset, uint64_t bits) const {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kSigned, bits)) {
+ return Status::NotOK;
+ }
+ auto status = getBitfield(offset, bits);
+ if (!status) {
+ return status;
+ }
+
+ int64_t value = CastToSignedWithoutBitChanges(status.GetValue());
+
+ // for a bits of k signed number, 1 << (bits - 1) is the MSB (most-significant bit).
+ // the number is a negative when the MSB is "1".
+ auto msb = static_cast<uint64_t>(1) << (bits - 1);
+ if ((value & msb) != 0) {
+ // The way of enlarge width of a signed integer is sign-extended.
+ // The values of higher bits should all "1", when the number is negative.
+ // For example:
+ // constexpr int32_t a = -128;
+ // static_assert(a == 0xffffff80);
+ // constexpr int64_t b = a; // b is -128 too.
+ // static_assert(b == 0xffffffffffffff80);
+ value |= CastToSignedWithoutBitChanges(std::numeric_limits<uint64_t>::max() << bits);
+ }
+ return value;
+ }
+
+ Status SetBitfield(uint32_t offset, uint32_t bits, uint64_t value) {
+ uint32_t first_byte = offset / 8;
+ uint32_t last_byte = (offset + bits - 1) / 8 + 1;
+ if (isOutOfBound(first_byte, last_byte - first_byte)) {
+ return Status::NotOK;
+ }
+
+ offset -= offset_ * 8;
+ while (bits--) {
+ bool v = (value & (uint64_t(1) << bits)) != 0;
+ uint32_t byte = offset >> 3;
+ uint32_t bit = 7 - (offset & 7);
+ uint32_t byteval = buf_[byte];
+ byteval &= ~(1 << bit);
+ byteval |= int(v) << bit;
+ buf_[byte] = char(byteval);
+ offset++;
+ }
+ return Status::OK();
+ }
+
+ private:
+ // The bit field cannot exceed 64 bits, takes an extra byte when it unaligned.
+ static constexpr uint32_t kSize = 8 + 1;
+
+ bool isOutOfBound(uint32_t offset, uint32_t bytes) const noexcept {
+ return offset < offset_ || offset_ + kSize < offset + bytes;
+ }
+
+ StatusOr<uint64_t> getBitfield(uint64_t offset, uint8_t bits) const {
+ uint32_t first_byte = offset / 8;
+ uint32_t last_byte = (offset + bits - 1) / 8 + 1;
+ if (isOutOfBound(first_byte, last_byte - first_byte)) {
+ return Status::NotOK;
Review Comment:
Maybe a error msg is better here.
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,306 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+#include "status.h"
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ static StatusOr<BitfieldEncoding> Create(Type type, uint8_t bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ return Status::NotOK;
+ }
+ BitfieldEncoding enc;
+ enc.type_ = type;
+ enc.bits_ = bits;
+ return enc;
+ }
+
+ Type GetType() const noexcept { return type_; }
+
+ bool IsSigned() const noexcept { return type_ == Type::kSigned; }
+
+ bool IsUnsigned() const noexcept { return type_ == Type::kUnsigned; }
+
+ uint8_t Bits() const noexcept { return bits_; }
+
+ Status SetBitsCount(uint8_t new_bits) {
+ if (!IsSupportedBitLengths(type_, new_bits)) {
+ return Status::NotOK;
+ }
+ bits_ = new_bits;
+ return Status::OK();
+ }
+
+ Status SetType(Type new_type) {
+ if (!IsSupportedBitLengths(new_type, bits_)) {
+ return Status::NotOK;
+ }
+ type_ = new_type;
+ return Status::OK();
+ }
+
+ std::string ToString() const noexcept { return (IsSigned() ? "i" : "u") + std::to_string(static_cast<int>(bits_)); }
+
+ private:
+ BitfieldEncoding() = default;
+
+ Type type_;
+ uint8_t bits_;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding{BitfieldEncoding::Create(BitfieldEncoding::Type::kSigned, 32).GetValue()};
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+} // namespace detail
+
+// safe cast from unsigned to signed, without any bit changes.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+inline int64_t CastToSignedWithoutBitChanges(uint64_t x) {
+ int64_t res = 0;
+ memcpy(&res, &x, sizeof(res));
+ return res;
+}
+
+class BitfieldValue {
+ public:
+ BitfieldValue(BitfieldEncoding encoding, uint64_t value) noexcept : encoding_(encoding), value_(value) {}
+
+ template <class T, std::enable_if_t<std::is_integral_v<T>, int> = 0>
+ bool operator==(T rhs) const {
+ return value_ == static_cast<uint64_t>(rhs);
+ }
+
+ template <class T>
+ friend bool operator==(T lhs, const BitfieldValue &rhs) {
+ return rhs == lhs;
+ }
+
+ BitfieldEncoding Encoding() const noexcept { return encoding_; }
+
+ uint64_t Value() const noexcept { return value_; }
+
+ private:
+ BitfieldEncoding encoding_;
+ uint64_t value_;
+};
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+inline StatusOr<bool> BitfieldPlus(uint64_t value, int64_t incr, BitfieldEncoding enc,
+ BitfieldOverflowBehavior overflow, uint64_t *dst) {
+ if (enc.IsSigned()) {
+ return detail::SignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+ }
+ return detail::UnsignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+}
+
+// return true if successful. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+inline StatusOr<bool> BitfieldOp(BitfieldOperation op, uint64_t old_value, uint64_t *new_value) {
+ if (op.type == BitfieldOperation::Type::kGet) {
+ *new_value = old_value;
+ return true;
+ }
+
+ bool overflow = false;
+ if (op.type == BitfieldOperation::Type::kSet) {
+ auto plus_res = BitfieldPlus(op.value, 0, op.encoding, op.overflow, new_value);
+ if (!plus_res) {
+ return plus_res;
+ }
+ overflow = plus_res.GetValue();
+ } else {
+ auto plus_res = BitfieldPlus(old_value, op.value, op.encoding, op.overflow, new_value);
+ if (!plus_res) {
+ return plus_res;
+ }
+ overflow = plus_res.GetValue();
+ }
+
+ return op.overflow != BitfieldOverflowBehavior::kFail || !overflow;
+}
+
+// Use a small buffer to store a range of bytes on a bitmap.
+// If you try to visit other place on bitmap. It will failed.
+class ArrayBitfieldBitmap {
+ public:
+ // offset is the byte offset of view in entire bitmap.
+ explicit ArrayBitfieldBitmap(uint32_t offset = 0) noexcept : offset_(offset) { Reset(); }
+
+ ArrayBitfieldBitmap(const ArrayBitfieldBitmap &) = delete;
+ ArrayBitfieldBitmap &operator=(const ArrayBitfieldBitmap &) = delete;
+ ~ArrayBitfieldBitmap() = default;
+
+ // Change the position this represents.
+ void SetOffset(uint64_t offset) noexcept { offset_ = offset; }
+
+ void Reset() { memset(buf_, 0, sizeof(buf_)); }
+
+ Status Set(uint32_t offset, uint32_t bytes, const uint8_t *src) {
+ if (isOutOfBound(offset, bytes)) {
+ return Status::NotOK;
+ }
+ offset -= offset_;
+ memcpy(buf_ + offset, src, bytes);
+ return Status::OK();
+ }
+
+ Status Get(uint32_t offset, uint32_t bytes, uint8_t *dst) const {
+ if (isOutOfBound(offset, bytes)) {
+ return Status::NotOK;
+ }
+ offset -= offset_;
+ memcpy(dst, buf_ + offset, bytes);
+ return Status::OK();
+ }
+
+ StatusOr<uint64_t> GetUnsignedBitfield(uint64_t offset, uint64_t bits) const {
Review Comment:
Can we extract `bit_offset` and byte_offset in variable name?
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,306 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+#include "status.h"
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ static StatusOr<BitfieldEncoding> Create(Type type, uint8_t bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ return Status::NotOK;
+ }
+ BitfieldEncoding enc;
+ enc.type_ = type;
+ enc.bits_ = bits;
+ return enc;
+ }
+
+ Type GetType() const noexcept { return type_; }
+
+ bool IsSigned() const noexcept { return type_ == Type::kSigned; }
+
+ bool IsUnsigned() const noexcept { return type_ == Type::kUnsigned; }
+
+ uint8_t Bits() const noexcept { return bits_; }
+
+ Status SetBitsCount(uint8_t new_bits) {
+ if (!IsSupportedBitLengths(type_, new_bits)) {
+ return Status::NotOK;
+ }
+ bits_ = new_bits;
+ return Status::OK();
+ }
+
+ Status SetType(Type new_type) {
+ if (!IsSupportedBitLengths(new_type, bits_)) {
+ return Status::NotOK;
+ }
+ type_ = new_type;
+ return Status::OK();
+ }
+
+ std::string ToString() const noexcept { return (IsSigned() ? "i" : "u") + std::to_string(static_cast<int>(bits_)); }
+
+ private:
+ BitfieldEncoding() = default;
+
+ Type type_;
+ uint8_t bits_;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding{BitfieldEncoding::Create(BitfieldEncoding::Type::kSigned, 32).GetValue()};
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+StatusOr<bool> UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow,
+ uint64_t *dst);
+} // namespace detail
+
+// safe cast from unsigned to signed, without any bit changes.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+inline int64_t CastToSignedWithoutBitChanges(uint64_t x) {
+ int64_t res = 0;
+ memcpy(&res, &x, sizeof(res));
+ return res;
+}
+
+class BitfieldValue {
+ public:
+ BitfieldValue(BitfieldEncoding encoding, uint64_t value) noexcept : encoding_(encoding), value_(value) {}
+
+ template <class T, std::enable_if_t<std::is_integral_v<T>, int> = 0>
+ bool operator==(T rhs) const {
+ return value_ == static_cast<uint64_t>(rhs);
+ }
+
+ template <class T>
+ friend bool operator==(T lhs, const BitfieldValue &rhs) {
+ return rhs == lhs;
+ }
+
+ BitfieldEncoding Encoding() const noexcept { return encoding_; }
+
+ uint64_t Value() const noexcept { return value_; }
+
+ private:
+ BitfieldEncoding encoding_;
+ uint64_t value_;
+};
+
+// return true if overflow. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+inline StatusOr<bool> BitfieldPlus(uint64_t value, int64_t incr, BitfieldEncoding enc,
+ BitfieldOverflowBehavior overflow, uint64_t *dst) {
+ if (enc.IsSigned()) {
+ return detail::SignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+ }
+ return detail::UnsignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+}
+
+// return true if successful. Status is not ok iff calling BitfieldEncoding::IsSupportedBitLengths()
+// for given op return false.
+inline StatusOr<bool> BitfieldOp(BitfieldOperation op, uint64_t old_value, uint64_t *new_value) {
+ if (op.type == BitfieldOperation::Type::kGet) {
+ *new_value = old_value;
+ return true;
+ }
+
+ bool overflow = false;
+ if (op.type == BitfieldOperation::Type::kSet) {
+ auto plus_res = BitfieldPlus(op.value, 0, op.encoding, op.overflow, new_value);
+ if (!plus_res) {
+ return plus_res;
+ }
+ overflow = plus_res.GetValue();
+ } else {
+ auto plus_res = BitfieldPlus(old_value, op.value, op.encoding, op.overflow, new_value);
+ if (!plus_res) {
+ return plus_res;
+ }
+ overflow = plus_res.GetValue();
+ }
+
+ return op.overflow != BitfieldOverflowBehavior::kFail || !overflow;
+}
+
+// Use a small buffer to store a range of bytes on a bitmap.
+// If you try to visit other place on bitmap. It will failed.
+class ArrayBitfieldBitmap {
+ public:
+ // offset is the byte offset of view in entire bitmap.
+ explicit ArrayBitfieldBitmap(uint32_t offset = 0) noexcept : offset_(offset) { Reset(); }
+
+ ArrayBitfieldBitmap(const ArrayBitfieldBitmap &) = delete;
+ ArrayBitfieldBitmap &operator=(const ArrayBitfieldBitmap &) = delete;
+ ~ArrayBitfieldBitmap() = default;
+
+ // Change the position this represents.
+ void SetOffset(uint64_t offset) noexcept { offset_ = offset; }
+
+ void Reset() { memset(buf_, 0, sizeof(buf_)); }
+
+ Status Set(uint32_t offset, uint32_t bytes, const uint8_t *src) {
+ if (isOutOfBound(offset, bytes)) {
+ return Status::NotOK;
+ }
+ offset -= offset_;
+ memcpy(buf_ + offset, src, bytes);
+ return Status::OK();
+ }
+
+ Status Get(uint32_t offset, uint32_t bytes, uint8_t *dst) const {
+ if (isOutOfBound(offset, bytes)) {
+ return Status::NotOK;
+ }
+ offset -= offset_;
+ memcpy(dst, buf_ + offset, bytes);
+ return Status::OK();
+ }
+
+ StatusOr<uint64_t> GetUnsignedBitfield(uint64_t offset, uint64_t bits) const {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kUnsigned, bits)) {
+ return Status::NotOK;
+ }
+ return getBitfield(offset, bits);
+ }
+
+ StatusOr<int64_t> GetSignedBitfield(uint64_t offset, uint64_t bits) const {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kSigned, bits)) {
+ return Status::NotOK;
+ }
+ auto status = getBitfield(offset, bits);
+ if (!status) {
+ return status;
+ }
Review Comment:
Using `GET_OR_RET` here
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1828965341
@mapleFU Hi, I finished all changes you pointed, could you review them again? Thanks.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1826208741
@git-hulk Could you help me approval the workflow? I fix some problems that I found. But I can't reproduction the unittest failed on my local machine.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1400162162
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <stdexcept>
+#include <cstdint>
+#include <limits>
+#include <type_traits>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+struct BitfieldEncoding {
+ bool is_signed;
+ // 1 ~ 63 if unsigned, 1 ~ 64 if signed.
Review Comment:
> Should we `static_assert(sizeof(...))` for this?
I don't get it. Should I add likes static_assert(sizeof(BitfieldEncoding) == 2)?
The comment in code means redis only support i1\~i64 and u1\~u63 in bitfield.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1820736699
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [15 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1822287175
> The mutex pool in lock manager may be reallocated by MultiGet(). Then the old mutex pointers in LockGuard all be invalid...I guess that's why I failed the test. Should I use a std::deque to replace the vector?
Guess you should use optional<LockGuard> instead of LockGuard directly in https://github.com/apache/kvrocks/pull/1901/files#diff-642915be939e08850c3ce460033f3cdf0969ab9bfa67b63639e82ab9daf454f6R606-R609.
Or it will release the mutex after exiting the if block, and exiting this function will cause to free the mutex again.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1822314669
@julic20s Guess not, you should use `std::optional<LockGuard>` instead of LockGuard directly since the LockGuard constructor will lock the mutex, and if you use LockGuard = default, it won't lock the mutex at all.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1837516107
Thank you for your contribution!
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1824401970
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [23 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1400572408
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldBitsLengthNotFitExecption : public std::runtime_error {
+ public:
+ BitfieldBitsLengthNotFitExecption() : std::runtime_error("bits length is not fitted to bitfield.") {}
+};
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ BitfieldEncoding(Type type, uint8_t bits) : type_(type), bits_(bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ }
+
+ bool IsSigned() const noexcept { return type_ == Type::kSigned; }
+
+ uint8_t Bits() const noexcept { return bits_; }
+
+ void SetBits(uint8_t new_bits) {
+ if (!IsSupportedBitLengths(type_, new_bits)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ bits_ = new_bits;
+ }
+
+ void SetType(Type new_type) {
+ if (!IsSupportedBitLengths(new_type, bits_)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ type_ = new_type;
+ }
+
+ std::string ToString() const noexcept { return (IsSigned() ? "i" : "u") + std::to_string(static_cast<int>(bits_)); }
+
+ private:
+ Type type_;
+ uint8_t bits_;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding{BitfieldEncoding::Type::kSigned, 32};
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+// return true if overflow
+bool SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow, uint64_t *dst);
+
+// return true if overflow.
+bool UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow, uint64_t *dst);
+} // namespace detail
+
+// safe cast from unsigned to signed, without any bit changes.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+inline int64_t CastToSignedWithoutBitChanges(uint64_t x) {
+ int64_t res = 0;
+ memcpy(&res, &x, sizeof(res));
+ return res;
+}
+
+// return true if overflow.
+inline auto BitfieldPlus(uint64_t value, int64_t incr, BitfieldEncoding enc, BitfieldOverflowBehavior overflow,
Review Comment:
Better using `bool` as response explicit?
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldBitsLengthNotFitExecption : public std::runtime_error {
Review Comment:
Hmmm for Style kvrocks uses `Status` rather than Exception, would you mind using status to wrap this?
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldBitsLengthNotFitExecption : public std::runtime_error {
+ public:
+ BitfieldBitsLengthNotFitExecption() : std::runtime_error("bits length is not fitted to bitfield.") {}
+};
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ BitfieldEncoding(Type type, uint8_t bits) : type_(type), bits_(bits) {
Review Comment:
This can be wrapped as a `Result<BitfieldEncoding>` builder, and `BitfieldEncoding` as a private ctor.
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldBitsLengthNotFitExecption : public std::runtime_error {
+ public:
+ BitfieldBitsLengthNotFitExecption() : std::runtime_error("bits length is not fitted to bitfield.") {}
+};
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ BitfieldEncoding(Type type, uint8_t bits) : type_(type), bits_(bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ }
+
+ bool IsSigned() const noexcept { return type_ == Type::kSigned; }
+
+ uint8_t Bits() const noexcept { return bits_; }
+
+ void SetBits(uint8_t new_bits) {
+ if (!IsSupportedBitLengths(type_, new_bits)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ bits_ = new_bits;
+ }
+
+ void SetType(Type new_type) {
+ if (!IsSupportedBitLengths(new_type, bits_)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ type_ = new_type;
+ }
+
+ std::string ToString() const noexcept { return (IsSigned() ? "i" : "u") + std::to_string(static_cast<int>(bits_)); }
+
+ private:
+ Type type_;
+ uint8_t bits_;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding{BitfieldEncoding::Type::kSigned, 32};
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+// return true if overflow
+bool SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow, uint64_t *dst);
+
+// return true if overflow.
+bool UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow, uint64_t *dst);
+} // namespace detail
+
+// safe cast from unsigned to signed, without any bit changes.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+inline int64_t CastToSignedWithoutBitChanges(uint64_t x) {
+ int64_t res = 0;
+ memcpy(&res, &x, sizeof(res));
+ return res;
+}
+
+// return true if overflow.
+inline auto BitfieldPlus(uint64_t value, int64_t incr, BitfieldEncoding enc, BitfieldOverflowBehavior overflow,
+ uint64_t *dst) {
+ if (enc.IsSigned()) {
+ return detail::SignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+ }
+ return detail::UnsignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+}
+
+// return true if successful
+inline bool BitfieldOp(BitfieldOperation op, uint64_t old_value, uint64_t *new_value) {
+ if (op.type == BitfieldOperation::Type::kGet) {
+ *new_value = old_value;
+ return true;
+ }
+
+ bool overflow = false;
+ if (op.type == BitfieldOperation::Type::kSet) {
+ overflow = BitfieldPlus(op.value, 0, op.encoding, op.overflow, new_value);
+ } else {
+ overflow = BitfieldPlus(old_value, op.value, op.encoding, op.overflow, new_value);
+ }
+
+ return op.overflow != BitfieldOverflowBehavior::kFail || !overflow;
+}
+
+template <class BitmapView>
Review Comment:
We can leave a todo here for optimizations. I didn't try it in `-O3` but it looks slow if bits isn't small
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldBitsLengthNotFitExecption : public std::runtime_error {
+ public:
+ BitfieldBitsLengthNotFitExecption() : std::runtime_error("bits length is not fitted to bitfield.") {}
+};
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ BitfieldEncoding(Type type, uint8_t bits) : type_(type), bits_(bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ }
+
+ bool IsSigned() const noexcept { return type_ == Type::kSigned; }
+
+ uint8_t Bits() const noexcept { return bits_; }
+
+ void SetBits(uint8_t new_bits) {
+ if (!IsSupportedBitLengths(type_, new_bits)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ bits_ = new_bits;
+ }
+
+ void SetType(Type new_type) {
+ if (!IsSupportedBitLengths(new_type, bits_)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ type_ = new_type;
+ }
+
+ std::string ToString() const noexcept { return (IsSigned() ? "i" : "u") + std::to_string(static_cast<int>(bits_)); }
+
+ private:
+ Type type_;
+ uint8_t bits_;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding{BitfieldEncoding::Type::kSigned, 32};
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+// return true if overflow
+bool SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow, uint64_t *dst);
+
+// return true if overflow.
+bool UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow, uint64_t *dst);
+} // namespace detail
+
+// safe cast from unsigned to signed, without any bit changes.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+inline int64_t CastToSignedWithoutBitChanges(uint64_t x) {
+ int64_t res = 0;
+ memcpy(&res, &x, sizeof(res));
+ return res;
+}
+
+// return true if overflow.
+inline auto BitfieldPlus(uint64_t value, int64_t incr, BitfieldEncoding enc, BitfieldOverflowBehavior overflow,
+ uint64_t *dst) {
+ if (enc.IsSigned()) {
+ return detail::SignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+ }
+ return detail::UnsignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+}
+
+// return true if successful
+inline bool BitfieldOp(BitfieldOperation op, uint64_t old_value, uint64_t *new_value) {
+ if (op.type == BitfieldOperation::Type::kGet) {
+ *new_value = old_value;
+ return true;
+ }
+
+ bool overflow = false;
+ if (op.type == BitfieldOperation::Type::kSet) {
+ overflow = BitfieldPlus(op.value, 0, op.encoding, op.overflow, new_value);
+ } else {
+ overflow = BitfieldPlus(old_value, op.value, op.encoding, op.overflow, new_value);
+ }
+
+ return op.overflow != BitfieldOverflowBehavior::kFail || !overflow;
+}
+
+template <class BitmapView>
+inline void SetBitfield(BitmapView &view, uint32_t offset, uint32_t bits, uint64_t value) {
+ while (bits--) {
+ bool v = (value & (uint64_t(1) << bits)) != 0;
+ uint32_t byte = offset >> 3;
+ uint32_t bit = 7 - (offset & 7);
+ uint32_t byteval = view[byte];
+ byteval &= ~(1 << bit);
+ byteval |= int(v) << bit;
+ view[byte] = char(byteval);
+ offset++;
+ }
+}
+
+template <class BitmapView>
+inline uint64_t GetUnsignedBitfield(const BitmapView &view, uint64_t offset, uint64_t bits) {
Review Comment:
ditto, we can leave a todo here?
##########
src/types/redis_bitmap.cc:
##########
@@ -38,6 +39,22 @@ const char kErrBitmapStringOutOfRange[] =
"The size of the bitmap string exceeds the "
"configuration item max-bitmap-to-string-mb";
+// min_bytes can not more than kBitmapSegmentBytes
+void ExpandBitmapSegment(std::string *segment, size_t min_bytes) {
+ auto old_size = segment->size();
+ if (min_bytes > old_size) {
+ size_t expand_size = 0;
+ if (min_bytes > old_size * 2) {
+ expand_size = min_bytes - old_size;
+ } else if (old_size * 2 > kBitmapSegmentBytes) {
+ expand_size = kBitmapSegmentBytes - old_size;
Review Comment:
I think we need to check or dcheck `kBitmapSegmentBytes` and size here (as an separate helper function, more checks seems better)
##########
src/common/bitfield_util.h:
##########
@@ -0,0 +1,180 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+#include <limits>
+#include <stdexcept>
+#include <string>
+
+enum class BitfieldOverflowBehavior : uint8_t { kWrap, kSat, kFail };
+
+class BitfieldBitsLengthNotFitExecption : public std::runtime_error {
+ public:
+ BitfieldBitsLengthNotFitExecption() : std::runtime_error("bits length is not fitted to bitfield.") {}
+};
+
+class BitfieldEncoding {
+ public:
+ enum class Type : uint8_t { kSigned, kUnsigned };
+
+ // check whether the bit length is fit to given number sign.
+ // Redis has bits length limitation to bitfield.
+ // Quote:
+ // The supported encodings are up to 64 bits for signed integers, and up to 63 bits for unsigned integers. This
+ // limitation with unsigned integers is due to the fact that currently the Redis protocol is unable to return 64 bit
+ // unsigned integers as replies.
+ // see also https://redis.io/commands/bitfield/
+ static bool IsSupportedBitLengths(Type type, uint8_t bits) {
+ uint8_t max_bits = 64;
+ if (type == Type::kUnsigned) {
+ max_bits = 63;
+ }
+ return 1 <= bits && bits <= max_bits;
+ }
+
+ BitfieldEncoding(Type type, uint8_t bits) : type_(type), bits_(bits) {
+ if (!IsSupportedBitLengths(type, bits)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ }
+
+ bool IsSigned() const noexcept { return type_ == Type::kSigned; }
+
+ uint8_t Bits() const noexcept { return bits_; }
+
+ void SetBits(uint8_t new_bits) {
+ if (!IsSupportedBitLengths(type_, new_bits)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ bits_ = new_bits;
+ }
+
+ void SetType(Type new_type) {
+ if (!IsSupportedBitLengths(new_type, bits_)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ type_ = new_type;
+ }
+
+ std::string ToString() const noexcept { return (IsSigned() ? "i" : "u") + std::to_string(static_cast<int>(bits_)); }
+
+ private:
+ Type type_;
+ uint8_t bits_;
+};
+
+struct BitfieldOperation {
+ // see https://redis.io/commands/bitfield/ to get more details.
+ enum class Type : uint8_t { kGet, kSet, kIncrBy };
+
+ Type type;
+ BitfieldOverflowBehavior overflow{BitfieldOverflowBehavior::kWrap};
+ BitfieldEncoding encoding{BitfieldEncoding::Type::kSigned, 32};
+ uint32_t offset;
+ // INCRBY amount or SET value
+ int64_t value;
+};
+
+namespace detail {
+// return true if overflow
+bool SignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow, uint64_t *dst);
+
+// return true if overflow.
+bool UnsignedBitfieldPlus(uint64_t value, int64_t incr, uint8_t bits, BitfieldOverflowBehavior overflow, uint64_t *dst);
+} // namespace detail
+
+// safe cast from unsigned to signed, without any bit changes.
+// see also "Integral conversions" on https://en.cppreference.com/w/cpp/language/implicit_conversion
+// If the destination type is signed, the result when overflow is implementation-defined until C++20
+inline int64_t CastToSignedWithoutBitChanges(uint64_t x) {
+ int64_t res = 0;
+ memcpy(&res, &x, sizeof(res));
+ return res;
+}
+
+// return true if overflow.
+inline auto BitfieldPlus(uint64_t value, int64_t incr, BitfieldEncoding enc, BitfieldOverflowBehavior overflow,
+ uint64_t *dst) {
+ if (enc.IsSigned()) {
+ return detail::SignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+ }
+ return detail::UnsignedBitfieldPlus(value, incr, enc.Bits(), overflow, dst);
+}
+
+// return true if successful
+inline bool BitfieldOp(BitfieldOperation op, uint64_t old_value, uint64_t *new_value) {
+ if (op.type == BitfieldOperation::Type::kGet) {
+ *new_value = old_value;
+ return true;
+ }
+
+ bool overflow = false;
+ if (op.type == BitfieldOperation::Type::kSet) {
+ overflow = BitfieldPlus(op.value, 0, op.encoding, op.overflow, new_value);
+ } else {
+ overflow = BitfieldPlus(old_value, op.value, op.encoding, op.overflow, new_value);
+ }
+
+ return op.overflow != BitfieldOverflowBehavior::kFail || !overflow;
+}
+
+template <class BitmapView>
+inline void SetBitfield(BitmapView &view, uint32_t offset, uint32_t bits, uint64_t value) {
+ while (bits--) {
+ bool v = (value & (uint64_t(1) << bits)) != 0;
+ uint32_t byte = offset >> 3;
+ uint32_t bit = 7 - (offset & 7);
+ uint32_t byteval = view[byte];
+ byteval &= ~(1 << bit);
+ byteval |= int(v) << bit;
+ view[byte] = char(byteval);
+ offset++;
+ }
+}
+
+template <class BitmapView>
+inline uint64_t GetUnsignedBitfield(const BitmapView &view, uint64_t offset, uint64_t bits) {
+ uint64_t value = 0;
+ while (bits--) {
+ uint32_t byte = offset >> 3;
+ uint32_t bit = 7 - (offset & 0x7);
+ uint32_t byteval = view[byte];
+ uint32_t bitval = (byteval >> bit) & 1;
+ value = (value << 1) | bitval;
+ offset++;
+ }
+ return value;
+}
+
+template <class BitmapView>
+inline int64_t GetSignedBitfield(const BitmapView &view, uint64_t offset, uint64_t bits) {
+ if (!BitfieldEncoding::IsSupportedBitLengths(BitfieldEncoding::Type::kSigned, bits)) {
+ throw BitfieldBitsLengthNotFitExecption();
+ }
+ int64_t value = CastToSignedWithoutBitChanges(GetUnsignedBitfield(view, offset, bits));
+
+ if ((value & (static_cast<uint64_t>(1) << (bits - 1))) != 0) {
+ value |= CastToSignedWithoutBitChanges(std::numeric_limits<uint64_t>::max() << bits);
+ }
Review Comment:
Hmmm this is a bit tricky here I think...Can we add some comment here?
##########
src/storage/batch_extractor.cc:
##########
@@ -232,6 +232,10 @@ rocksdb::Status WriteBatchExtractor::PutCF(uint32_t column_family_id, const Slic
first_seen_ = false;
}
break;
+ case kRedisCmdBitfield:
Review Comment:
Would you mind testing the slot-migration case here?
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1822315476
Oh... I define a default move assignment. Which cause a dangling pointer
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1400663151
##########
src/commands/cmd_bit.cc:
##########
@@ -225,10 +227,145 @@ class CommandBitOp : public Commander {
BitOpFlags op_flag_;
};
+class CommandBitfield : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ BitfieldOperation cmd;
+
+ read_only_ = true;
+ // BITFIELD <key> [commands...]
+ for (auto group = args.begin() + 2; group != args.end();) {
Review Comment:
Though this code LGTM, we need to check the argument of the number of `group` doesn't visit the `args.end()`. Would you mind just using the `Parser` here?
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1401423417
##########
src/types/redis_bitmap.cc:
##########
@@ -538,6 +545,281 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+namespace {
+using SegmentCache = std::pair<std::string, std::string>;
+
+// used to simplify accessing bitfield that occupies multiple continuous segments.
+// you can access multiple segments like accessing in one big segments.
+class BitmapSegmentsView {
+ public:
+ BitmapSegmentsView() = default;
+ explicit BitmapSegmentsView(std::vector<SegmentCache *> segments, bool expand_in_advance = true)
+ : segments_(std::move(segments)) {
+ if (expand_in_advance && segments_.size() > 1) {
+ // expand segments during iteration (not in advance) maybe increase allocation times.
+ // we assume that all end of each segment except last one will be accessed.
+ // note: segments_.size() is unsigned.
+ for (size_t i = 0; i < segments_.size() - 1; ++i) {
+ ExpandBitmapSegment(&segments_[i]->second, kBitmapSegmentBytes);
+ }
+ }
+ }
+
+ char &operator[](uint32_t index) noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ const char &operator[](uint32_t index) const noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ SegmentCache &GetSegment(uint32_t index) noexcept { return *segments_[index]; }
+
+ const SegmentCache &GetSegment(uint32_t index) const noexcept { return *segments_[index]; }
+
+ size_t LastSegmentSize() const noexcept { return segments_.back()->second.size(); }
+
+ size_t SegmentsCount() const noexcept { return segments_.size(); }
+
+ auto SegmentBegin() noexcept { return segments_.begin(); }
+
+ auto SegmentEnd() noexcept { return segments_.end(); }
+
+ private:
+ std::vector<SegmentCache *> segments_;
+};
+} // namespace
+
+template <bool ReadOnly>
+rocksdb::Status Bitmap::bitfield(const Slice &user_key, const std::vector<BitfieldOperation> &ops,
+ std::vector<std::string> *rets) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
+
+ LockGuard guard;
+ if constexpr (!ReadOnly) {
+ guard = LockGuard(storage_->GetLockManager(), ns_key);
+ }
+
+ BitmapMetadata metadata;
+ std::string raw_value;
+ auto s = GetMetadata(ns_key, &metadata, &raw_value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+
+ if (metadata.Type() == RedisType::kRedisString) {
+ if constexpr (ReadOnly) {
+ s = BitmapString::BitfieldReadOnly(ns_key, raw_value, ops, rets);
+ } else {
+ s = BitmapString(storage_, namespace_).Bitfield(ns_key, &raw_value, ops, rets);
+ }
+ return s;
+ }
+
+ if (metadata.Type() != RedisType::kRedisBitmap) {
+ return rocksdb::Status::InvalidArgument("The value is not a bitmap or string.");
+ }
+
+ std::unordered_map<uint32_t, SegmentCache> seg_cache;
+ auto get_segment = [&](uint32_t index, SegmentCache **res) {
+ auto [seg_itor, no_cache] = seg_cache.try_emplace(index);
+ auto &[sub_key, value] = seg_itor->second;
+ if (no_cache) {
+ auto sub_sub_key = std::to_string(index * kBitmapSegmentBytes);
+ sub_key = InternalKey(ns_key, sub_sub_key, metadata.version, storage_->IsSlotIdEncoded()).Encode();
+ rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(), sub_key, &value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+ }
+ *res = &seg_itor->second;
+ return rocksdb::Status::OK();
+ };
+
+ auto init_view = [&get_segment](uint32_t offset, uint32_t bits, BitmapSegmentsView *view) {
+ uint32_t first_segment = offset / kBitmapSegmentBits;
+ uint32_t last_segment = (offset + bits - 1) / kBitmapSegmentBits;
+
+ std::vector<SegmentCache *> segments(last_segment - first_segment + 1);
+ for (auto &seg : segments) {
+ auto seg_status = get_segment(first_segment++, &seg);
+ if (!seg_status.ok()) {
+ return seg_status;
+ }
+ }
+ *view = BitmapSegmentsView(std::move(segments));
+ return rocksdb::Status::OK();
+ };
+
+ // get bitfield value from entire bitmap
+ auto get = [&init_view](uint32_t offset, BitfieldEncoding enc, uint64_t *res) {
+ BitmapSegmentsView view;
+ auto view_status = init_view(offset, enc.Bits(), &view);
+ if (!view_status.ok()) {
+ return view_status;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ if (enc.IsSigned()) {
+ *res = GetSignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ } else {
+ *res = GetUnsignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ if constexpr (ReadOnly) {
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ return rocksdb::Status::InvalidArgument("Write bitfield in read-only mode.");
+ }
+
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_old_value));
+ } else {
+ ret = redis::Integer(unsigned_old_value);
+ }
+
+ rets->push_back(std::move(ret));
+ }
+ return rocksdb::Status::OK();
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ bitfieldWriteAheadLog(batch, ops);
+
+ // set bitfield value among segments
+ auto set = [&](uint32_t offset, BitfieldEncoding enc, BitfieldOverflowBehavior overflow, uint64_t value) {
+ BitmapSegmentsView view;
+ s = init_view(offset, enc.Bits(), &view);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ SetBitfield(view, first_bit_segment_offset, enc.Bits(), value);
+
+ for (auto segment = view.SegmentBegin(); segment != view.SegmentEnd(); ++segment) {
+ batch->Put((*segment)->first, (*segment)->second);
+ }
+
+ uint64_t segments_cnt = view.SegmentsCount();
+ uint64_t used_size = (offset / 8) + (segments_cnt - 1) * kBitmapSegmentBytes + view.LastSegmentSize();
+ if (metadata.size < used_size) {
+ metadata.size = used_size;
+ std::string bytes;
+ metadata.Encode(&bytes);
+ batch->Put(metadata_cf_handle_, ns_key, bytes);
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint64_t unsigned_new_value = 0;
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value)) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ s = set(op.offset, op.encoding, op.overflow, unsigned_new_value);
+ if (!s.ok()) {
+ return s;
+ }
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_new_value));
+ } else {
+ ret = redis::Integer(unsigned_new_value);
+ }
+ } else {
+ ret = redis::NilString();
+ }
Review Comment:
Should I use std::variant<nullptr_t, uint64_t, int64_t> or define a new type to handle this? There are 3 of types could store in result list (null, unsigned and signed). These two way above can store result in 16 bytes. But it doesn't seem intuitive enough.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1825147601
I'm a bit busy on workdays, I'll go through a pass this weekend. If other thinks it's ok we can review an merge it first
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1825889362
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [15 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1405347883
##########
src/types/redis_bitmap.cc:
##########
@@ -187,17 +205,7 @@ rocksdb::Status Bitmap::SetBit(const Slice &user_key, uint32_t offset, bool new_
uint32_t byte_index = (offset / 8) % kBitmapSegmentBytes;
uint64_t used_size = index + byte_index + 1;
uint64_t bitmap_size = std::max(used_size, metadata.size);
- if (byte_index >= value.size()) { // expand the bitmap
- size_t expand_size = 0;
- if (byte_index >= value.size() * 2) {
- expand_size = byte_index - value.size() + 1;
- } else if (value.size() * 2 > kBitmapSegmentBytes) {
- expand_size = kBitmapSegmentBytes - value.size();
- } else {
- expand_size = value.size();
- }
- value.append(expand_size, 0);
- }
+ ExpandBitmapSegment(&value, byte_index + 1);
Review Comment:
byte_index is the last index will be used. It can be explained as `expand_size = (byte_index + 1) /* new size */ - value.size() /* old size */`;
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1822318909
Thanks, I get it. The actual cause is that I define a default move assignment function, which cause a dangling pointer.
________________________________
发件人: hulk ***@***.***>
发送时间: 2023年11月22日 16:30
收件人: apache/kvrocks ***@***.***>
抄送: julic20s ***@***.***>; Mention ***@***.***>
主题: Re: [apache/kvrocks] Support for the BITFIELD command (PR #1901)
@julic20s<https://github.com/julic20s> Guess not, you should use std::optional<LockGuard> instead of LockGuard directly since the LockGuard constructor will lock the mutex, and if you use LockGuard = default, it won't lock the mutex at all.
―
Reply to this email directly, view it on GitHub<https://github.com/apache/kvrocks/pull/1901#issuecomment-1822314669>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AXIOCT53WTNNJLK4GF5RTLTYFWZ2VAVCNFSM6AAAAAA7TG4R22VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRSGMYTINRWHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "julic20s (via GitHub)" <gi...@apache.org>.
julic20s commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1401423417
##########
src/types/redis_bitmap.cc:
##########
@@ -538,6 +545,281 @@ rocksdb::Status Bitmap::BitOp(BitOpFlags op_flag, const std::string &op_name, co
return storage_->Write(storage_->DefaultWriteOptions(), batch->GetWriteBatch());
}
+namespace {
+using SegmentCache = std::pair<std::string, std::string>;
+
+// used to simplify accessing bitfield that occupies multiple continuous segments.
+// you can access multiple segments like accessing in one big segments.
+class BitmapSegmentsView {
+ public:
+ BitmapSegmentsView() = default;
+ explicit BitmapSegmentsView(std::vector<SegmentCache *> segments, bool expand_in_advance = true)
+ : segments_(std::move(segments)) {
+ if (expand_in_advance && segments_.size() > 1) {
+ // expand segments during iteration (not in advance) maybe increase allocation times.
+ // we assume that all end of each segment except last one will be accessed.
+ // note: segments_.size() is unsigned.
+ for (size_t i = 0; i < segments_.size() - 1; ++i) {
+ ExpandBitmapSegment(&segments_[i]->second, kBitmapSegmentBytes);
+ }
+ }
+ }
+
+ char &operator[](uint32_t index) noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ const char &operator[](uint32_t index) const noexcept {
+ std::string &content = segments_[index / kBitmapSegmentBytes]->second;
+ index %= kBitmapSegmentBytes;
+ ExpandBitmapSegment(&content, index + 1);
+ return content[index];
+ }
+
+ SegmentCache &GetSegment(uint32_t index) noexcept { return *segments_[index]; }
+
+ const SegmentCache &GetSegment(uint32_t index) const noexcept { return *segments_[index]; }
+
+ size_t LastSegmentSize() const noexcept { return segments_.back()->second.size(); }
+
+ size_t SegmentsCount() const noexcept { return segments_.size(); }
+
+ auto SegmentBegin() noexcept { return segments_.begin(); }
+
+ auto SegmentEnd() noexcept { return segments_.end(); }
+
+ private:
+ std::vector<SegmentCache *> segments_;
+};
+} // namespace
+
+template <bool ReadOnly>
+rocksdb::Status Bitmap::bitfield(const Slice &user_key, const std::vector<BitfieldOperation> &ops,
+ std::vector<std::string> *rets) {
+ std::string ns_key = AppendNamespacePrefix(user_key);
+
+ LockGuard guard;
+ if constexpr (!ReadOnly) {
+ guard = LockGuard(storage_->GetLockManager(), ns_key);
+ }
+
+ BitmapMetadata metadata;
+ std::string raw_value;
+ auto s = GetMetadata(ns_key, &metadata, &raw_value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+
+ if (metadata.Type() == RedisType::kRedisString) {
+ if constexpr (ReadOnly) {
+ s = BitmapString::BitfieldReadOnly(ns_key, raw_value, ops, rets);
+ } else {
+ s = BitmapString(storage_, namespace_).Bitfield(ns_key, &raw_value, ops, rets);
+ }
+ return s;
+ }
+
+ if (metadata.Type() != RedisType::kRedisBitmap) {
+ return rocksdb::Status::InvalidArgument("The value is not a bitmap or string.");
+ }
+
+ std::unordered_map<uint32_t, SegmentCache> seg_cache;
+ auto get_segment = [&](uint32_t index, SegmentCache **res) {
+ auto [seg_itor, no_cache] = seg_cache.try_emplace(index);
+ auto &[sub_key, value] = seg_itor->second;
+ if (no_cache) {
+ auto sub_sub_key = std::to_string(index * kBitmapSegmentBytes);
+ sub_key = InternalKey(ns_key, sub_sub_key, metadata.version, storage_->IsSlotIdEncoded()).Encode();
+ rocksdb::Status s = storage_->Get(rocksdb::ReadOptions(), sub_key, &value);
+ if (!s.ok() && !s.IsNotFound()) {
+ return s;
+ }
+ }
+ *res = &seg_itor->second;
+ return rocksdb::Status::OK();
+ };
+
+ auto init_view = [&get_segment](uint32_t offset, uint32_t bits, BitmapSegmentsView *view) {
+ uint32_t first_segment = offset / kBitmapSegmentBits;
+ uint32_t last_segment = (offset + bits - 1) / kBitmapSegmentBits;
+
+ std::vector<SegmentCache *> segments(last_segment - first_segment + 1);
+ for (auto &seg : segments) {
+ auto seg_status = get_segment(first_segment++, &seg);
+ if (!seg_status.ok()) {
+ return seg_status;
+ }
+ }
+ *view = BitmapSegmentsView(std::move(segments));
+ return rocksdb::Status::OK();
+ };
+
+ // get bitfield value from entire bitmap
+ auto get = [&init_view](uint32_t offset, BitfieldEncoding enc, uint64_t *res) {
+ BitmapSegmentsView view;
+ auto view_status = init_view(offset, enc.Bits(), &view);
+ if (!view_status.ok()) {
+ return view_status;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ if (enc.IsSigned()) {
+ *res = GetSignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ } else {
+ *res = GetUnsignedBitfield(view, first_bit_segment_offset, enc.Bits());
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ if constexpr (ReadOnly) {
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ return rocksdb::Status::InvalidArgument("Write bitfield in read-only mode.");
+ }
+
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_old_value));
+ } else {
+ ret = redis::Integer(unsigned_old_value);
+ }
+
+ rets->push_back(std::move(ret));
+ }
+ return rocksdb::Status::OK();
+ }
+
+ auto batch = storage_->GetWriteBatchBase();
+ bitfieldWriteAheadLog(batch, ops);
+
+ // set bitfield value among segments
+ auto set = [&](uint32_t offset, BitfieldEncoding enc, BitfieldOverflowBehavior overflow, uint64_t value) {
+ BitmapSegmentsView view;
+ s = init_view(offset, enc.Bits(), &view);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint32_t first_bit_segment_offset = offset % kBitmapSegmentBits;
+ SetBitfield(view, first_bit_segment_offset, enc.Bits(), value);
+
+ for (auto segment = view.SegmentBegin(); segment != view.SegmentEnd(); ++segment) {
+ batch->Put((*segment)->first, (*segment)->second);
+ }
+
+ uint64_t segments_cnt = view.SegmentsCount();
+ uint64_t used_size = (offset / 8) + (segments_cnt - 1) * kBitmapSegmentBytes + view.LastSegmentSize();
+ if (metadata.size < used_size) {
+ metadata.size = used_size;
+ std::string bytes;
+ metadata.Encode(&bytes);
+ batch->Put(metadata_cf_handle_, ns_key, bytes);
+ }
+
+ return rocksdb::Status::OK();
+ };
+
+ std::string ret;
+ for (BitfieldOperation op : ops) {
+ uint64_t unsigned_old_value = 0;
+ s = get(op.offset, op.encoding, &unsigned_old_value);
+ if (!s.ok()) {
+ return s;
+ }
+
+ uint64_t unsigned_new_value = 0;
+ if (BitfieldOp(op, unsigned_old_value, &unsigned_new_value)) {
+ if (op.type != BitfieldOperation::Type::kGet) {
+ s = set(op.offset, op.encoding, op.overflow, unsigned_new_value);
+ if (!s.ok()) {
+ return s;
+ }
+ }
+
+ if (op.type == BitfieldOperation::Type::kSet) {
+ unsigned_new_value = unsigned_old_value;
+ }
+
+ if (op.encoding.IsSigned()) {
+ ret = redis::Integer(CastToSignedWithoutBitChanges(unsigned_new_value));
+ } else {
+ ret = redis::Integer(unsigned_new_value);
+ }
+ } else {
+ ret = redis::NilString();
+ }
Review Comment:
Should I use std::variant<nullptr_t, uint64_t, int64_t> or define a new type to handle this? There are 3 of type could store in result list (null, unsigned and signed). These two way above can store result in 16 bytes. But it doesn't seem intuitive enough.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1826811805
> I am trying to use rocksdb::PinnableSlice to improve Bitmap::SegmentCacheStore. But I failed because of some strange error. When I use `std::unordered_map<uint32_t, rocksdb::PinnableSlice> cache_` and get its iterator, the iterator compare function is failed to instantiate (such as: `it != cache_.end()`), which makes me can not travel `cache_` in `BatchForFlush()`. `rocksdb::Cleanable` has a bug in current rocksdb version(It's copy constructor deleted, but the param of it is not marked const).
I think using `PinnableSlice` not required.
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1402912187
##########
src/common/lock_manager.h:
##########
@@ -81,6 +81,8 @@ class LockManager {
class LockGuard {
public:
+ LockGuard() = default;
Review Comment:
This ctor will make the state of a LockGuard object more complicated, which is not what we want in my view.
You can refer to https://en.cppreference.com/w/cpp/thread/lock_guard/lock_guard that also doesn't provide such a ctor.
And if you do not want to initialize the object when you define it, please use `std::optional`
##########
src/common/lock_manager.h:
##########
@@ -94,8 +96,10 @@ class LockGuard {
LockGuard(LockGuard &&guard) : lock_(guard.lock_) { guard.lock_ = nullptr; }
+ LockGuard &operator=(LockGuard &&) = default;
Review Comment:
IMHO it's not a well-formed move assignment operator, which can lead to double unlock.
##########
src/commands/command_parser.h:
##########
@@ -56,12 +60,39 @@ struct CommandParser {
decltype(auto) RawPeek() const { return *begin_; }
+ decltype(auto) operator[](size_t index) const { return begin_[index]; }
+
decltype(auto) RawTake() { return *begin_++; }
decltype(auto) RawNext() { ++begin_; }
bool Good() const { return begin_ != end_; }
+ std::enable_if_t<IsRandomAccessIter(), size_t> Remains() const {
+ // O(1) iff Iter is random access iterator.
+ auto d = std::distance(begin_, end_);
+ if (d < 0) {
+ d = 0;
+ }
Review Comment:
I think maybe we can just DCHECK or assert here, then we can find problem when we encounter an invalid state.
##########
src/commands/command_parser.h:
##########
@@ -56,12 +60,39 @@ struct CommandParser {
decltype(auto) RawPeek() const { return *begin_; }
+ decltype(auto) operator[](size_t index) const { return begin_[index]; }
Review Comment:
I think you can use something like std::advance here to avoid the requirement of random access iterator.
##########
src/commands/command_parser.h:
##########
@@ -41,6 +41,10 @@ struct CommandParser {
public:
using ValueType = typename Iter::value_type;
+ static constexpr bool IsRandomAccessIter() noexcept {
+ return std::is_base_of_v<std::random_access_iterator_tag, typename std::iterator_traits<Iter>::iterator_category>;
+ }
Review Comment:
```suggestion
static constexpr bool IsRandomAccessIter =
std::is_base_of_v<std::random_access_iterator_tag, typename std::iterator_traits<Iter>::iterator_category>;
```
##########
src/commands/command_parser.h:
##########
@@ -56,12 +60,39 @@ struct CommandParser {
decltype(auto) RawPeek() const { return *begin_; }
+ decltype(auto) operator[](size_t index) const { return begin_[index]; }
+
decltype(auto) RawTake() { return *begin_++; }
decltype(auto) RawNext() { ++begin_; }
bool Good() const { return begin_ != end_; }
+ std::enable_if_t<IsRandomAccessIter(), size_t> Remains() const {
+ // O(1) iff Iter is random access iterator.
+ auto d = std::distance(begin_, end_);
+ if (d < 0) {
+ d = 0;
+ }
+ return d;
+ }
+
+ size_t Eat(size_t count) {
Review Comment:
```suggestion
size_t Skip(size_t count) {
```
##########
src/common/parse_util.h:
##########
@@ -53,13 +58,18 @@ struct ParseIntFunc<long long> { // NOLINT
constexpr static const auto value = std::strtoll;
};
+template <>
+struct ParseIntFunc<unsigned char> { // NOLINT
+ constexpr static const auto value = std::strtol;
Review Comment:
```suggestion
constexpr static const auto value = std::strtoul;
```
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1821456147
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [8 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1819461262
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [32 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [3.5% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1819442218
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [32 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [3.5% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#issuecomment-1828152114
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=1901)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=1901&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=1901&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=1901&metric=new_duplicated_lines_density&view=list)
--
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
Re: [PR] Support for the BITFIELD command [kvrocks]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1901:
URL: https://github.com/apache/kvrocks/pull/1901#discussion_r1401451667
##########
src/commands/cmd_bit.cc:
##########
@@ -225,10 +227,145 @@ class CommandBitOp : public Commander {
BitOpFlags op_flag_;
};
+class CommandBitfield : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ BitfieldOperation cmd;
+
+ read_only_ = true;
+ // BITFIELD <key> [commands...]
+ for (auto group = args.begin() + 2; group != args.end();) {
Review Comment:
Oh seems `size` is already checked, you can decide whether using parser or current code.
--
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