Adding ZRangeStore #2052

slorello89 · 2022-03-22T14:01:47Z

Adds ZRangeStore for #2047

APIs added:

SortedSetRangeAndStore
SortedSetRangeAndStoreAsync

Rather than funneling everything through a single method, I left the interface to be reflective of the current ZRange interface in IDatabase/IDatabaseAsync.

Added a new Message type:

SortedSetRangeAndStoreCommandMessage to handle the ZRANGESTORE as it didn't quite fit any of the other message types. I left this in RedisDatabase.cs as this follows the pattern of SortedSetCombineAndStoreCommandMessage, if you'd like to evict it to somewhere else I've no issue doing that.

API Quirks

ZRANGESTORE with the REV flag is a bit odd, all it effectively does is reverse the ordering of the start/stop arguments, basically if REV is set start must be greater than stop, I did not add any sort of smarts to effect this reversal by default as again this would not be the expected behavior of the older APIs, happy to effect the swap if you think that API would be more desirous.

NickCraver · 2022-03-22T15:59:56Z

Awesome - will try and review this tonight! Some quick pass feedback:

Let's rename SortedSetOrderBy to SortedSetOrder (to not release "By").
There's a few throw new NotImplementedException(); which should be .Inner calls
I'm not sure about the method split (need to look harder - my instinct say we collapse these but will experiment - for example SortAndStore* has a type overload). The reason those were separate methods were because they had different return types which isn't the case here. The more I look through writing this...definitely thinking 1 method (per sync/async) taking the "by" argument - thoughts?
Is there a case where the return type wouldn't be long for these? If not, that's a more friendly API (or did you spot a consistency issue there?)

I'll try and give a thorough review tonight but wanted to give feedback ASAP too. This looks like a great effort to get the API in and match the norms, hugely appreciated <3

slorello89 · 2022-03-22T16:58:37Z

Hey @NickCraver :

~~Let's rename SortedSetOrderBy to SortedSetOrder (to not release "By").~~ Done
~~There's a few throw new NotImplementedException(); which should be .Inner calls~~ - Missed those, done
I'm not sure about the method split (need to look harder - my instinct says we collapse these but will experiment - for example SortAndStore* has a type overload). The reason those were separate methods were because they had different return types which isn't the case here. The more I look through writing this...definitely thinking 1 method (per sync/async) taking the "by" argument - thoughts? : I agree with you, a single interface makes more sense, as I mentioned, the reason for the split is because the ZRANGE commands are split out between lex/rank/value, I would have guessed the reason for the separation was because the old ZRANGE commands were split between lex/rank/value (the ZRANGEBYLEX/ZRANGEBYSCORE commands have been 'deprecated' and ZRANGE with the sub-options is the new preference which now mimics the ZRANGESTORE interface. My instinct is to agree with you (they should be one unified method), but I'm typically willing to subside my judgement to convention, so I'll leave that to your preference.
~~Is there a case where the return type wouldn't be long for these? If not, that's a more friendly API (or did you spot a consistency issue there?)~~ This operates like other Store operations, only case you can expect a non-long response would be a wrong-type error if you are operating on a key that exists and is not a sorted-set key, e.g.:

127.0.0.1:6379> set foo bar
OK
127.0.0.1:6379> zrangestore bar foo 0 1
(error) WRONGTYPE Operation against a key holding the wrong kind of value

this would of course bubble up as a Redis Server exception so I don't think there's anything to worry about from a type miss-match perspective. So I agree with you, long is a better interface, so I changed that

NickCraver · 2022-03-22T17:05:12Z

@slorello89 Oh I see now (thanks for the detail!) - yes for certain that's why those were separate. I'd recommend we more closely match Redis here on these new methods by combining and look at folding those old style together in a future major release.

slorello89 · 2022-03-22T18:07:06Z

@NickCraver - sounds good, I'll fold them together, honestly will make the API much cleaner. Did I break the thing in appveyor? sounds like it's probably separate from what I did. Probably should skip all my code since it's running on windows lol.

NickCraver · 2022-03-22T18:08:45Z

Aye ignore - that's not you :)

slorello89 · 2022-03-22T18:35:40Z

@NickCraver - one final question, for erroneous combinations of arguments, should we catch the error's pre-flight?

E.g. using the inclusive/exclusive ranges for ZRANGE for ranks is not valid, same with using LIMIT/offset (skip/take).

My philosophy is so long as the errors thrown by the server clear enough I would typically leave such validations to the server, not sure where you land on this:

127.0.0.1:6379> ZRANGESTORE destKey sourceKey 0 (-1
(error) ERR value is not an integer or out of range
127.0.0.1:6379> ZRANGESTORE destKey source 0 -1 LIMIT 0 10
(error) ERR syntax error, LIMIT is only supported in combination with either BYSCORE or BYLEX

your call (I sort of don't think either of those errors will be extremely helpful to someone debugging so we're probably better off throwing argument exceptions). This isn't really necessary when they're split because you can control the inputs.

NickCraver · 2022-03-22T20:27:16Z

one final question, for erroneous combinations of arguments, should we catch the error's pre-flight?

I could go either way on this - if it's a few lines to put an ArgumentException in, I lean towards doing it on because it then doesn't couple our args/names/etc. to server names which can be confusing as things shift over time and we maintain compatibility. No strong feelings - we can always add :)

NickCraver · 2022-03-22T20:35:17Z

tests/StackExchange.Redis.Tests/SortedSets.cs

+            db.KeyDelete(new RedisKey[] {sourceKey, destinationKey}, CommandFlags.FireAndForget);
+            db.SortedSetAdd(sourceKey, lexEntries, CommandFlags.FireAndForget);
+            var exception = Assert.Throws<ArgumentException>(()=>db.SortedSetRangeAndStore(destinationKey, sourceKey,0,-1, take:5));
+            Assert.Equal($"take argument is not valid when sortedSetOrder is ByRank you may want to try setting the SortedSetOrder to ByLex or ByScore{Environment.NewLine}Parameter name: take", exception.Message);


This may be built on one OS and run on another - if new lines are in tests you'll want to use the optional param:

Assert.Equal("", "", ignoreLineEndingDifferences: true);

I figured out what it was, it's not an OS thing, it's a Framework vs core thing lol. framework's Argument

good ol' framework uses environment newline: https://github.com/microsoft/referencesource/blob/master/mscorlib/system/argumentexception.cs#L70

whereas core runtime uses a space:
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/ArgumentException.cs#L86

lol

I just said, use the param name, good enough lol.

…n framework and core.

NickCraver

Went through and tried to give a thorough review - major kudos on a plethora of tests <3 - happy to take these tweaks, hands off, or anywhere in-between - please let me know! Thanks a ton for everything so far here.

NickCraver · 2022-03-22T23:21:17Z

src/StackExchange.Redis/Enums/SortedSetOrder.cs

+    {
+        SortedSetOrder.ByLex => RedisLiterals.BYLEX,
+        SortedSetOrder.ByScore => RedisLiterals.BYSCORE,
+        _ => RedisValue.EmptyString


Recommend RedisValue.Null here, and downstream always call .GetLiteral() and check ! .IsNull on it (prop on RedisValue). This makes maintenance a bit easier. Right now these 2 if/switches have to correlate.

src/StackExchange.Redis/Interfaces/IDatabase.cs

NickCraver · 2022-03-22T23:28:30Z

src/StackExchange.Redis/Interfaces/IDatabaseAsync.cs

+        /// <param name="take">Number of elements to pull into the new set.</param>
+        /// <param name="flags">The flags to use for this operation</param>
+        /// <remarks>https://redis.io/commands/zrangestore</remarks>
+        /// <returns>The cardinality of the newly created sorted set</returns>


Copy from above please!

NickCraver · 2022-03-22T23:39:35Z

src/StackExchange.Redis/RedisDatabase.cs

+
+            if (sortedSetOrder != SortedSetOrder.ByRank)
+            {
+                arguments.Add(sortedSetOrder.GetLiteral());


Instead, let's call .GetLiteral() and check for .IsNull on our if here - extra line, but cleaner and decoupled.

Sure thing, that's how I initially had it, I figured an enum-comparison would be slightly more efficient than the null check.

src/StackExchange.Redis/RedisDatabase.cs

src/StackExchange.Redis/RedisLiterals.cs

NickCraver · 2022-03-22T23:41:06Z

src/StackExchange.Redis/RedisLiterals.cs

@@ -84,6 +86,7 @@ public static readonly RedisValue
            PING = "PING",
            PURGE = "PURGE",
            PX = "PX",
+            REV = "REV",


Alpha order on all these please :)

NickCraver · 2022-03-22T23:44:24Z

src/StackExchange.Redis/RedisDatabase.cs

+                arguments.Add(RedisLiterals.LIMIT);
+                arguments.Add(skip);
+                arguments.Add(take);
+            }


I'm happy to take a pass at this but basically we want to avoid the List and array allocations here, likely by using an existing Message type. We may need to add a 7th on CommandKeyValueValueValueValueValueMessage. If you want, I can take the efficiency part of that on and push those bits :)

I don't mind doing this, what's the correct pattern here?

I guess it would be CommandKeyKeyValueValueValueValueValueValueValueMessage, set any unused value to null and just not write the nulls out?

as there's the command, 2 keys, and at most 7 literals?

src/StackExchange.Redis/Interfaces/IDatabaseAsync.cs

Committing comment & more trivial suggestions in batch. Co-authored-by: Nick Craver <nrcraver@gmail.com>

slorello89 · 2022-03-23T03:09:59Z

@NickCraver

I went through and made the updates you requested, One thing I'd point you at is all the classes I added to Message.cs, basically tuplish extensions of the Message class so that we could fit all the different argument combinations for ZRANGESTORE, there's one that's superfluous CommandKeyKeyValueValueValueValueValueMessage since there's no combination of arguments for ZRANGESTORE that has 5 non-key arguments, but as I was filling in the ones before and after it I figured I might as well leave it (can tare it out if you prefer).

Also not 100% sure I did the unshipped->shipped conversion correctly, I think there's a script somewhere that automagically ports them for you, but it was less effort to just alphabetize copy/paste 😂 so if that's off I'd ask you to just do the port for that bit of meta-data.

NickCraver · 2022-03-24T20:34:01Z

src/StackExchange.Redis/Message.cs

+            public override int ArgCount => 8;
+        }
+
+        private sealed class CommandKeyKeyValueValueValueValueValueValueValueMessage : CommandKeyBase


It's possible we've jumped the shark...but the garbage collector will thank us.

NickCraver · 2022-03-24T20:36:08Z

src/StackExchange.Redis/RedisDatabase.cs

+                else
+                {
+                    return Message.Create(db, flags, RedisCommand.ZRANGESTORE, destinationKey, sourceKey, start, stop, RedisLiterals.REV);
+                }


TODO: let's make this a switch expression with a _ => argument exception for order. (can do tonight)

NickCraver · 2022-03-24T20:36:37Z

src/StackExchange.Redis/RedisDatabase.cs

+                    return Message.Create(db, flags, RedisCommand.ZRANGESTORE, destinationKey, sourceKey, formattedStart,
+                        formattedStop, sortedSetOrder.GetLiteral());
+                }
+            }


TODO: can so more switch expressions here to simplicity - will poke tonight!

NickCraver · 2022-03-24T20:37:38Z

@slorello89 Sorry I've been slammed the last 2 days. The changes are just fantastic, thanks a ton - I'd like to push just a few style/doc tweaks tonight then merge this in :) I should be able to get to it this evening!

NickCraver · 2022-03-25T00:51:54Z

src/StackExchange.Redis/RedisDatabase.cs

+        {
+            var msg = CreateSortedSetRangeStoreMessage(Database, flags, sourceKey, destinationKey, start, stop,
+                sortedSetOrder, order, exclude, skip, take);
+            return await ExecuteAsync(msg, ResultProcessor.Int64);


Oh note for future - no async/await needed here, saves a state machine :)

NickCraver

Looking awesome - thanks a ton for doing this and going all out on fitting in the existing codebase <3

slorello89 added 2 commits March 22, 2022 09:50

Adding ZRangeStore

0fc5991

fixing tests, both test names and skip feature flag

0c2486f

NickCraver self-assigned this Mar 22, 2022

Addressing some of Nick's comments

56ddecc

collapsing into a single SortedSetRangeAndStore(Async) method

3ccd78f

NickCraver reviewed Mar 22, 2022

View reviewed changes

slorello89 added 2 commits March 22, 2022 16:38

fixing unit test so it ignores weirdness in argument exception betwee…

4d8a132

…n framework and core.

cleaning up inadvertent newlines/spaces

2082223

NickCraver requested changes Mar 22, 2022

View reviewed changes

slorello89 commented Mar 23, 2022

View reviewed changes

src/StackExchange.Redis/Interfaces/IDatabaseAsync.cs Outdated Show resolved Hide resolved

slorello89 and others added 2 commits March 22, 2022 21:27

Apply suggestions from code review

040c515

Committing comment & more trivial suggestions in batch. Co-authored-by: Nick Craver <nrcraver@gmail.com>

addressing Nick's comments

954554b

slorello89 requested a review from NickCraver March 23, 2022 03:04

NickCraver reviewed Mar 24, 2022

View reviewed changes

NickCraver reviewed Mar 25, 2022

View reviewed changes

NickCraver added 4 commits March 24, 2022 20:52

Tidy up!

a8e75fe

Switch statements for sorted set messages

4ec270a

Cleanup

4d667fc

Add release notes!

4940ec0

NickCraver linked an issue Mar 25, 2022 that may be closed by this pull request

Support for ZRANGESTORE #2047

Closed

NickCraver approved these changes Mar 25, 2022

View reviewed changes

NickCraver merged commit af55fcb into StackExchange:main Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ZRangeStore #2052

Adding ZRangeStore #2052

slorello89 commented Mar 22, 2022 •

edited by NickCraver

Loading

NickCraver commented Mar 22, 2022

slorello89 commented Mar 22, 2022 •

edited

Loading

NickCraver commented Mar 22, 2022

slorello89 commented Mar 22, 2022

NickCraver commented Mar 22, 2022

slorello89 commented Mar 22, 2022 •

edited

Loading

NickCraver commented Mar 22, 2022

NickCraver Mar 22, 2022 •

edited

Loading

slorello89 Mar 22, 2022

slorello89 Mar 22, 2022

NickCraver left a comment

NickCraver Mar 22, 2022

NickCraver Mar 22, 2022

NickCraver Mar 22, 2022

slorello89 Mar 23, 2022

NickCraver Mar 22, 2022

NickCraver Mar 22, 2022

slorello89 Mar 23, 2022 •

edited

Loading

slorello89 Mar 23, 2022

slorello89 commented Mar 23, 2022

NickCraver Mar 24, 2022

NickCraver Mar 24, 2022

NickCraver Mar 24, 2022

NickCraver commented Mar 24, 2022

NickCraver Mar 25, 2022

NickCraver left a comment

Adding ZRangeStore #2052

Adding ZRangeStore #2052

Conversation

slorello89 commented Mar 22, 2022 • edited by NickCraver Loading

NickCraver commented Mar 22, 2022

slorello89 commented Mar 22, 2022 • edited Loading

NickCraver commented Mar 22, 2022

slorello89 commented Mar 22, 2022

NickCraver commented Mar 22, 2022

slorello89 commented Mar 22, 2022 • edited Loading

NickCraver commented Mar 22, 2022

NickCraver Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickCraver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorello89 Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorello89 commented Mar 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickCraver commented Mar 24, 2022

Choose a reason for hiding this comment

NickCraver left a comment

Choose a reason for hiding this comment

slorello89 commented Mar 22, 2022 •

edited by NickCraver

Loading

slorello89 commented Mar 22, 2022 •

edited

Loading

slorello89 commented Mar 22, 2022 •

edited

Loading

NickCraver Mar 22, 2022 •

edited

Loading

slorello89 Mar 23, 2022 •

edited

Loading