Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix:HSET support multi feild【wip】 #2930

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from
Open

fix:HSET support multi feild【wip】 #2930

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb6897d
fix:HSET support multi feild
brother-jin Oct 24, 2024
d58c457
argv
brother-jin Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/pika_command.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameHSet, std::move(hsetptr)));
////HGetCmd
std::unique_ptr<Cmd> hgetptr =
std::make_unique<HGetCmd>(kCmdNameHGet, 3, kCmdFlagsRead | kCmdFlagsHash | kCmdFlagsUpdateCache | kCmdFlagsDoThroughDB | kCmdFlagsReadCache |kCmdFlagsFast);
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)));
Comment on lines +395 to 396
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

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:

  1. Accept exactly 2 arguments (key and field)
  2. Return a single value for a single field
  3. 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:

  1. The Redis HGET command is designed to retrieve a single field
  2. HGET and HMGET are distinct commands with different purposes
  3. 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:

  1. The actual implementation of both commands to understand their behavior
  2. Test cases or examples showing the expected usage
  3. 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:

  1. The test cases to understand the expected behavior
  2. The actual implementation details of both commands
  3. 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

////HGetallCmd
std::unique_ptr<Cmd> hgetallptr =
Expand Down
Loading