You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/11/13 17:10:49 UTC
[GitHub] [incubator-kvrocks] tanruixiang opened a new pull request, #1120: Support more HRANGE options
tanruixiang opened a new pull request, #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120
This close #1118 and #1082 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022850274
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
Yes, they can do the same thing, I'll change the command to `hrangebylex`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1069035327
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
Could you move the function to range_spec.cc, so it can be reused in zset and hash?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1041236220
##########
src/types/redis_hash.h:
##########
@@ -36,6 +36,16 @@ struct FieldValue {
enum class HashFetchType { kAll = 0, kOnlyKey = 1, kOnlyValue = 2 };
+struct HashRangeSpec {
Review Comment:
```suggestion
struct HashRangeLexSpec {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1038925951
##########
src/commands/redis_cmd.cc:
##########
@@ -1652,7 +1660,10 @@ class CommandHRange : public Commander {
}
private:
- int64_t limit_ = LONG_MAX;
+ bool reversed_ = false;
+ int64_t offset_ = 0;
+ int64_t count_ = -1;
+ HashRangeSpec spec_;
Review Comment:
You can correct me if I'm wrong.
Suggestion: you can remove `offset` and `count` since you have them as members of `HashRangeSpec`.
Additionally, to avoid casting, perhaps you can change the type of `count` from `int64_t` to `uint64_t` (of course if there is no special behavior when `count == -1` means something because the initial value of it is -1).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1038954114
##########
src/commands/redis_cmd.cc:
##########
@@ -1616,31 +1616,36 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
Review Comment:
```suggestion
class CommandHRangeByLex : public Commander {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1050345892
##########
src/commands/redis_cmd.cc:
##########
@@ -1614,34 +1614,39 @@ class CommandHGetAll : public Commander {
}
};
-class CommandHRange : public Commander {
+class CommandHRangeByLex : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return {Status::RedisParseErr, errWrongNumOfArguments};
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICase("REV")) {
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#issuecomment-1328010625
I will try to finish this pr as soon as possible before the big refactoring.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022839448
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
I can get your point, but "the smallest element in a hash" is implicitly in LSM storage and may be confusing. IMHO, supporting the BYLEX should be enough for the hash range.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1046814720
##########
src/commands/redis_cmd.cc:
##########
@@ -1614,34 +1614,39 @@ class CommandHGetAll : public Commander {
}
};
-class CommandHRange : public Commander {
+class CommandHRangeByLex : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return {Status::RedisParseErr, errWrongNumOfArguments};
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICase("REV")) {
Review Comment:
This can be aligned with zrange and makes sense, for example if you need to take the largest data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1046815176
##########
src/commands/redis_cmd.cc:
##########
@@ -1614,34 +1614,39 @@ class CommandHGetAll : public Commander {
}
};
-class CommandHRange : public Commander {
+class CommandHRangeByLex : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return {Status::RedisParseErr, errWrongNumOfArguments};
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICase("REV")) {
Review Comment:
About aligning zrange I will deal with it in the new pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1069411425
##########
src/common/range_spec.h:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+
+#include "status.h"
+struct CommonRangeLexSpec {
+ std::string min, max;
+ bool minex, maxex; /* are min or max exclusive */
+ bool max_infinite; /* are max infinite */
+ int64_t offset, count;
+ bool removed, reversed;
+ CommonRangeLexSpec()
+ : minex(false), maxex(false), max_infinite(false), offset(-1), count(-1), removed(false), reversed(false) {}
+};
+
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
Review Comment:
Same as above, we prefer a newline in the end of the file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1046851010
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
We can unite these same structure and methods of HashRangeSpec and ZRangeSpec next.
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
We can unite these same structure and methods of HashRangeSpec and ZRangeSpec in next PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1069120873
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
OK, I am very sorry for missing this function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022811632
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
For example, to take the smallest element, you can use `hrange 0 0 ` or `hrange - + limit 0 1`. You do not need to specify a limit when using index, just the desired range.
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
For example, to take the smallest element, you can use `hrange key 0 0 ` or `hrange key - + limit 0 1`. You do not need to specify a limit when using index, just the desired range.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1041846150
##########
src/types/redis_hash.h:
##########
@@ -36,6 +36,16 @@ struct FieldValue {
enum class HashFetchType { kAll = 0, kOnlyKey = 1, kOnlyValue = 2 };
+struct HashRangeSpec {
Review Comment:
Yes, do we need to merge these two into one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022850274
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
Yes, they can do the same thing. Do you think it is better if we change the command to `hrange` or `hrangebylex`.Because `zrangebylex` is no longer supported. I'm not sure if we need to align our commands to `zrangebylex`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1051744434
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
> I have no strong opinion for this. Could you merge these structures and functions in this PR? @tanruixiang
Ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1050342430
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
why not this pr, i don't think we should import different function to deal with similar things
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1045094301
##########
src/types/redis_hash.h:
##########
@@ -36,6 +36,16 @@ struct FieldValue {
enum class HashFetchType { kAll = 0, kOnlyKey = 1, kOnlyValue = 2 };
+struct HashRangeSpec {
Review Comment:
Ok, I will unify it in the next pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice merged pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice merged PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1069411287
##########
src/common/range_spce.cc:
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+#include "range_spec.h"
+
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec) {
+ if (min == "+" || max == "-") {
+ return Status(Status::NotOK, "min > max");
+ }
+
+ if (min == "-") {
+ spec->min = "";
+ } else {
+ if (min[0] == '(') {
+ spec->minex = true;
+ } else if (min[0] == '[') {
+ spec->minex = false;
+ } else {
+ return Status(Status::NotOK, "the min is illegal");
+ }
+ spec->min = min.substr(1);
+ }
+
+ if (max == "+") {
+ spec->max_infinite = true;
+ } else {
+ if (max[0] == '(') {
+ spec->maxex = true;
+ } else if (max[0] == '[') {
+ spec->maxex = false;
+ } else {
+ return Status(Status::NotOK, "the max is illegal");
+ }
+ spec->max = max.substr(1);
+ }
+ return Status::OK();
+}
Review Comment:
we prefer a newline in the end of the file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1069410633
##########
src/common/range_spce.cc:
##########
@@ -0,0 +1,53 @@
+/*
Review Comment:
there is a typo in the filename :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1025243073
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
Yes, I think it's good to keep the hrange command with BYLEX option only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1041237244
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
Is it totally same as ZSet::ParseRangeLexSpec?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1050366684
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
I have no strong opinion for this. Could you merge these structures and functions in this PR? @tanruixiang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1046851010
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
We can merge these same structure and methods of HashRangeSpec and ZRangeSpec in next PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022850274
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
Yes, they can do the same thing. Do you think it is better if we change the command to hrange or hrangebylex.Because zrangebylex is no longer supported. I'm not sure if we need to align our commands to `zrangebylex`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1041238123
##########
src/types/redis_hash.h:
##########
@@ -36,6 +36,16 @@ struct FieldValue {
enum class HashFetchType { kAll = 0, kOnlyKey = 1, kOnlyValue = 2 };
+struct HashRangeSpec {
Review Comment:
Is it totally same as ZRangeLexSpec?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1038928880
##########
src/commands/redis_cmd.cc:
##########
@@ -1652,7 +1660,10 @@ class CommandHRange : public Commander {
}
private:
- int64_t limit_ = LONG_MAX;
+ bool reversed_ = false;
+ int64_t offset_ = 0;
+ int64_t count_ = -1;
+ HashRangeSpec spec_;
Review Comment:
Yes, we need negative numbers. There are some redundant variables that can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022809473
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
Maybe there is a need for a minimum of k values or something similar?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022797119
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
BYINDEX should make no sense for the hash?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022819843
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
> @tanruixiang What do you mean by "the smallest element in a hash"?
The dictionary order in hash is minimum.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022814766
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
@tanruixiang What do you mean by "the smallest element in a hash"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1038925951
##########
src/commands/redis_cmd.cc:
##########
@@ -1652,7 +1660,10 @@ class CommandHRange : public Commander {
}
private:
- int64_t limit_ = LONG_MAX;
+ bool reversed_ = false;
+ int64_t offset_ = 0;
+ int64_t count_ = -1;
+ HashRangeSpec spec_;
Review Comment:
You can correct me if I'm wrong.
Suggestion: you can remove `offset` and `count` since you have them as members of `HashRangeSpec`.
Additionally, to avoid casting, perhaps you can change the type of `count` from `int64_t` to `uint64_t`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#issuecomment-1384794665
Thanks all. Merging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1041232818
##########
src/commands/redis_cmd.cc:
##########
@@ -1614,34 +1614,39 @@ class CommandHGetAll : public Commander {
}
};
-class CommandHRange : public Commander {
+class CommandHRangeByLex : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return {Status::RedisParseErr, errWrongNumOfArguments};
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICase("REV")) {
+ spec_.reversed = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ spec_.offset = GET_OR_RET(parser.TakeInt());
+ spec_.count = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
-
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return {Status::RedisInvalidCmd, errInvalidSyntax};
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
}
-
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return {Status::RedisParseErr, errValueNotInteger};
-
- limit_ = *parse_result;
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
}
- return Commander::Parse(args);
+ return Status::OK();
}
Status Execute(Server *svr, Connection *conn, std::string *output) override {
Redis::Hash hash_db(svr->storage_, conn->GetNamespace());
std::vector<FieldValue> field_values;
- auto s = hash_db.Range(args_[1], args_[2], args_[3], limit_, &field_values);
+ rocksdb::Status s = hash_db.RangeByLex(args_[1], spec_, &field_values);
if (!s.ok()) {
- return {Status::RedisExecErr, s.ToString()};
+ return Status(Status::RedisExecErr, s.ToString());
Review Comment:
```suggestion
return {Status::RedisExecErr, s.ToString()};
```
I prefer to stay at the original style.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1051744434
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1050342430
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
why not this pr, i don't think we should import this function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1046679610
##########
src/commands/redis_cmd.cc:
##########
@@ -1614,34 +1614,39 @@ class CommandHGetAll : public Commander {
}
};
-class CommandHRange : public Commander {
+class CommandHRangeByLex : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return {Status::RedisParseErr, errWrongNumOfArguments};
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICase("REV")) {
Review Comment:
why support `REV` option?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022850274
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
Yes, they can do the same thing. Do you think it is better if we change the command to hrange or hrangebylex.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1069410633
##########
src/common/range_spce.cc:
##########
@@ -0,0 +1,53 @@
+/*
Review Comment:
typo in filename
##########
src/common/range_spce.cc:
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+#include "range_spec.h"
+
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec) {
+ if (min == "+" || max == "-") {
+ return Status(Status::NotOK, "min > max");
+ }
+
+ if (min == "-") {
+ spec->min = "";
+ } else {
+ if (min[0] == '(') {
+ spec->minex = true;
+ } else if (min[0] == '[') {
+ spec->minex = false;
+ } else {
+ return Status(Status::NotOK, "the min is illegal");
+ }
+ spec->min = min.substr(1);
+ }
+
+ if (max == "+") {
+ spec->max_infinite = true;
+ } else {
+ if (max[0] == '(') {
+ spec->maxex = true;
+ } else if (max[0] == '[') {
+ spec->maxex = false;
+ } else {
+ return Status(Status::NotOK, "the max is illegal");
+ }
+ spec->max = max.substr(1);
+ }
+ return Status::OK();
+}
Review Comment:
We prefer a newline in the end of the file
##########
src/common/range_spec.h:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+
+#include "status.h"
+struct CommonRangeLexSpec {
+ std::string min, max;
+ bool minex, maxex; /* are min or max exclusive */
+ bool max_infinite; /* are max infinite */
+ int64_t offset, count;
+ bool removed, reversed;
+ CommonRangeLexSpec()
+ : minex(false), maxex(false), max_infinite(false), offset(-1), count(-1), removed(false), reversed(false) {}
+};
+
+Status ParseRangeLexSpec(const std::string &min, const std::string &max, CommonRangeLexSpec *spec);
Review Comment:
We prefer a newline in the end of the file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1022821898
##########
src/types/redis_hash.h:
##########
@@ -29,13 +29,23 @@
#include "storage/redis_db.h"
#include "storage/redis_metadata.h"
-typedef struct FieldValue {
+struct FieldValue {
std::string field;
std::string value;
-} FieldValue;
+};
enum class HashFetchType { kAll = 0, kOnlyKey = 1, kOnlyValue = 2 };
+struct HashSpec {
Review Comment:
I think that it should be `HashRangeSpec` or something like that to describe that this is a spec for a range command in a hash.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#issuecomment-1328010042
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1044058693
##########
src/types/redis_hash.cc:
##########
@@ -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) {
Review Comment:
Same as above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1120: `HrangebyLex` supports specify intervals
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1044058584
##########
src/types/redis_hash.h:
##########
@@ -36,6 +36,16 @@ struct FieldValue {
enum class HashFetchType { kAll = 0, kOnlyKey = 1, kOnlyValue = 2 };
+struct HashRangeSpec {
Review Comment:
I think we can do it in the next PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1120: Support more `HRANGE` options
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1120:
URL: https://github.com/apache/incubator-kvrocks/pull/1120#discussion_r1025243073
##########
src/commands/redis_cmd.cc:
##########
@@ -1508,25 +1508,60 @@ class CommandHGetAll : public Commander {
class CommandHRange : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- if (args.size() != 6 && args.size() != 4) {
- return Status(Status::RedisParseErr, errWrongNumOfArguments);
- }
- if (args.size() == 6 && Util::ToLower(args[4]) != "limit") {
- return Status(Status::RedisInvalidCmd, errInvalidSyntax);
+ CommandParser parser(args, 4);
+ while (parser.Good()) {
+ if (parser.EatEqICaseFlag("BYLEX", by_flag_)) {
+ } else if (parser.EatEqICase("REV")) {
+ reversed_ = true;
+ } else if (parser.EatEqICase("LIMIT")) {
+ offset_ = GET_OR_RET(parser.TakeInt());
+ count_ = GET_OR_RET(parser.TakeInt());
+ } else {
+ return parser.InvalidSyntax();
+ }
}
- if (args.size() == 6) {
- auto parse_result = ParseInt<int64_t>(args_[5], 10);
- if (!parse_result) return Status(Status::RedisParseErr, errValueNotInteger);
- limit_ = *parse_result;
+ if (by_flag_ == "BYLEX") {
+ spec_.reversed = reversed_;
+ spec_.count = count_;
+ spec_.offset = offset_;
+ Status s;
+ if (spec_.reversed) {
+ s = Redis::Hash::ParseRangeLexSpec(args[3], args[2], &spec_);
+ } else {
+ s = Redis::Hash::ParseRangeLexSpec(args[2], args[3], &spec_);
+ }
+ if (!s.IsOK()) {
+ return Status(Status::RedisParseErr, s.Msg());
+ }
+ } else {
+ by_flag_ = "BYINDEX";
Review Comment:
Yes, I think it's good to keep the hrange command with BYLEX only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org