Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zrangebyscore doesn't allow +inf/-inf #30

Closed
mateuszlewko opened this issue Dec 29, 2018 · 3 comments · Fixed by #240
Closed

zrangebyscore doesn't allow +inf/-inf #30

mateuszlewko opened this issue Dec 29, 2018 · 3 comments · Fixed by #240
Labels
redis-doc Issue caused by https://github.com/antirez/redis-doc - would require changes in that repo before fix

Comments

@mateuszlewko
Copy link

Type definitions of some redis commands are not complete. For example here is type definition for
zrangebyscore(key: string, min: number, max: number, withscores: "WITHSCORES", offset_count: ["LIMIT", number, number]): Promise<any[]>;
Min and max arguments accept only number, where node_redis could also accept string or just "+inf" | "-inf".

@mmkal
Copy link
Owner

mmkal commented Jan 2, 2019

What's the use case? You should be able to use JavaScript's -Infinty and Infinty keywords. They have number type in typescript.

Using strings for number-typed arguments is exactly the sort of thing this library is supposed to protect against, unless you've found something you're unable to do. This works, and IMO is better than using "-inf"/"+inf":

it("can use zrangebyscore with infinite limits", async () => {
    const client = createHandyClient();
    await client.zadd("myzset", [1, "one"], [2, "two"], [3, "three"], [-Number.MAX_VALUE, "lowest"], [Number.MAX_VALUE, "highest"]);

    const all = await client.zrangebyscore("myzset", -Infinity, Infinity);
    expect(all).toEqual(["lowest", "one", "two", "three", "highest"]);

    const positives = await client.zrangebyscore("myzset", 0, Infinity);
    expect(positives).toEqual(["one", "two", "three", "highest"]);

    const negatives = await client.zrangebyscore("myzset", -Infinity, 0);
    expect(negatives).toEqual(["lowest"]);
});

@mateuszlewko
Copy link
Author

Function parameter that has type: number | "+inf" | "-inf", doesn't allow any string. It either has to be string literal "+inf" / "-inf" or variable of type "+inf" | "-inf". Passing variable of type string results in:

Argument of type 'string' is not assignable to parameter of type 'number | "+inf" | "-inf"'.ts(2345)

Although passing Infinity has the same result (thanks to the fact that node uses the same 64 bit floating point numbers as redis), it doesn't mean it has the same effect under the hood. I can imagine that passing "+inf" could be slightly faster, as it always means 'last element in set' and this search could be done in O(1), where as search for any number (including infinity) takes O(log n). I could be wrong about the fact that there is any noticeable difference, but having type number | "+inf" | "-inf" doesn't have any downsides, and it feels more api compliant.

Anyway, it's still probably low priority.

@mmkal
Copy link
Owner

mmkal commented Jan 9, 2019

That's a good point, if redis and javascript used different float implementations using Infinty might not work anymore. Right now the types are automatically generated from redis-doc's command.json (otherwise this project would be very hard to maintain). But the redis-doc just lists max and min as double.

So possibly this issue should be moved to that repo - since it's not actually a double. It's a double, or -inf, or +inf, or in fact open ranges like (123. That last one will be very hard to model in a non-hacky way in typescript, without resorting to string. Most double values in commands.json don't allow those special values because they're not limits. I can't off the top of my head think of a clean way to separate min/max from, say, the increment amount in incrbyfloat. Open to suggestions though.

@mmkal mmkal added the redis-doc Issue caused by https://github.com/antirez/redis-doc - would require changes in that repo before fix label Dec 24, 2019
@mmkal mmkal changed the title Type definitions are not complete zrangebyscore doesn't allow +inf/-inf Oct 12, 2020
mmkal added a commit that referenced this issue Oct 26, 2020
…240)

Fixes #30

Now that v2 has a more maintainable "patching" system, this works around redis/redis-doc#1420 - to be removed if a fix is merged there.

This effectively allows string values to be passed as min and max for ZRANGEBYSCORE, ZREMRANGEBYSCORE, ZREVRANGEBYSCORE and ZCOUNT.

Note: in future, it may be possible to use template literal types to only allow strings like (1 rather than any strings, but it's likely too complex and will require a higher typescript version than most users have.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
redis-doc Issue caused by https://github.com/antirez/redis-doc - would require changes in that repo before fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants