Skip to content

Conversation

baobaomaomeng
Copy link

GH-2628
Added DELETERANGE command to delete keys by pattern,use like scan.The command format translates to English as: DELETERANGE cursor [MATCH pattern] [COUNT count] [TYPE type].The default parameters align with the SCAN command.

@baobaomaomeng baobaomaomeng changed the title add deleterange command and test [feat] add deleterange command and test Mar 20, 2025
@baobaomaomeng baobaomaomeng changed the title [feat] add deleterange command and test feat: add deleterange command and test Mar 20, 2025
Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

Seems that this command is implemented as scan and MDel?

I'm not sure should deleterange supports prefix delete ( which could implemented with the help of rocksdb::DeleteRange ) or a scan and delete script like this? @git-hulk @PragmaTwice @ShooterIT

@git-hulk
Copy link
Member

Seems that this command is implemented as scan and MDel?

I'm not sure should deleterange supports prefix delete ( which could implemented with the help of rocksdb::DeleteRange ) or a scan and delete script like this? @git-hulk @PragmaTwice @ShooterIT

From my side, I think the prefix deletion could be a part of this command and we should use rocksdb::DeleteRange to avoid the performance issue if possible.

And for the DeleteRange command, I have one question: is it good to support the TYPE option? which might heavily affect the performance since it needs to scan and retrieve the metadata.

@baobaomaomeng
Copy link
Author

Seems that this command is implemented as scan and MDel?
I'm not sure should deleterange supports prefix delete ( which could implemented with the help of rocksdb::DeleteRange ) or a scan and delete script like this? @git-hulk @PragmaTwice @ShooterIT

From my side, I think the prefix deletion could be a part of this command and we should use rocksdb::DeleteRange to avoid the performance issue if possible.

And for the DeleteRange command, I have one question: is it good to support the TYPE option? which might heavily affect the performance since it needs to scan and retrieve the metadata.

may be we need two commands,deletepattern and deleterange,the range delete.Handling special operations for range deletion in regular expressions is overly challenging. Additionally, the 'type' is decoded from iter->value(), and I don't quite understand why it requires scanning metadata when the entire implementation seems to be just a combination of scan + mdel.

@git-hulk
Copy link
Member

Additionally, the 'type' is decoded from iter->value(), and I don't quite understand why it requires scanning metadata when the entire implementation seems to be just a combination of scan + mdel.

For example, we have 1,000,000 strings and 100 hash keys with random key ranges. And we might need to scan the entire database if we would like to delete keys by the hash type.

And yes, it makes sense to use SCAN + DEL if we could limit the maximum count on each delete. But perhaps it would be better to use SCAN + rocksdb::DeleteRange if possible.

@PragmaTwice
Copy link
Member

I don't think SCAN is necessary if we have a good semantics and syntax of this command, e.g. for DELETERANGE <prefix>, we don't need to SCAN, instead we can just call DeleteRange, right?

@git-hulk
Copy link
Member

e.g. for DELETERANGE , we don't need to SCAN, instead we can just call DeleteRange, right?

Yes, this would work if we don't support the COUNT option.

@torwig
Copy link
Contributor

torwig commented Mar 21, 2025

e.g. for DELETERANGE , we don't need to SCAN, instead we can just call DeleteRange, right?

Yes, this would work if we don't support the COUNT option.

We usually have COUNT to limit the work that the server should do (avoid long-running execution).

@git-hulk
Copy link
Member

We usually have COUNT to limit the work that the server should do (avoid long-running execution).

@torwig We could simply use rocksdb::DeleteRange if we don't need to support the COUNT. And it would be fast since it just appends a write batch in rocksdb.

@baobaomaomeng
Copy link
Author

@git-hulk @PragmaTwice @ShooterIT

Now, deleterange is a command with a clear semantic for range deletion. This command can be used in two ways:

1.deleterange begin_key end_key, which deletes key-value pairs in the range [begin_key, end_key).
2.deleterange prefix, which deletes key-value pairs with the specified prefix (the prefix must end with * to indicate it is a prefix).

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

As for typos, can we check crate-ci/typos#316 (comment)?

Copy link
Member

@mapleFU mapleFU left a comment

Choose a reason for hiding this comment

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

What about a "deleteprefix" command?

Current impl looks ok to me, however it supports two kind of cmd:

deleterange begin-key end-key
deleterange prefix

The (1) deletes [begin-key, end-key), which may need re-delete end-key. The second just delete prefix. I think using same command is ok but maybe it's not a good idea for an dangerous operation like this

@@ -111,6 +111,7 @@ class Database {
[[nodiscard]] rocksdb::Status Expire(engine::Context &ctx, const Slice &user_key, uint64_t timestamp);
[[nodiscard]] rocksdb::Status Del(engine::Context &ctx, const Slice &user_key);
[[nodiscard]] rocksdb::Status MDel(engine::Context &ctx, const std::vector<Slice> &keys, uint64_t *deleted_cnt);
[[nodiscard]] rocksdb::Status DeleteRange(engine::Context &ctx, const Slice &start, const Slice &end);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[[nodiscard]] rocksdb::Status DeleteRange(engine::Context &ctx, const Slice &start, const Slice &end);
[[nodiscard]] rocksdb::Status DeleteRange(engine::Context &ctx, Slice start, Slice end);

Comment on lines +366 to +369
if (args[1].find('*') != args[1].size() - 1) {
return {Status::RedisParseErr, errInvalidSyntax};
}
args_[1] = args[1].substr(0, args[1].size() - 1);
Copy link
Member

Choose a reason for hiding this comment

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

So this is a regex rule?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants