SCAN: New Feature SCAN cursor [TYPE type] modifier suggested in issue #6107

[AngusP]

Extends the SCAN (but not ZSCAN, HSCAN etc.) API to

SCAN cursor [MATCH pattern] [COUNT count] [TYPE type]

This allows scanning the DB for keys of a single type, which may be useful for finding streams, garbage collection, migrations etc.

The feature was suggested by @gkorland in #6107.

The argument type is the string you get when asking Redis for the TYPE of a key, so works with modules as well as the native types. 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. Similarly, Hyperloglogs, bitmaps and bit fields are backed by strings so are also not distinguishable.

Additionally, for sparse types that are rare in the DB, this has the same behaviour as MATCH in that it may return many empty results before giving something, even for large COUNTs.

Also adds tests to check basic functionality of this optional TYPE 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/messy with C string, so I haven't addressed it.

The redis-doc repo will need to be updated with this new arg if accepted.

Many thanks as always 👍

[itamarhaber]

Looks good, although I'm not sure about the two "new" types introduced: 'none' and 'unknown'.

[AngusP]

@itamarhaber Yeah, there's a little difficulty in working out how to handle those errors here - These values are possible but not actually types so we maybe ought to do and return nothing...

"none" can be returned if the key is missed (lookupKeyReadWithFlags returned NULL) so a test case to trigger this would be useful. As far as I can see, "unknown" shouldn't be possible to trigger unless Redis is rather confused - after all, these two non-types can be returned from TYPE key too but I've never seen this happen in my experience...

[AngusP]

@antirez any opinion on whether this would be a useful feature to have?

[antirez]

Hello @AngusP, this feature looks good to me, thanks a lot for proposing. Please could you operate the following trivial changes?

1. In code like char* typeNameCanonicalize(robj*), follow the rest of the Redis code by writing the asterisk near the function name (or variable name).
2. Let's rename the function in a way that is more Redis-ish :-D, for instance getObjectTypeName() would be perfect.

Thanks!

[AngusP]

@antirez will do, apologies - should’ve spotted the style!

[antirez]

@AngusP I apologize for asking about the changes actually, it's not the important part of the PR, but given that it's a trivial change, better to stay consistent :-) Thanks.

[AngusP]

@antirez see new commit for your recommended changes

[antirez]

Thanks @AngusP, merged! If you will follow up with redis-doc as well, it will be appreciated. Make sure to comment here as well once you submit the doc PR as the redis-doc repo is inspected with a different frequency. Thank you.

[antirez]

P.S. In the unit tests you wrote, I would change them to include a non-string object in the first iteration, because to test all-white all-black is generally not able to visit as many states as testing for mixed conditions.

[AngusP]

@antirez redis-doc redis/redis-doc#1137

I'm not too sure what you mean "I would change them to include a non-string object in the first iteration", do you mean to test for a database populated with a mix of strings and not-strings? I'd agree that would be a more robust test, but there's no easy debug populate equivalent I'm aware of - is there a tcl test function for this kind of thing? Or, I can just add a loop to do so

[gkorland]

@antirez can it be ported to 5.0.x too?

[antirez]

@AngusP Hey! I just mean: DEBUG POPULATE ... + ZADD foobar, so you have at least a key that is not a string. And later you check that the count is 1000 and 1 :-D Thanks for the doc.

[antirez]

@gkorland I think so.

@AngusP maybe for Redis 6 we should at least report HyperLogLog as a type if it matches the HLL format. Or, even better, we should move away from the string representation entirely. I'll think about this.