fix:HSET support multi feild【wip】 #2930
Conversation
WalkthroughThe changes involve significant modifications to the command handling logic in Changes
Poem
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/pika_command.cc (1 hunks)
🔇 Additional comments (2)
src/pika_command.cc (2)
391-391:⚠️ Potential issueVerify the correctness of the arity value for
HSET.The arity for
HSEThas been changed to-3. Please verify that this value accurately represents the command's required number of arguments. In Redis,HSETwith multiple field-value pairs typically has an arity of-4, as it requires at least one key and one field-value pair.
395-395:⚠️ Potential issueChanging
HGETto support multiple fields may introduce compatibility issues.The
HGETcommand is now associated withHMgetCmdand its arity changed to-3, allowing it to accept multiple fields. In standard Redis behavior,HGETretrieves a single field, whileHMGETis used for multiple fields. ModifyingHGETto accept multiple fields may lead to unexpected behavior for clients relying on the traditional single-field functionality. Consider whether this change is intentional and assess its impact on existing client applications.
src/pika_command.cc
Outdated
| @@ -388,11 +388,11 @@ void InitCmdTable(CmdTable* cmd_table) { | |||
| cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameHDel, std::move(hdelptr))); | |||
| ////HSetCmd | |||
| std::unique_ptr<Cmd> hsetptr = | |||
| std::make_unique<HSetCmd>(kCmdNameHSet, 4, kCmdFlagsWrite | kCmdFlagsHash | kCmdFlagsUpdateCache | kCmdFlagsDoThroughDB | kCmdFlagsFast); | |||
| std::make_unique<HMsetCmd>(kCmdNameHSet, -3, kCmdFlagsWrite | kCmdFlagsHash | kCmdFlagsUpdateCache | kCmdFlagsDoThroughDB | kCmdFlagsFast); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming HMsetCmd to HSetCmd for clarity and consistency.
The HSET command is now associated with HMsetCmd, which may cause confusion since HMsetCmd typically corresponds to the HMSET command. To maintain clarity and align with command naming conventions, consider renaming HMsetCmd back to HSetCmd or updating the class name to reflect its association with HSET.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/pika_command.cc (1 hunks)
🔇 Additional comments (1)
src/pika_command.cc (1)
391-392: Consider the architectural implications of using HMsetCmd for HSET.While using
HMsetCmdforHSETenables multi-field support, there are some architectural considerations:
- This changes the behavior of the HSET command to handle multiple field-value pairs
- The arity change from 4 to -4 indicates variable arguments support
- Ensure this aligns with Redis command specifications and maintains backward compatibility
Let's verify the Redis command specifications and usage:
#!/bin/bash # Search for HSET command usage patterns in tests and examples rg -A 5 "HSET.*\("
| std::make_unique<HMgetCmd>(kCmdNameHGet, -3, kCmdFlagsRead | kCmdFlagsHash | kCmdFlagsUpdateCache | kCmdFlagsDoThroughDB | kCmdFlagsReadCache |kCmdFlagsFast); | ||
| cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameHGet, std::move(hgetptr))); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the test cases and command definitions, I can now provide a definitive response:
Using HMgetCmd for HGET command is incorrect and breaks Redis compatibility
The test cases in hash.tcl clearly show that HGET is designed and expected to:
- Accept exactly 2 arguments (key and field)
- Return a single value for a single field
- Have different behavior from HMGET (which is tested separately in the test file)
The change from arity=3 to arity=-3 and using HMgetCmd would break this contract and the existing test cases. The test HGET against non existing key explicitly verifies single field behavior.
Recommend reverting the changes as suggested in the original review:
- std::make_unique<HMgetCmd>(kCmdNameHGet, -3, kCmdFlagsRead | kCmdFlagsHash | kCmdFlagsUpdateCache | kCmdFlagsDoThroughDB | kCmdFlagsReadCache |kCmdFlagsFast);
+ std::make_unique<HGetCmd>(kCmdNameHGet, 3, kCmdFlagsRead | kCmdFlagsHash | kCmdFlagsUpdateCache | kCmdFlagsDoThroughDB | kCmdFlagsReadCache |kCmdFlagsFast);🔗 Analysis chain
Potential issue: HGET should not use HMgetCmd.
Using HMgetCmd for the HGET command appears incorrect:
- The Redis HGET command is designed to retrieve a single field
- HGET and HMGET are distinct commands with different purposes
- This change could break compatibility with Redis clients expecting standard HGET behavior
Recommend reverting this change and keeping HGET as a single field operation:
- std::make_unique<HMgetCmd>(kCmdNameHGet, -3, kCmdFlagsRead | kCmdFlagsHash | kCmdFlagsUpdateCache | kCmdFlagsDoThroughDB | kCmdFlagsReadCache |kCmdFlagsFast);
+ std::make_unique<HGetCmd>(kCmdNameHGet, 3, kCmdFlagsRead | kCmdFlagsHash | kCmdFlagsUpdateCache | kCmdFlagsDoThroughDB | kCmdFlagsReadCache |kCmdFlagsFast);Let's verify the Redis command specifications for HGET:
Let me gather more specific information about the HGET implementation.
The previous search didn't yield much information, so I'm looking for:
- The actual implementation of both commands to understand their behavior
- Test cases or examples showing the expected usage
- Command definitions in header files to verify the command structure
This will help us confirm if using HMgetCmd for HGET is indeed problematic or if there's a valid reason for this change.
Let me check the test files to understand the expected behavior of HGET.
I want to verify:
- The test cases to understand the expected behavior
- The actual implementation details of both commands
- The command definitions and flags to see if there's any relationship between HGET and HMGET
This will help us make a definitive conclusion about whether using HMgetCmd for HGET is correct or not.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for HGET command usage patterns
rg -A 5 "HGET.*\("
Length of output: 2648
Script:
#!/bin/bash
# Search for HGetCmd and HMgetCmd class definitions and implementations
ast-grep --pattern 'class HGetCmd {
$$$
}'
ast-grep --pattern 'class HMgetCmd {
$$$
}'
# Look for HGET command tests or examples
rg -l "HGET" include/pika_command.h src/pika_command.cc tests/
Length of output: 201
Script:
#!/bin/bash
# Check test cases for HGET command behavior
cat tests/unit/type/hash.tcl
# Look for HGET command implementation in source files
rg -A 10 "class HGetCmd|class HMgetCmd" src/
# Check command flags and definitions
rg "kCmdNameHGet|kCmdNameHMGet" include/
Length of output: 16706
chejinge
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Documentation