Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HrangebyLex supports specify intervals #1120

Merged
merged 26 commits into from
Jan 17, 2023

Conversation

tanruixiang
Copy link
Member

This close #1118 and #1082 .

@tanruixiang tanruixiang linked an issue Nov 13, 2022 that may be closed by this pull request
2 tasks
@tanruixiang tanruixiang changed the title Support more HRANGE options Support more HRANGE options Nov 13, 2022
src/commands/redis_cmd.cc Outdated Show resolved Hide resolved
src/types/redis_hash.h Outdated Show resolved Hide resolved
@tisonkun
Copy link
Member

Code conflict as we did some refactors. cc @torwig @PragmaTwice could you help with providing the plan on following refactors so that we don't resolve conflicts many times?

@tanruixiang
Copy link
Member Author

I will try to finish this pr as soon as possible before the big refactoring.

git-hulk
git-hulk previously approved these changes Dec 4, 2022
Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

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

Cool, LGTM

@tanruixiang tanruixiang changed the title Support more HRANGE options HrangebyLex supports specify intervals Dec 4, 2022
src/commands/redis_cmd.cc Outdated Show resolved Hide resolved
src/types/redis_hash.h Outdated Show resolved Hide resolved
src/types/redis_hash.cc Outdated Show resolved Hide resolved
Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

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

zrangebylex also has lex range parser, we should unite the two into one.

return {Status::RedisParseErr, errWrongNumOfArguments};
CommandParser parser(args, 4);
while (parser.Good()) {
if (parser.EatEqICase("REV")) {
Copy link
Member

Choose a reason for hiding this comment

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

why support REV option?

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be aligned with zrange and makes sense, for example if you need to take the largest data.

Copy link
Member Author

Choose a reason for hiding this comment

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

About aligning zrange I will deal with it in the new pr.

Copy link
Member

Choose a reason for hiding this comment

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

i think hrangebylex is enough currently since i heard there is such a demand. Developers may not be used to that hash support range operations, we can implement when someone wants it, WDYT?

PragmaTwice
PragmaTwice previously approved these changes Dec 13, 2022
@@ -355,4 +383,36 @@ rocksdb::Status Hash::Scan(const Slice &user_key, const std::string &cursor, uin
return SubKeyScanner::Scan(kRedisHash, user_key, cursor, limit, field_prefix, fields, values);
}

Status Hash::ParseRangeLexSpec(const std::string &min, const std::string &max, HashRangeSpec *spec) {
Copy link
Member

@PragmaTwice PragmaTwice Dec 13, 2022

Choose a reason for hiding this comment

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

We can merge these same structure and methods of HashRangeSpec and ZRangeSpec in next PR.

Copy link
Member

@ShooterIT ShooterIT Dec 16, 2022

Choose a reason for hiding this comment

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

why not this pr, i don't think we should import different function to deal with similar things

Copy link
Member

Choose a reason for hiding this comment

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

I have no strong opinion for this. Could you merge these structures and functions in this PR? @tanruixiang

Copy link
Member Author

@tanruixiang tanruixiang Dec 19, 2022

Choose a reason for hiding this comment

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

I have no strong opinion for this. Could you merge these structures and functions in this PR? @tanruixiang

Ok. Sorry to have missed this comment.

@@ -355,4 +383,36 @@ rocksdb::Status Hash::Scan(const Slice &user_key, const std::string &cursor, uin
return SubKeyScanner::Scan(kRedisHash, user_key, cursor, limit, field_prefix, fields, values);
}

Status Hash::ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you move the function to range_spec.cc, so it can be reused in zset and hash?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I am very sorry for missing this function.

@@ -0,0 +1,53 @@
/*
Copy link
Member

@PragmaTwice PragmaTwice Jan 13, 2023

Choose a reason for hiding this comment

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

there is a typo in the filename :)

src/common/range_spce.cc Outdated Show resolved Hide resolved
src/common/range_spec.h Outdated Show resolved Hide resolved
@PragmaTwice PragmaTwice requested review from ShooterIT and git-hulk and removed request for git-hulk January 14, 2023 03:23
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

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

Thanks for your impressive work @tanruixiang

@PragmaTwice
Copy link
Member

Thanks all. Merging...

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.

Support more HRANGE options Doubt about the HRANGE command
6 participants