A unified abstraction for redis command parsing

[PragmaTwice]

Search before asking

• [X] I had searched in the issues and found no similar issues.

Motivation

comments from @PragmaTwice: I feel that it might be a bit too cumbersome to operate a vector<string> for each command to parse, and there would be a lot of duplicated logic. It might be nice to design some unified abstraction, but I haven't figured out how to do that yet.

Here are some initial ideas.

To parse SET key value [ NX | XX] [GET] [ EX seconds | PX milliseconds | EXAT unix-time-seconds | PXAT unix-time-milliseconds | KEEPTTL] (ref):

#define CHECK(x) if(!x.ok()) return parse failed
#define CHECK_ASSIGN(x, v, default) if(x == default || x == v) x = v else return parse failed

iter = parsing iterator(args...);

CHECK(iter.pick_str(&key)); // insure the arg exists, eat it and dump to `key` if exists
CHECK(iter.pick_str(&value));

while(!iter.reach_end()) {
  auto pos = iter.get_pos();

  if(iter.pick_eq("NX")) { // check the current arg is NX, eat it and return true if it is, otherwise return false
    CHECK_ASSIGN(set_flag, nx, none)
  } else if(iter.pick_eq("XX")) {
    CHECK_ASSIGN(set_flag, xx, none)
  }

  if(iter.pick_eq("GET")) {
     get_flag = true;
  }
  
  if(iter.pick_eq("EX")) {
    CHECK_ASSIGN(expire_flag, ex, none);
    CHECK(iter.pick_int(&expire_value));
  } else if(iter.pick_eq("PX")) {
    CHECK_ASSIGN(expire_flag, px, none);
    CHECK(iter.pick_int(&expire_value));
  } else if(iter.pick_eq("EXAT")) {
    CHECK_ASSIGN(expire_flag, exat, none);
    CHECK(iter.pick_int(&expire_value));
  } else if(iter.pick_eq("PXAT")) {
    CHECK_ASSIGN(expire_flag, pxat, none);
    CHECK(iter.pick_int(&expire_value));
  } else if(iter.pick_eq("KEEPTTL")) {
    CHECK_ASSIGN(expire_flag, keepttl, none);
  }

  if(iter.at(pos)) return parse failed;
}

or a shorter version with string literal as enums:

#define CHECK_ASSIGN(x, v) if(x == nullptr || x == v) x = v else return parse failed

const char *iter.pick_eq_literals(const char *strs[]) {
  for(str : strs) {
    if(iter.pick_eq(str)) return str;
  }
  return nullptr;
}

iter = parsing iterator(args...);

CHECK(iter.pick_str(&key));
CHECK(iter.pick_str(&value));

while(!iter.reach_end()) {
  auto pos = iter.get_pos();

  if(new_set_flag = iter.pick_eq_literals("NX", "XX")) {
    CHECK_ASSIGN(set_flag, new_set_flag)
  }

  if(iter.pick_eq("GET")) {
     get_flag = true
  }

  if(new_expire_flag = iter.pick_eq_literals("EX", "PX", "EXAT", "PXAT", "KEEPTTL")) {
    CHECK_ASSIGN(expire_flag, new_expire_flag);
  }
  
  if(new_expire_flag && new_expire_flag != "KEEPTTL"s) {
    CHECK(iter.pick_int(&expire_value));
  }

  if(iter.at(pos)) return parse failed;
}

We can design it as an utility class, so command authors are free to use it or not.

Advantages:

• do not need to maintain the parsing state by ourselves (mostly)
• do not need to parse integer, float, or any input type by ourselves; report parse error for these types
• as efficient as previous

What do you think? I would appreciate any comments! @git-hulk @ShooterIT

comments from @git-hulk It looks better than before, this idea is really nice to me.

Originally posted by @ShooterIT in https://github.com/apache/incubator-kvrocks/issues/456#issuecomment-1132391216

Solution

No response

Are you willing to submit a PR?

• [X] I'm willing to submit a PR!