[Compatibility] Added COMMAND GETKEYS and GETKEYSANDFLAGS command #888
Conversation
| return true; | ||
| } | ||
|
|
||
| private void ExtractKeys(RespCommandKeySpecification spec, List<byte[]> keyList) |
There was a problem hiding this comment.
What I would probably do is have this as an extension method to SessionParseState (e.g. TryGetKeysFromKeySpecs) that takes an array of RespCommandKeySpecifications.
Then the internal implementation of extracting the keys from a single key spec could a class method inside RespCommandKeySpecification.
| { | ||
| int startIndex = 0; | ||
|
|
||
| if (spec.BeginSearch is BeginSearchIndex bsIndex) |
There was a problem hiding this comment.
You can take advantage of the OO design here and handle each class-specific scenario in the class implementation itself.
| { | ||
| // Handle negative StartFrom by converting to positive index from end | ||
| int searchStartIndex = bsKeyword.StartFrom; | ||
| if (searchStartIndex < 0) |
There was a problem hiding this comment.
Can probably avoid having 2 loops that do the same thing here, just have a condition for the index if the order is descending (this path is probably not as performance-critical).
| // Search backwards from the calculated start position for the keyword | ||
| for (int i = searchStartIndex; i >= 0; i--) | ||
| { | ||
| if (parseState.GetArgSliceByRef(i).ReadOnlySpan.EqualsUpperCaseSpanIgnoringCase(Encoding.ASCII.GetBytes(bsKeyword.Keyword))) |
There was a problem hiding this comment.
You can store Encoding.ASCII.GetBytes(bsKeyword.Keyword) prior to the loop instead of re-evaluating it each time.
| return AbortWithErrorMessage(CmdStrings.RESP_COMMAND_HAS_NO_KEY_ARGS); | ||
| } | ||
|
|
||
| var keyList = new List<byte[]>(); |
There was a problem hiding this comment.
Why do we have to allocate a byte[] for each key? The keys already exist in the parse state.
| return AbortWithWrongNumberOfArguments(nameof(RespCommand.COMMAND_GETKEYS)); | ||
| } | ||
|
|
||
| var cmdName = parseState.GetString(0).ToUpperInvariant(); |
There was a problem hiding this comment.
No need for ToUpperInvariant(), the TryGet... methods already take care of that.
| return AbortWithWrongNumberOfArguments(nameof(RespCommand.COMMAND_GETKEYSANDFLAGS)); | ||
| } | ||
|
|
||
| var cmdName = parseState.GetString(0).ToUpperInvariant(); |
There was a problem hiding this comment.
See same comments as in GETKEYS
| { | ||
| var keyCount = keyList.Count; | ||
| ExtractKeys(spec, keyList); | ||
| var flags = EnumUtils.GetEnumDescriptions(spec.Flags); |
There was a problem hiding this comment.
spec.respFormatFlags should already contain this info, you can create a public getter than returns this field.
|
|
||
| foreach (var spec in cmdInfo.KeySpecifications) | ||
| { | ||
| var keyCount = keyList.Count; |
There was a problem hiding this comment.
nit: just for clarity, should probably be named prevKeyCount
| @@ -233,7 +233,7 @@ private static IReadOnlyDictionary<string, RespCommandDocs> GetUpdatedCommandsDo | |||
| } | |||
|
|
|||
| // Update commands docs with commands to add | |||
| foreach (var command in commandsToAdd.Keys) | |||
| foreach (var command in commandsToAdd.Keys.Where(x => x.Command.StartsWith("COMMAND"))) | |||
Adding the COMMAND GETKEYS and GETKEYSANDFLAGS command to garnet