feat: support for the sort command[draft]

[PokIsemaine]

• issues: Add support of the command SORT and SORT_RO #1952

I'm trying to support the sort command and later the sort_ro command for kvrocks, this is my first time submitting a pull request for this project and I need some advice, so I set it to draft temporarily.

The help I need is as follows：

• There are three TODOs in the code and I don’t know how to deal with them. It seems that they are related to ACL, Cluster, CLIENT_SCRIPT, but I have not found relevant support for kvrocks at present. What should I do? The following is the relevant code in the redis implementation:

  • TODO1: https://github.com/redis/redis/blob/f4481e657f905074fa515701af3f695757817d88/src/sort.c#L237
  • TODO2: https://github.com/redis/redis/blob/f4481e657f905074fa515701af3f695757817d88/src/sort.c#L218
  • TODO3: https://github.com/redis/redis/blob/f4481e657f905074fa515701af3f695757817d88/src/sort.c#L296

• How to support denyoom and movablekeys flags, or whether kvrocks needs to support it?

127.0.0.1:6379> command info sort
1)  1) "sort"
    2) (integer) -2
    3) 1) write
       2) denyoom
       3) movablekeys

• Whether there is any need to modify or add to the existing code (especially exceptions), comments, and tests (especially boundary cases) in the draft?

If you can provide me with any advice I would be very grateful！

The information and code I referenced are as follows:

• [1] https://redis.io/commands/sort/
• [2] https://redis.io/commands/sort_ro/
• [3] https://github.com/redis/redis/blob/unstable/src/sort.c imitate its implementation
• [4] Execute command docs sort and command info sort to obtain the parameters, flags and other information of the sort command

[mapleFU]

It seems that they are related to ACL, Cluster, CLIENT_SCRIPT, but I have not found relevant support for kvrocks at present. What should I do?

I think we can leave them unimplemented first

How to support denyoom and movablekeys flags

About oom currently it's a bit hard to manage the memory by custom allocator. Maybe we can separate a machine/process level global memory tracking module and check that. This can separate to an other patch or discuss it firstly

[PokIsemaine]

It seems that they are related to ACL, Cluster, CLIENT_SCRIPT, but I have not found relevant support for kvrocks at present. What should I do?

I think we can leave them unimplemented first

How to support denyoom and movablekeys flags

About oom currently it's a bit hard to manage the memory by custom allocator. Maybe we can separate a machine/process level global memory tracking module and check that. This can separate to an other patch or discuss it firstly

Thanks!
So what do we need to do to modify these TODO comments? Delete them or keep them (perhaps with more formal comments)

[jihuayu]

@PokIsemaine Thanks for your PR. Your code is amazingly clean. The test cases are also very detailed.
For these TODO comments, I think we can keep them.

The logic LGTM.
But according to our conventions, this command should be placed in the cmd_key.cc file, and the core logic of the Execute function should be extracted and placed in the redis::Database class.

Below is a example:

kvrocks/src/commands/cmd_key.cc

Lines 138 to 149 in 26cbdb9

	class CommandPTTL : public Commander {
	public:
	Status Execute(Server *srv, Connection *conn, std::string *output) override {
	redis::Database redis(srv->storage, conn->GetNamespace());
	int64_t ttl = 0;
	auto s = redis.TTL(args_[1], &ttl);
	if (!s.ok()) return {Status::RedisExecErr, s.ToString()};
	*output = redis::Integer(ttl);
	return Status::OK();
	}
	};

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[PragmaTwice]

This flag is meaningless in Kvrocks. Please remove it and refer to KeyRangeVecGen.

[PokIsemaine]

Okay, I need to learn about KeyRangeVecGen.

[PragmaTwice]

I think we don't plan to implement it in this PR, so please remove it.

[PokIsemaine]

I agree with this, thanks for the heads up, I'll remove the flags later on.

[PragmaTwice]

ditto

[PragmaTwice]

The implementation of SORT should be in redis_db.cc. The parsing part can be in cmd_key.cc.

[PokIsemaine]

@PokIsemaine Thanks for your PR. Your code is amazingly clean. The test cases are also very detailed. For these TODO comments, I think we can keep them.

The logic LGTM. But according to our conventions, this command should be placed in the cmd_key.cc file, and the core logic of the Execute function should be extracted and placed in the redis::Database class.

Below is a example:

kvrocks/src/commands/cmd_key.cc

Lines 138 to 149 in 26cbdb9

	class CommandPTTL : public Commander {
	public:
	Status Execute(Server *srv, Connection *conn, std::string *output) override {
	redis::Database redis(srv->storage, conn->GetNamespace());
	int64_t ttl = 0;
	auto s = redis.TTL(args_[1], &ttl);
	if (!s.ok()) return {Status::RedisExecErr, s.ToString()};
	*output = redis::Integer(ttl);
	return Status::OK();
	}
	};

Thanks for the advice, I'll probably do it this weekend