feat:HSET support HMSET #3003
feat:HSET support HMSET #3003
Conversation
WalkthroughThe changes enhance the hash command processing by introducing new member variables in the header file to store multiple fields and field-value pairs. The implementation in the source file has been adjusted to improve argument validation and handling for commands such as Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HSetCmd
participant Storage
Client ->> HSetCmd: Send hash set command with args
HSetCmd ->> HSetCmd: Validate argument count (min 4)
alt Single field-value pair
HSetCmd ->> Storage: Process single field-value operation
else Multiple field-value pairs
HSetCmd ->> HSetCmd: Loop through pairs and populate fields_values_
HSetCmd ->> Storage: Process multiple field-value operations
end
Storage -->> HSetCmd: Return result
HSetCmd ->> Client: Return response
sequenceDiagram
participant Client
participant HGetCmd
participant Storage
Client ->> HGetCmd: Send hash get command with args
HGetCmd ->> HGetCmd: Validate argument count (min 3)
alt Single field
HGetCmd ->> Storage: Retrieve a single field value
else Multiple fields
HGetCmd ->> HGetCmd: Populate fields_ with multiple fields
HGetCmd ->> Storage: Retrieve multiple field values (HMGet)
end
Storage -->> HGetCmd: Return result(s)
HGetCmd ->> Client: Send response
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/pika_hash.cc (5)
51-69: Improve readability by using constants and adding documentation.The argument validation logic is correct but could be more maintainable.
Consider these improvements:
+// Minimum arguments: HSET key field value +constexpr size_t HSET_MIN_ARGS = 4; + void HSetCmd::DoInitial() { - if (argv_.size() < 4) { + if (argv_.size() < HSET_MIN_ARGS) { res_.SetRes(CmdRes::kWrongNum, kCmdNameHSet); return; } key_ = argv_[1]; - if (argv_.size() == 4) { + // Single field-value pair: HSET key field value + if (argv_.size() == HSET_MIN_ARGS) { field_ = argv_[2]; value_ = argv_[3]; } - else if (argv_.size() > 4 && argv_.size() % 2 == 0) { + // Multiple field-value pairs: HSET key field1 value1 field2 value2 ... + else if (argv_.size() > HSET_MIN_ARGS && argv_.size() % 2 == 0) { for (size_t i = 2; i < argv_.size(); i += 2) { fields_values_.emplace_back(argv_[i], argv_[i + 1]); } } else { res_.SetRes(CmdRes::kWrongNum, kCmdNameHSet); } }
74-92: Translate comment to English and improve error messages.The implementation correctly handles both single and multiple field-value pairs but needs some improvements.
Consider these changes:
if (argv_.size() == 4) { - // 处理传统 HSET，设置单个字段-值对 + // Handle traditional HSET with single field-value pair int32_t count = 0; s_ = db_->storage()->HSet(key_, field_, value_, &count); if (s_.ok()) { res_.AppendContent(":" + std::to_string(count)); AddSlotKey("h", key_, db_); } else { - res_.SetRes(CmdRes::kErrOther, s_.ToString()); + res_.SetRes(CmdRes::kErrOther, "Failed to set single field-value pair: " + s_.ToString()); } } else if (argv_.size() > 4 && argv_.size() % 2 == 0) { s_ = db_->storage()->HMSet(key_, fields_values_); if (s_.ok()) { res_.AppendContent(":" + std::to_string(fields_values_.size())); AddSlotKey("h", key_, db_); } else { - res_.SetRes(CmdRes::kErrOther, s_.ToString()); + res_.SetRes(CmdRes::kErrOther, "Failed to set multiple field-value pairs: " + s_.ToString()); } }
118-132: Improve readability by using constants and adding documentation.The argument validation logic is correct but could be more maintainable.
Consider these improvements:
+// Minimum arguments: HGET key field +constexpr size_t HGET_MIN_ARGS = 3; + void HGetCmd::DoInitial() { - if (argv_.size() < 3) { + if (argv_.size() < HGET_MIN_ARGS) { res_.SetRes(CmdRes::kWrongNum, kCmdNameHGet); return; } key_ = argv_[1]; - if (argv_.size() == 3) { + // Single field: HGET key field + if (argv_.size() == HGET_MIN_ARGS) { field_ = argv_[2]; } - else if (argv_.size() > 3) { + // Multiple fields: HGET key field1 field2 ... + else if (argv_.size() > HGET_MIN_ARGS) { for (size_t i = 2; i < argv_.size(); ++i) { fields_.push_back(argv_[i]); } } }
137-167: Improve error messages and optimize single field case.The implementation correctly handles both single and multiple fields but could be improved.
Consider these improvements:
if (argv_.size() == 3) { - std::string value; - s_ = db_->storage()->HGet(key_, field_, &value); + // Reuse fields_ vector to avoid code duplication + fields_.clear(); + fields_.push_back(field_); + std::vector<storage::ValueStatus> values; + s_ = db_->storage()->HMGet(key_, fields_, &values); if (s_.ok()) { - res_.AppendStringLenUint64(value.size()); - res_.AppendContent(value); + res_.AppendStringLenUint64(values[0].value.size()); + res_.AppendContent(values[0].value); } else if (s_.IsNotFound()) { res_.AppendContent("$-1"); } else { - res_.SetRes(CmdRes::kErrOther, s_.ToString()); + res_.SetRes(CmdRes::kErrOther, "Failed to get field value: " + s_.ToString()); } } else if (argv_.size() > 3) { std::vector<storage::ValueStatus> values; s_ = db_->storage()->HMGet(key_, fields_, &values); if (s_.ok()) { res_.AppendArrayLen(values.size()); for (const auto& vs : values) { if (vs.status.ok()) { res_.AppendStringLenUint64(vs.value.size()); res_.AppendContent(vs.value); } else { res_.AppendContent("$-1"); } } } else { - res_.SetRes(CmdRes::kErrOther, s_.ToString()); + res_.SetRes(CmdRes::kErrOther, "Failed to get multiple field values: " + s_.ToString()); } }
172-202: Improve error messages and optimize single field case in cache handling.The cache implementation correctly handles both single and multiple fields but could be improved.
Consider these improvements:
if (argv_.size() == 3) { - std::string value; - auto s = db_->cache()->HGet(key_, field_, &value); + // Reuse fields_ vector to avoid code duplication + fields_.clear(); + fields_.push_back(field_); + std::vector<storage::ValueStatus> values; + auto s = db_->cache()->HMGet(key_, fields_, &values); if (s.ok()) { - res_.AppendStringLen(value.size()); - res_.AppendContent(value); + res_.AppendStringLen(values[0].value.size()); + res_.AppendContent(values[0].value); } else if (s.IsNotFound()) { res_.SetRes(CmdRes::kCacheMiss); } else { - res_.SetRes(CmdRes::kErrOther, s.ToString()); + res_.SetRes(CmdRes::kErrOther, "Failed to get field value from cache: " + s.ToString()); } } else if (argv_.size() > 3) { std::vector<storage::ValueStatus> values; auto s = db_->cache()->HMGet(key_, fields_, &values); if (s.ok()) { res_.AppendArrayLen(values.size()); for (const auto& vs : values) { if (vs.status.ok()) { res_.AppendStringLen(vs.value.size()); res_.AppendContent(vs.value); } else { res_.AppendContent("$-1"); } } } else if (s.IsNotFound()) { res_.SetRes(CmdRes::kCacheMiss); } else { - res_.SetRes(CmdRes::kErrOther, s.ToString()); + res_.SetRes(CmdRes::kErrOther, "Failed to get multiple field values from cache: " + s.ToString()); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/pika_hash.h(2 hunks)src/pika_hash.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build_on_macos
- GitHub Check: Analyze (go)
- GitHub Check: build_on_centos
🔇 Additional comments (2)
include/pika_hash.h (2)
62-62: LGTM! Good addition for supporting multiple field operations.The new
fields_vector member enables handling multiple fields while maintaining backward compatibility with single field operations.
108-108: LGTM! Well-structured addition for supporting multiple field-value pairs.The new
fields_values_vector member usingstorage::FieldValuestruct provides a clean way to handle multiple field-value pairs while maintaining backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pika.yml(2 hunks)tests/integration/hash_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pika.yml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
- GitHub Check: build_on_centos
🔇 Additional comments (1)
tests/integration/hash_test.go (1)
277-310: LGTM! Comprehensive struct testing.The test thoroughly validates hash operations with structs, covering:
- Various field types (string, int16, time.Duration, interface{})
- Redis tag options (omitempty, ignored fields)
- Nil value handling
- Multiple hash keys
| It("should HGet2", func() { | ||
| err := client.HSet(ctx, "hash", "key1", "hello1").Err() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| vals, err := client.HMGet(ctx, "hash", "key1").Result() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(vals).To(Equal([]interface{}{"hello1"})) | ||
| }) |
There was a problem hiding this comment.
Remove duplicate test case.
This test case is identical to the existing "should HMGet" test (lines 209-216) and doesn't add any new test coverage.
Remove this redundant test case to maintain a clean and maintainable test suite.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/pika_hash.cc (3)
75-77: Inconsistent response format between single and multiple field operations.The response format differs between single field HSet (
:count) and multiple field HMSet (:fields_values_.size()). Consider standardizing the response format for better client compatibility.- res_.AppendContent(":" + std::to_string(count)); + res_.AppendContent(":" + std::to_string(1)); AddSlotKey("h", key_, db_); } else {Also applies to: 84-85
104-109: Cache update could be more efficient.The cache update logic performs separate checks for single and multiple field operations. Consider consolidating the cache update logic to reduce code duplication and improve maintainability.
- if (argv_.size() == 4) { - db_->cache()->HSetIfKeyExist(key_, field_, value_); - } - else if (argv_.size() > 4 && argv_.size() % 2 == 0) { - db_->cache()->HMSet(key_, fields_values_); - } + if (argv_.size() == 4) { + fields_values_ = {{field_, value_}}; + } + db_->cache()->HMSet(key_, fields_values_);
138-145: Standardize error handling between single and multiple field operations.The error handling differs between single field HGet and multiple field HMGet operations. Consider standardizing the error handling for better maintainability.
if (s_.ok()) { - res_.AppendStringLenUint64(value.size()); - res_.AppendContent(value); + res_.AppendArrayLen(1); + if (value.empty()) { + res_.AppendContent("$-1"); + } else { + res_.AppendStringLenUint64(value.size()); + res_.AppendContent(value); + } } else if (s_.IsNotFound()) { - res_.AppendContent("$-1"); + res_.AppendArrayLen(0); } else { res_.SetRes(CmdRes::kErrOther, s_.ToString()); }Also applies to: 151-163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pika_hash.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_centos
- GitHub Check: build_on_macos
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pika_hash.cc (2)
62-68: Consider adding size limit validation.While the argument count validation is correct, consider adding a maximum limit check for the number of field-value pairs to prevent memory issues with extremely large inputs.
else if (argv_.size() > 4 && argv_.size() % 2 == 0) { + const size_t MAX_PAIRS = 1000000; // Adjust based on your requirements + if ((argv_.size() - 2) / 2 > MAX_PAIRS) { + res_.SetRes(CmdRes::kErrOther, "Too many field-value pairs"); + return; + } for (size_t i = 2; i < argv_.size(); i += 2) { fields_values_.emplace_back(argv_[i], argv_[i + 1]); }
115-129: Consider adding size limit validation for multiple fields.While the argument handling is correct, consider adding a maximum limit check for the number of fields to prevent memory issues with extremely large inputs.
else if (argv_.size() > 3) { + const size_t MAX_FIELDS = 1000000; // Adjust based on your requirements + if (argv_.size() - 2 > MAX_FIELDS) { + res_.SetRes(CmdRes::kErrOther, "Too many fields"); + return; + } for (size_t i = 2; i < argv_.size(); ++i) { fields_.push_back(argv_[i]); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pika_hash.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
- GitHub Check: Analyze (go)
- GitHub Check: build_on_ubuntu
🔇 Additional comments (2)
src/pika_hash.cc (2)
51-54: LGTM! Improved argument validation.The argument validation now correctly handles both single field-value pair and multiple field-value pairs cases.
134-164: LGTM! Well-structured response handling.The response handling is well-structured and correctly formats responses for both single and multiple field cases. The error handling is comprehensive and follows consistent patterns.
Summary by CodeRabbit