Migrate ZADD #1613
Migrate ZADD #1613
Conversation
|
How do I make sure when I run ZADD it actually is using? |
|
Resolves #1604 |
vivekkothari
force-pushed
the
vivek/migrate-zadd
branch
from
558668b to
390f36c
Compare
vivekkothari
force-pushed
the
vivek/migrate-zadd
branch
from
cf62995 to
f68f76c
Compare
WalkthroughThis pull request simplifies response handling in several command implementations by replacing the direct construction of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ShardManager
participant ZADDHandler as ZADD
participant SortedSetStore as SortedSet
Client->>ShardManager: Send ZADD command with arguments
ShardManager->>ZADDHandler: Execute ZADD command
ZADDHandler->>ZADDHandler: Parse flags and validate arguments
ZADDHandler->>SortedSetStore: getOrCreateSortedSet(key)
SortedSetStore-->>ZADDHandler: Return SortedSet instance
ZADDHandler->>ZADDHandler: Process members with flags
ZADDHandler->>SortedSetStore: storeUpdatedSet(key, SortedSet)
ZADDHandler-->>ShardManager: Return response via cmdResInt
ShardManager-->>Client: Send command result
Assessment against linked issues
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
🧹 Nitpick comments (2)
internal/cmd/cmd_zadd.go (2)
64-71: Potential duplication of argument checks.
The same "argument count < 3" check appears in bothevalZADDandexecuteZADD. Consider centralizing the check in one place (e.g.,evalZADD) to adhere to DRY.func executeZADD(c *Cmd, sm *shardmanager.ShardManager) (*CmdRes, error) { - if len(c.C.Args) < 3 { - return cmdResNil, errors.ErrWrongArgumentCount("ZADD") - } shard := sm.GetShardForKey(c.C.Args[0]) return evalZADD(c, shard.Thread.Store()) }
117-132: Comprehensive validation of flag combinations.
validateFlagsAndArgscarefully checks contradictory flags. Consider adding unit tests to assert that invalid combinations (e.g., NX+XX) produce errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/cmd/cmd_del.go(2 hunks)internal/cmd/cmd_exists.go(2 hunks)internal/cmd/cmd_expiretime.go(1 hunks)internal/cmd/cmd_hset.go(1 hunks)internal/cmd/cmd_incrby.go(2 hunks)internal/cmd/cmd_ttl.go(1 hunks)internal/cmd/cmd_zadd.go(1 hunks)internal/cmd/cmds.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
internal/cmd/cmd_ttl.go (1)
internal/store/expire.go (1)
GetExpiry(29-32)
internal/cmd/cmd_zadd.go (1)
internal/cmd/cmds.go (4)
CommandMeta(72-80)CommandRegistry(94-96)Cmd(23-29)CmdRes(67-70)
🔇 Additional comments (20)
internal/cmd/cmds.go (2)
185-190: Utility function is concise and efficient.
ThecmdResIntfunction is straightforward, returning aCmdReswith an integer value. It promotes code reuse and aligns with the pattern of constructing responses without exposingwiredirectly.
192-196: Float response creation is consistent and readable.
Similarly,cmdResFloatsimplifies floating-point response construction, avoiding repetitive code. Both functions serve as a clean abstraction layer for building responses.internal/cmd/cmd_zadd.go (7)
15-36: Command metadata appears logically defined.
This block accurately documents the ZADD command’s syntax, flags, and usage with clarity. TheHelpShortandHelpLongdescriptions are detailed and user-friendly.
38-40: Command registration is properly integrated.
RegisteringcZADDininit()ensures the command is discoverable. The approach is consistent with other commands in the codebase.
42-62: Argument validation and flag handling are well-structured.
Overall,evalZADDproperly checks argument counts and retrieves/creates the sorted set. Just ensure that any additional error conditions (e.g., missing key) are fully tested.
73-83: Robust retrieval or creation of the sorted set.
getOrCreateSortedSetelegantly differentiates between existing sets and new sets. The error handling is coherent, returning a wrong type error if incompatible.
85-115: Flag parsing is intuitive and maintains clarity.
parseFlagsuses a direct approach for each recognized flag. The switch ensures unrecognized tokens terminate the search, which aligns with Redis-like commands.
195-202: Skip logic is readable and straightforward.
shouldSkipMembereffectively short-circuits insertion when flags like XX and NX conflict with existence. The function neatly isolates this logic for improved readability.
204-207: Properly ensures updated data is saved.
storeUpdatedSetuses a well-defined method to store the sorted set, preserving the new or updated data consistently.internal/cmd/cmd_expiretime.go (1)
59-59: Consistent usage of cmdResInt.
Replacing the inlinewireresponse withcmdResInt(int64(expiry / 1000))unifies response creation with other commands. This maintains consistency and improves maintainability.internal/cmd/cmd_hset.go (1)
103-103: Nice simplification of response creationThe change to use
cmdResIntsimplifies the response creation by removing direct wire package usage, making the code more maintainable and consistent with other commands.internal/cmd/cmd_del.go (2)
56-56: Streamlined response creationGood change to use
cmdResIntfor response creation, which aligns with the broader refactoring effort across command implementations.
73-73: Consistent response handlingThis change properly maintains the behavior while simplifying the code through the use of
cmdResInt.internal/cmd/cmd_incrby.go (2)
58-58: Cleaned up response handlingGood refactoring to use
cmdResInt, which improves code clarity and eliminates directwirepackage dependency.
73-73: Simplified response creationThis change correctly maintains the same behavior while making the code more maintainable.
internal/cmd/cmd_ttl.go (3)
52-52: Consistent error response handlingGood refactoring to use
cmdResInt(-2)for the non-existent key case, which aligns with the TTL command specification.
58-58: Cleaner response for no expiration caseUsing
cmdResInt(-1)correctly indicates a key with no expiration while simplifying the response creation.
63-63: Improved TTL response handlingThis change maintains the calculation of remaining time in seconds while streamlining the response creation.
internal/cmd/cmd_exists.go (2)
52-52: Improved code clarity with new helper function.The code now uses
cmdResInt(count)instead of directly creating a wire response, making the implementation more concise and readable while reducing the risk of inconsistent response formats across different commands.
74-74: Consistent response handling pattern applied.Good job maintaining consistency by applying the same pattern as in the
evalEXISTSfunction. ThecmdResInthelper function helps standardize integer response creation across all commands.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/commands/ironhawk/zadd_test.go (1)
1-109: Consider adding edge case testsThe current test suite is comprehensive but could benefit from additional edge cases:
- Adding a large number of members in a single command
- Testing with very large/small scores
- Verifying behavior with duplicate members in a single command
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cmd/cmd_zadd.go(1 hunks)tests/commands/ironhawk/main_test.go(1 hunks)tests/commands/ironhawk/zadd_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cmd/cmd_zadd.go
🧰 Additional context used
🧬 Code Definitions (1)
tests/commands/ironhawk/zadd_test.go (1)
tests/commands/ironhawk/main_test.go (1)
TestCase(24-29)
🔇 Additional comments (8)
tests/commands/ironhawk/main_test.go (1)
38-39: Good addition to support float64 testingThe added case in the
assertEqualfunction correctly handles float64 comparisons for the ZADD command responses, particularly for the INCR flag which returns floating-point scores.tests/commands/ironhawk/zadd_test.go (7)
12-33: Error handling tests are thoroughThese test cases correctly verify that the ZADD command returns appropriate error messages when called with insufficient arguments.
34-50: NX, XX, and CH flag tests are well implementedThis test case thoroughly validates the functionality of NX (only add new members), XX (only update existing members), and CH (return number of changed elements) flags, including their combinations.
51-69: GT and LT flag tests look goodThe test cases correctly verify the behavior of GT (update only if new score is greater) and LT (update only if new score is less) flags, ensuring they work as expected for both existing and new members.
70-80: INCR flag tests correctly handle floating-point resultsThis test validates that the INCR flag properly increments scores and returns floating-point results. The test matches with the float64 handling added to the assertEqual function.
81-93: Invalid flag combination tests are comprehensiveThese tests ensure that incompatible flag combinations (NX+XX, GT+LT, INCR with multiple pairs) are properly rejected with appropriate error messages.
94-106: CH flag multi-change tests are correctThis test case properly verifies that the CH flag correctly counts and returns the number of changed elements in various scenarios.
8-11:Details
✅ Verification successful
Test setup looks good
The test function properly sets up the client connection and ensures it's closed after the tests are complete.
🏁 Script executed:
#!/bin/bash # Check if getLocalConnection() is defined somewhere else rg "func getLocalConnection\(\)" --type goLength of output: 120
Confirmed: Test setup is properly implemented.
The test at
tests/commands/ironhawk/zadd_test.gocorrectly callsgetLocalConnection()(defined intests/commands/ironhawk/setup.go) to establish the client connection, and the correspondingdefer client.Close()ensures that the connection is properly closed after execution. There are no further modifications required.
vivekkothari
force-pushed
the
vivek/migrate-zadd
branch
from
b28c9ea to
12ef1c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/cmd/cmd_zadd.go (1)
152-155: Minor redundancy in the increment logic.
score = 0.0 + scoreis effectively redundant. Directly assigningscoreor skipping this line might simplify the next steps.- score = 0.0 + score + // The above line can be removed without affecting correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/cmd/cmd_del.go(2 hunks)internal/cmd/cmd_exists.go(2 hunks)internal/cmd/cmd_expiretime.go(1 hunks)internal/cmd/cmd_hset.go(1 hunks)internal/cmd/cmd_incrby.go(2 hunks)internal/cmd/cmd_ttl.go(1 hunks)internal/cmd/cmd_zadd.go(1 hunks)internal/cmd/cmds.go(1 hunks)tests/commands/ironhawk/zadd_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/cmd/cmd_incrby.go
- internal/cmd/cmd_hset.go
- internal/cmd/cmd_expiretime.go
- internal/cmd/cmd_ttl.go
- internal/cmd/cmd_del.go
🧰 Additional context used
🧬 Code Definitions (1)
internal/cmd/cmd_zadd.go (1)
internal/cmd/cmds.go (4)
CommandMeta(72-80)CommandRegistry(94-96)Cmd(23-29)CmdRes(67-70)
🔇 Additional comments (10)
internal/cmd/cmds.go (2)
185-190: Helper function for integer responses looks good.This helper function succinctly wraps an integer within a CmdRes object and is consistent with the overall inlining strategy to minimize overhead.
192-196: Helper function for floating-point responses is well-structured.Similar to
cmdResInt, returning a floating-point value inlined helps maintain consistency across numerical responses and reduces boilerplate.internal/cmd/cmd_exists.go (2)
52-52: Confirmed: Returning integer response using cmdResInt is valid.The updated approach simplifies code by removing direct wire.Response construction.
74-74: Same logic for the second occurrence is appropriate.Reusing
cmdResInt(count)ensures consistency and clarifies the return type to be an integer.tests/commands/ironhawk/zadd_test.go (3)
8-19: Good coverage for argument validations.Testing error messages for insufficient arguments ensures robust handling of edge cases.
35-49: Comprehensive flags testing (NX, XX, CH).Verifying each flag’s behavior with different sequences helps ensure correctness and prevents regression.
82-92: Solid negative tests for invalid flag combinations.Explicitly confirming that incompatible flags yield correct errors strengthens the command’s reliability.
internal/cmd/cmd_zadd.go (3)
160-161: Returning(nil, nil)under INCR with GT/LT flags.This behavior was highlighted in a past review. It silently concludes the command without a response, which may or may not be intended. Consider returning a valid response or an error for improved clarity.
42-62: Argument count checks and flags parsing are well structured.Verifying minimum arguments and evenly paired score-member inputs helps catch common mistakes early.
1-2:Details
✅ Verification successful
Request verification of usage across the codebase.
Since this ZADD implementation is new, it might be beneficial to confirm all references in the repository to ensure no old usage remains.
🏁 Script executed:
#!/bin/bash # Searching references to the 'ZADD' command usage across the entire codebase: rg -A 5 '\bZADD\b'Length of output: 69941
ZADD Command Usage Verified Across the Repository
The grep output confirms that all references to the "ZADD" command—across tests, command implementations, evaluations, and documentation—align with the new implementation. No legacy or outdated usages seem to remain.
- Tests & Examples: All test cases (e.g., in
tests0/zset_test.go,tests/commands/ironhawk/zadd_test.go, etc.) consistently use the updated "ZADD" syntax, including the valid combination of flags.- Implementation: The command implementation in
internal/cmd/cmd_zadd.goshows the expected updated usage and flag parsing.- Documentation: Updated docs in both release notes and skipped commands files confirm the new syntax and behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/cmd/cmd_zadd.go (1)
117-125: 🛠️ Refactor suggestionReturn an explicit response instead of
(nil, nil)when conditions fail under INCR.Returning
(nil, nil)silently ends the command without notifying the user. This can be confusing, especially when GT or LT conditions fail. Providing a response (like zero changed count or an explanatory error) could help clarify the outcome.Here’s an example adjustment to return a response code instead:
if (flags["GT"] && exists && score <= currentScore) || (flags["LT"] && exists && score >= currentScore) { - return nil, nil + // Return an integer response indicating no changes were made + return cmdResInt(0), nil }
🧹 Nitpick comments (2)
internal/cmd/cmd_zadd.go (2)
20-21: Remove duplicate mention of "CH" in the HelpLong options list.Currently "CH" is listed twice in line 21. This can cause confusion for users reading the docs.
Apply the following diff to remove one instance of "CH":
-Options: NX, XX, CH, INCR, GT, LT, CH +Options: NX, XX, CH, INCR, GT, LT
102-158: Consider splitting the member-processing logic for clarity.The
processMembersWithFlagsfunction performs multiple tasks—parsing scores, applying flags, incrementing, validating GT/LT checks, registering updates—all in one block. Extracting sub-steps into helper functions (e.g., one for handling INCR logic, another for CH logic) can improve readability and reduce complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/cmd/cmd_zadd.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/cmd/cmd_zadd.go (1)
internal/cmd/cmds.go (4)
CommandMeta(72-80)CommandRegistry(94-96)Cmd(23-29)CmdRes(67-70)
🔇 Additional comments (2)
internal/cmd/cmd_zadd.go (2)
160-174: Revisit "LT" and "GT" usage with "NX" / "XX" for alignment with Redis-like semantics.Although the code disallows combining LT/GT with NX or each other, consider clarifying or documenting allowed/unsupported flag combinations. For instance, "XX GT" might also be relevant in updates-only scenarios.
Would you like to verify if these flag combinations are consistent with your intended behavior and the reference implementations (e.g., in
cmd_set.goor other commands)? If so, I can generate scripts to further analyze usage to ensure correctness.
15-40: Reuse standard flag-parsing patterns for consistency with other commands.Older review comments recommended extracting and reusing the flag parsing logic (as done in
cmd_set.go, for example). Doing so promotes maintainability and aligns with the codebase’s established patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/cmd/cmd_zadd.go (1)
42-66:⚠️ Potential issueGuard against potential out-of-range errors when parsing score-member pairs.
As soon as you identify a float argument, you assume its corresponding member argument is atc.C.Args[i+1]. This can cause an index-out-of-range error if the user does not provide a next argument.for i := 1; i < len(c.C.Args); i++ { arg := strings.ToUpper(c.C.Args[i]) switch arg { case "XX", "NX", "LT", "GT", "CH", "INCR": flags[arg] = true default: + if i+1 >= len(c.C.Args) { + return cmdResNil, errors.ErrGeneral("Missing member argument for score " + arg) + } score, err := strconv.ParseFloat(arg, 64) if err != nil || math.IsNaN(score) { return cmdResNil, errors.ErrInvalidNumberFormat } params[score] = c.C.Args[i+1] i++ }
🧹 Nitpick comments (2)
internal/cmd/cmd_zadd.go (2)
15-36: Remove repeated mention of “CH” in help text.
Currently, the help text lists “CH” twice, which could confuse users. Please remove the duplicate reference for clarity.-HelpLong: ` -Adds all the specified members ... -Options: NX, XX, CH, INCR, GT, LT, CH +(remove the second CH entry)
86-93: Avoid repeating argument-count checks inexecuteZADD.
You are re-checkinglen(c.C.Args) < 3, which duplicates the logic inevalZADD. Consider unifying the argument validation to streamline maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/cmd/cmd_zadd.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/cmd/cmd_zadd.go (1)
internal/cmd/cmds.go (4)
CommandMeta(72-80)CommandRegistry(94-96)Cmd(23-29)CmdRes(67-70)
🔇 Additional comments (8)
internal/cmd/cmd_zadd.go (8)
1-13: Package and imports look good.
No immediate issues spotted with package naming or the imported libraries.
38-40: Command registration is straightforward.
Usinginit()to register the command withCommandRegistry.AddCommand(cZADD)is a clean and conventional approach.
69-77: Flag compatibility checks look correct.
The code correctly rejects incompatible combinations like NX with XX, and GT with LT. Good coverage of edge cases.
78-84: Fetching or creating the SortedSet is well-structured.
Reusing (or creating) the sorted set before processing flags and scores is clear and effective.
95-105: Creation and retrieval of the sorted set is correct.
The logic to handle existing objects vs. creating a new one is concise and consistent with the pattern in other commands.
107-158: Returning(nil, nil)can lead to no response on INCR with GT/LT checks.
This silent exit may confuse clients expecting a response. Consider returning an explicit result such ascmdResInt(0)or an error to clarify that no changes were made.-if (flags["GT"] && exists && score <= currentScore) || - (flags["LT"] && exists && score >= currentScore) { - return nil, nil +if (flags["GT"] && exists && score <= currentScore) || + (flags["LT"] && exists && score >= currentScore) { + return cmdResInt(0), nil }
160-174: Conditional logic for skipping members is consistent with flags.
The function straightforwardly handles NX, XX, GT, and LT checks.
176-179: Storing the updated set is simple and effective.
This cleanly persists changes back to the store, preserving the sorted set’s updated state.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/cmd/cmd_zadd.go (1)
118-130:⚠️ Potential issueFix INCR logic and error handling.
Two issues with the INCR logic:
- The
score = 0.0 + scoreoperation is redundant.- If GT/LT conditions fail under INCR, the function returns
(nil, nil)which silently ends the command without a proper response.if flags["INCR"] { if exists { score += currentScore - } else { - score = 0.0 + score } // Now check GT and LT conditions based on the incremented score and return accordingly if (flags["GT"] && exists && score <= currentScore) || (flags["LT"] && exists && score >= currentScore) { - return nil, nil + return cmdResFloat(0), nil } }
🧹 Nitpick comments (4)
internal/cmd/cmd_zadd.go (4)
58-58: Fix typo in comment.There's a typo in the comment: "repesneting" should be "representing".
- // This should be a float repesneting the score. If its not, its a problem. + // This should be a float representing the score. If it's not, it's a problem.
63-65: Provide a more specific error message for missing member.When a score is provided without a member, consider returning a more specific error message rather than a generic argument count error.
- if i+1 >= len(c.C.Args) { - return cmdResNil, errors.ErrWrongArgumentCount("ZADD") - } + if i+1 >= len(c.C.Args) { + return cmdResNil, errors.ErrGeneral("Missing member argument for score " + arg) + }
72-77: Consider consolidating flag compatibility checks.The flag compatibility validation is split into two if statements. Consider consolidating these checks for better readability or providing more specific error messages.
- if flags["NX"] && flags["XX"] { - return cmdResNil, errors.ErrGeneral("XX and NX options at the same time are not compatible") - } - if (flags["GT"] && flags["NX"]) || (flags["LT"] && flags["NX"]) || (flags["GT"] && flags["LT"]) { - return cmdResNil, errors.ErrGeneral("GT, LT, and/or NX options at the same time are not compatible") - } + // Check mutually exclusive flags + if flags["NX"] && flags["XX"] { + return cmdResNil, errors.ErrGeneral("XX and NX options at the same time are not compatible") + } + if flags["GT"] && flags["LT"] { + return cmdResNil, errors.ErrGeneral("GT and LT options at the same time are not compatible") + } + if flags["NX"] && (flags["GT"] || flags["LT"]) { + return cmdResNil, errors.ErrGeneral("NX cannot be used with GT or LT options") + }
132-132: Correct the comment about member skipping.The comment states that members should be skipped based on NX or XX flags, but the shouldSkipMember function also checks GT and LT flags.
- // Check if the member should be skipped based on NX or XX flags + // Check if the member should be skipped based on NX, XX, GT, or LT flags
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/cmd/cmd_zadd.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/cmd/cmd_zadd.go (1)
internal/cmd/cmds.go (4)
CommandMeta(72-80)CommandRegistry(94-96)Cmd(23-29)CmdRes(67-70)
🔇 Additional comments (3)
internal/cmd/cmd_zadd.go (3)
163-177: The shouldSkipMember function is implemented correctly.The function properly evaluates all flag combinations to determine if a member should be skipped based on NX, XX, GT, and LT flags.
179-182: The storeUpdatedSet function is well encapsulated.Good job encapsulating the storage logic into a separate function with a clear purpose.
15-36: Command definition follows standard format.The command metadata is well-defined with proper name, syntax, help text, examples, and implementation functions.
vivekkothari
force-pushed
the
vivek/migrate-zadd
branch
from
26db5f9 to
db672ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/cmd/cmd_zadd.go (2)
89-96: Remove duplicate argument validation in executeZADD.The argument count validation is already performed in evalZADD, making this check redundant.
func executeZADD(c *Cmd, sm *shardmanager.ShardManager) (*CmdRes, error) { - if len(c.C.Args) < 3 { - return cmdResNil, errors.ErrWrongArgumentCount("ZADD") - } - shard := sm.GetShardForKey(c.C.Args[0]) return evalZADD(c, shard.Thread.Store()) }
125-129: Return a more descriptive response for INCR with GT/LT condition failure.Currently, the function returns
(nil, nil)when GT/LT conditions fail with INCR, which silently ends command processing. Consider returning a more descriptive response for clarity.if (flags["GT"] && exists && score <= currentScore) || (flags["LT"] && exists && score >= currentScore) { - return nil, nil + // Return zero to indicate no change when GT/LT condition fails + return cmdResInt(0), nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/cmd/cmd_del.go(2 hunks)internal/cmd/cmd_exists.go(2 hunks)internal/cmd/cmd_expiretime.go(1 hunks)internal/cmd/cmd_hset.go(1 hunks)internal/cmd/cmd_incrby.go(2 hunks)internal/cmd/cmd_ttl.go(1 hunks)internal/cmd/cmd_zadd.go(1 hunks)internal/cmd/cmds.go(1 hunks)tests/commands/ironhawk/zadd_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/commands/ironhawk/zadd_test.go
- internal/cmd/cmd_incrby.go
- internal/cmd/cmd_hset.go
- internal/cmd/cmd_ttl.go
- internal/cmd/cmd_expiretime.go
- internal/cmd/cmd_del.go
🧰 Additional context used
🧬 Code Definitions (1)
internal/cmd/cmd_zadd.go (1)
internal/cmd/cmds.go (4)
CommandMeta(72-80)CommandRegistry(94-96)Cmd(23-29)CmdRes(67-70)
🔇 Additional comments (10)
internal/cmd/cmds.go (2)
185-190: Good addition of the cmdResInt utility function!This utility function creates a
CmdReswith an integer value, simplifying response creation across commands. The inline comment correctly notes that Go's compiler will likely inline this small function, avoiding runtime overhead.
192-196: Good addition of the cmdResFloat utility function!This utility function for float responses complements the integer version, providing a consistent way to generate float responses. Both functions follow the established pattern of reusable response objects in this file.
internal/cmd/cmd_exists.go (2)
52-52: Good usage of the new cmdResInt utility function!Replacing the direct construction of
CmdReswith the utility function improves readability while maintaining the same functionality.
74-74: Good usage of the new cmdResInt utility function!Consistently using the utility function here as well improves code maintainability.
internal/cmd/cmd_zadd.go (6)
15-36: Well-structured ZADD command declaration with comprehensive documentation.The command metadata includes clear syntax, helpful examples, and detailed descriptions of all supported options.
63-65: Good boundary check for score-member pairs.This validation ensures that every score has a corresponding member argument, preventing index out-of-range errors.
72-80: Good validation of incompatible flag combinations.The implementation correctly handles incompatible flag combinations, following Redis semantics:
- NX and XX are mutually exclusive
- GT, LT, and NX combinations are incompatible
- INCR requires exactly one score-member pair
98-108: Well-implemented function to get or create sorted sets.The function properly handles type checking of existing objects and creation of new sets when needed.
164-177: Clean separation of member skipping logic.Extracting this logic into a separate function improves readability and maintainability.
111-161:Details
✅ Verification successful
Verify the command against Redis specifications.
The implementation generally follows the Redis ZADD semantics, but please verify the following:
- INCR with no existing member (line 122): Redis adds the increment as the initial score
- Return value handling for CH (line 156): Verify it counts all changes correctly
- Response with INCR flag (line 148): Verify it correctly returns the new score as a float
🌐 Web query:
How does Redis ZADD command handle INCR flag with non-existing members?💡 Result:
The Redis
ZADDcommand with theINCRflag increments a member's score by the specified value, even if the member doesn't exist. Here's how it handles non-existing members:
Creates the member with an initial score of 0, then applies the increment:
If the specified member isn't in the sorted set, Redis treats its initial score as0and adds the increment value to it. The new member is inserted with a final score equal to the increment[4][13].Returns the new score instead of the usual integer reply:
WithINCR,ZADDbehaves likeZINCRBY, returning the updated score as a floating-point value (e.g.,"2.5")[1][4]. For non-existing members, this equals the increment value provided in the command.Example:
ZADD leaderboard INCR 100 "new_player"If "new_player" doesn't exist, they'll be added with a score of
100(0 + 100), and the command returns"100".This behavior makes
ZADD INCRatomic for score updates, eliminating the need for separate existence checks. However:
- Conflicting flags like
NX(prevent new members) will cause failure if used withINCR[1][4].- If the key doesn't exist, it's automatically created[4].
The operation has a time complexity of O(log(N)) per member due to the sorted set's underlying structure[7][13].
Citations:
- 1: http://www.eloqdata.com/eloqkv/zset/ZADD
- 2: https://valkey.io/commands/zadd/
- 3: https://redis.pjam.me/post/chapter-10-adding-sorted-set-commands/
- 4: https://www.dragonflydb.io/docs/command-reference/sorted-sets/zadd
- 5: https://redis.io/docs/latest/commands/zrem/
- 6: https://redis-py.readthedocs.io/en/v4.1.3/commands.html
- 7: https://redis.io/docs/latest/commands/zadd/
- 8: https://upstash.com/docs/redis/sdks/ts/commands/zset/zadd
- 9: https://docs.rs/redis/latest/redis/trait.Commands.html
- 10: redis/redis-py#1084
- 11: https://redis-py.readthedocs.io/en/stable/commands.html
- 12: https://redis.io/docs/latest/commands/incr/
- 13: https://www.digitalocean.com/community/cheatsheets/how-to-manage-sorted-sets-in-redis
- 14: https://redis.io/docs/latest/commands/
- 15: https://redis.github.net.cn/docs/latest/commands/zadd/
- 16: alicebob/miniredis#30
Verification complete: Code behavior aligns with Redis ZADD specifications.
- INCR Flag: When the member does not exist, the code correctly initializes its score as 0 and then adds the increment, mirroring Redis behavior.
- CH Flag: The implementation returns the sum of added and updated changes, which is consistent with Redis semantics.
- INCR Response: The function returns the updated score as a float, as expected.
3 participants
Resolved #1604
Migrate ZADD
Also create
cmdResIntfunction which creates an int response.Verified that go-compiler inlines the function, so it should cause no runtime overhead.
Testing evidence
Summary by CodeRabbit