[NEW] SetCommand may be need more compatible with redis.

[mzygQAQ]

1. [ set key val nx px ] should be reply with "syntax error", but kvrocks can works
2. [ set key val px 1000 ex 1000 ] should be reply with "syntax error", but kvrocks can works.
3. redis supports [ set key val keepttl ], but kvrocks not supports

[ShooterIT]

Yes, kvrocks doesn't check the parameters carefully,

And currently keepttl ,PXAT, EXAT and some other flags, kvrocks doesn't support.

Do you want to fix this and support these flags? @guoxiangCN

[mzygQAQ]

i 'm trying my best~

[ShooterIT]

@guoxiangCN has no spare time to do that, if someone wants to do that, go ahead

[ShooterIT]

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

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

[ShooterIT]

@PragmaTwice move your comments here, and discuss here to avoid far too many comments for #595

[PragmaTwice]

Thanks! BTW I think this idea is applicable to all commands, including SET, especially some complex ones.

[ShooterIT]

@PragmaTwice oh, it will be a big change, i think we should open a new issue to discuss

[PragmaTwice]

Thanks. Moved to #598.

[tisonkun]

Closing as partially resolved...

set key val nx px and set key val px 1000 ex 1000 have been aligned with Redis behavior now. "set key val keepttl" is a new feature that we can track in a new issue.