Add the support of the LCS command #2116
Conversation
There was a problem hiding this comment.
Thank you @JoverZhang, wonderful algorithm.
I think it's not good to put so much logic in the commander.
Would it be better to move the specific logic to redis::String? @git-hulk
|
Can someone confirm whether this logic is correct? I am also unable to determine it. |
There was a problem hiding this comment.
General LGTM. Just some few nits
@jihuayu Actually I think this impl works(Since it's code it's similiar with Redis' impl). However, I think the most important thing is prevent malicious user from using lcs to attack the system, or lcs making the server crash...
There was a problem hiding this comment.
As mentioned by @jihuayu, the implementation of the algorithm should be put into redis_string.cc instead of cmd_string.cc.
Understood, I have moved the algorithm to |
src/types/redis_string.h
Outdated
|
|
||
| struct StringLCSIdxResult { | ||
| // Matched ranges. | ||
| std::list<StringLCSMatchedRange> matches; |
There was a problem hiding this comment.
Though I don't think performance of LCS it's important, I think deque or vector ( using vector without pop_front) would be better here.
There was a problem hiding this comment.
Yeah, deque or vector is better here.
There was a problem hiding this comment.
Thank. I have modified this to use the vector.
Initially, I hadn't used the vector because I was concerned that a very large vector would significantly increase memory usage when building the output at the end.
However, I forgot that dp actually frees up more memory space at the end of string_db.LCS().
|
|
Thank you for your contribution! |
|
Hi @JoverZhang, can you add this command to our docs? |
Okay, I added it. |
Close #1955