From bf963253ecfd367b49081a26c1b5c410558aecfc Mon Sep 17 00:00:00 2001 From: Angus Pearson Date: Wed, 22 May 2019 16:39:04 +0100 Subject: [PATCH 1/4] Implement `SCAN cursor [TYPE type]` modifier suggested in issue #6107. Add tests to check basic functionality of this optional keyword, and also tested with a module (redisgraph). Checked quickly with valgrind, no issues. Copies name the type name canonicalisation code from `typeCommand`, perhaps this would be better factored out to prevent the two diverging and both needing to be edited to add new `OBJ_*` types, but this is a little fiddly with C strings. The [redis-doc](https://github.com/antirez/redis-doc/blob/master/commands.json) repo will need to be updated with this new arg if accepted. A quirk to be aware of here is that the GEO commands are backed by zsets not their own type, so they're not distinguishable from other zsets. Additionally, for sparse types this has the same behaviour as `MATCH` in that it may return many empty results before giving something, even for large `COUNT`s. --- src/db.c | 32 +++++++++++++++++++++++++++++++- tests/unit/scan.tcl | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/src/db.c b/src/db.c index b537a29a4..6623f7f2f 100644 --- a/src/db.c +++ b/src/db.c @@ -613,7 +613,7 @@ int parseScanCursorOrReply(client *c, robj *o, unsigned long *cursor) { } /* This command implements SCAN, HSCAN and SSCAN commands. - * If object 'o' is passed, then it must be a Hash or Set object, otherwise + * If object 'o' is passed, then it must be a Hash, Set or Zset object, otherwise * if 'o' is NULL the command will operate on the dictionary associated with * the current database. * @@ -629,6 +629,7 @@ void scanGenericCommand(client *c, robj *o, unsigned long cursor) { listNode *node, *nextnode; long count = 10; sds pat = NULL; + sds typename = NULL; int patlen = 0, use_pattern = 0; dict *ht; @@ -665,6 +666,10 @@ void scanGenericCommand(client *c, robj *o, unsigned long cursor) { use_pattern = !(pat[0] == '*' && patlen == 1); i += 2; + } else if (!strcasecmp(c->argv[i]->ptr, "type") && o == NULL && j >= 2) { + /* SCAN for a particular type only applies to the db dict */ + typename = c->argv[i+1]->ptr; + i+= 2; } else { addReply(c,shared.syntaxerr); goto cleanup; @@ -759,6 +764,31 @@ void scanGenericCommand(client *c, robj *o, unsigned long cursor) { } } + /* Filter an element if it isn't the type we want. */ + if (!filter && o == NULL && typename){ + robj* typecheck; + char *type; + typecheck = lookupKeyReadWithFlags(c->db, kobj, LOOKUP_NOTOUCH); + if (typecheck == NULL) { + type = "none"; + } else { + switch(typecheck->type) { + case OBJ_STRING: type = "string"; break; + case OBJ_LIST: type = "list"; break; + case OBJ_SET: type = "set"; break; + case OBJ_ZSET: type = "zset"; break; + case OBJ_HASH: type = "hash"; break; + case OBJ_STREAM: type = "stream"; break; + case OBJ_MODULE: { + moduleValue *mv = typecheck->ptr; + type = mv->type->name; + }; break; + default: type = "unknown"; break; + } + } + if (strcasecmp((char*) typename, type)) filter = 1; + } + /* Filter element if it is an expired key. */ if (!filter && o == NULL && expireIfNeeded(c->db, kobj)) filter = 1; diff --git a/tests/unit/scan.tcl b/tests/unit/scan.tcl index c0f4349d2..9f9ff4df2 100644 --- a/tests/unit/scan.tcl +++ b/tests/unit/scan.tcl @@ -53,6 +53,51 @@ start_server {tags {"scan"}} { assert_equal 100 [llength $keys] } + test "SCAN TYPE" { + r flushdb + # populate only creates strings + r debug populate 1000 + + # Check non-strings are excluded + set cur 0 + set keys {} + while 1 { + set res [r scan $cur type "list"] + set cur [lindex $res 0] + set k [lindex $res 1] + lappend keys {*}$k + if {$cur == 0} break + } + + assert_equal 0 [llength $keys] + + # Check strings are included + set cur 0 + set keys {} + while 1 { + set res [r scan $cur type "string"] + set cur [lindex $res 0] + set k [lindex $res 1] + lappend keys {*}$k + if {$cur == 0} break + } + + assert_equal 1000 [llength $keys] + + # Check all three args work together + set cur 0 + set keys {} + while 1 { + set res [r scan $cur type "string" match "key:*" count 10] + set cur [lindex $res 0] + set k [lindex $res 1] + lappend keys {*}$k + if {$cur == 0} break + } + + assert_equal 1000 [llength $keys] + } + foreach enc {intset hashtable} { test "SSCAN with encoding $enc" { # Create the Set From e2adea21884260dc983242cc483f4602104146e5 Mon Sep 17 00:00:00 2001 From: Angus Pearson Date: Mon, 10 Jun 2019 17:41:44 +0100 Subject: [PATCH 2/4] Add char* typeNameCanonicalize(robj*) to remove duplicate code between SCAN and TYPE commands, and to keep OBJ_* enum to string canonicalization in one place. --- src/db.c | 37 +++++++++++-------------------------- src/server.h | 6 ++++++ 2 files changed, 17 insertions(+), 26 deletions(-) diff --git a/src/db.c b/src/db.c index 6623f7f2f..6557ddc3c 100644 --- a/src/db.c +++ b/src/db.c @@ -766,26 +766,8 @@ void scanGenericCommand(client *c, robj *o, unsigned long cursor) { /* Filter an element if it isn't the type we want. */ if (!filter && o == NULL && typename){ - robj* typecheck; - char *type; - typecheck = lookupKeyReadWithFlags(c->db, kobj, LOOKUP_NOTOUCH); - if (typecheck == NULL) { - type = "none"; - } else { - switch(typecheck->type) { - case OBJ_STRING: type = "string"; break; - case OBJ_LIST: type = "list"; break; - case OBJ_SET: type = "set"; break; - case OBJ_ZSET: type = "zset"; break; - case OBJ_HASH: type = "hash"; break; - case OBJ_STREAM: type = "stream"; break; - case OBJ_MODULE: { - moduleValue *mv = typecheck->ptr; - type = mv->type->name; - }; break; - default: type = "unknown"; break; - } - } + robj* typecheck = lookupKeyReadWithFlags(c->db, kobj, LOOKUP_NOTOUCH); + char* type = typeNameCanonicalize(typecheck); if (strcasecmp((char*) typename, type)) filter = 1; } @@ -845,11 +827,8 @@ void lastsaveCommand(client *c) { addReplyLongLong(c,server.lastsave); } -void typeCommand(client *c) { - robj *o; - char *type; - - o = lookupKeyReadWithFlags(c->db,c->argv[1],LOOKUP_NOTOUCH); +char* typeNameCanonicalize(robj *o) { + char* type; if (o == NULL) { type = "none"; } else { @@ -867,7 +846,13 @@ void typeCommand(client *c) { default: type = "unknown"; break; } } - addReplyStatus(c,type); + return type; +} + +void typeCommand(client *c) { + robj *o; + o = lookupKeyReadWithFlags(c->db,c->argv[1],LOOKUP_NOTOUCH); + addReplyStatus(c, typeNameCanonicalize(o)); } void shutdownCommand(client *c) { diff --git a/src/server.h b/src/server.h index 0813f8bd1..06d0611fd 100644 --- a/src/server.h +++ b/src/server.h @@ -646,6 +646,12 @@ typedef struct redisObject { void *ptr; } robj; +/* The 'cannonical' name for a type as enumerated above is given by the + * below function. Native types are checked against the OBJ_STRING, + * OBJ_LIST, OBJ_* defines, and Module types have their registered name + * returned.*/ +char* typeNameCanonicalize(robj*); + /* Macro used to initialize a Redis object allocated on the stack. * Note that this macro is taken near the structure definition to make sure * we'll update it when the structure is changed, to avoid bugs like From 38cd5fd9f66aecc0d9a09892701cb938a48d61b1 Mon Sep 17 00:00:00 2001 From: Angus Pearson Date: Thu, 13 Jun 2019 17:49:33 +0100 Subject: [PATCH 3/4] Spelling cannonical -> canonical --- src/server.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server.h b/src/server.h index 06d0611fd..dc02edb5c 100644 --- a/src/server.h +++ b/src/server.h @@ -646,7 +646,7 @@ typedef struct redisObject { void *ptr; } robj; -/* The 'cannonical' name for a type as enumerated above is given by the +/* The 'canonical' name for a type as enumerated above is given by the * below function. Native types are checked against the OBJ_STRING, * OBJ_LIST, OBJ_* defines, and Module types have their registered name * returned.*/ From 6eb52e200ce3af68433b69e50e2a5044f7074b08 Mon Sep 17 00:00:00 2001 From: Angus Pearson Date: Mon, 8 Jul 2019 11:04:37 +0100 Subject: [PATCH 4/4] Change typeNameCanonicalize -> getObjectTypeName, and other style changes --- src/db.c | 6 +++--- src/server.h | 9 ++++----- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/db.c b/src/db.c index 6557ddc3c..bb53081f6 100644 --- a/src/db.c +++ b/src/db.c @@ -767,7 +767,7 @@ void scanGenericCommand(client *c, robj *o, unsigned long cursor) { /* Filter an element if it isn't the type we want. */ if (!filter && o == NULL && typename){ robj* typecheck = lookupKeyReadWithFlags(c->db, kobj, LOOKUP_NOTOUCH); - char* type = typeNameCanonicalize(typecheck); + char* type = getObjectTypeName(typecheck); if (strcasecmp((char*) typename, type)) filter = 1; } @@ -827,7 +827,7 @@ void lastsaveCommand(client *c) { addReplyLongLong(c,server.lastsave); } -char* typeNameCanonicalize(robj *o) { +char* getObjectTypeName(robj *o) { char* type; if (o == NULL) { type = "none"; @@ -852,7 +852,7 @@ char* typeNameCanonicalize(robj *o) { void typeCommand(client *c) { robj *o; o = lookupKeyReadWithFlags(c->db,c->argv[1],LOOKUP_NOTOUCH); - addReplyStatus(c, typeNameCanonicalize(o)); + addReplyStatus(c, getObjectTypeName(o)); } void shutdownCommand(client *c) { diff --git a/src/server.h b/src/server.h index dc02edb5c..19ef1ac59 100644 --- a/src/server.h +++ b/src/server.h @@ -646,11 +646,10 @@ typedef struct redisObject { void *ptr; } robj; -/* The 'canonical' name for a type as enumerated above is given by the - * below function. Native types are checked against the OBJ_STRING, - * OBJ_LIST, OBJ_* defines, and Module types have their registered name - * returned.*/ -char* typeNameCanonicalize(robj*); +/* The a string name for an object's type as listed above + * Native types are checked against the OBJ_STRING, OBJ_LIST, OBJ_* defines, + * and Module types have their registered name returned. */ +char *getObjectTypeName(robj*); /* Macro used to initialize a Redis object allocated on the stack. * Note that this macro is taken near the structure definition to make sure