Sorted set new commands: ZDIFF, ZDIFFSTORE, ZINTER, ZINTERCARD, and ZUNION by Avital-Fine · Pull Request #2075

Sorted set new commands: ZDIFF, ZDIFFSTORE, ZINTER, ZINTERCARD, and ZUNION by Avital-Fine · Pull Request #2075 · StackExchange/StackExchange.Redis

Conversation

Covers:

ZDIFF
ZDIFFSTORE
ZINTER
ZINTERCARD
ZUNION

#2055

Avital-Fine added 2 commits

April 10, 2022 13:51

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here! Added some comments inline - as always happy to adjust and push or just keep commenting, whichever is a bigger time/sync save on your end. A few questions inline for thoughts!

NickCraver changed the title ~~Sorted set new commands~~ Sorted set new commands: ZDIFF, ZDIFFSTORE, ZINTER, ZINTERCARD, and ZUNION

Apr 10, 2022

@slorello89 I've touched this a lot, so would appreciate a third set of eyes if you get a chance!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickCraver just have some questions about the efficiency vs maintainability bits of the Message Creation.


		private Message GetSortedSetCombineCommandMessage(SetOperation operation, RedisKey[] keys, double[]? weights, Aggregate aggregate, bool withScores, CommandFlags flags)
		{
		var command = operation switch

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably want to move this logic to the SetOperation enum as an extension method.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This makes sense with an arg - done!

		}

		var i = 0;
		var values = new RedisValue[1 + keys.Length +

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much more expensive would it be to initialize a List with this capacity and using Add/AddRange as opposed to an array and then having to track i, this is a bit. . . feels like we're reinventing the wheel here? List<T>(int) is effectively just doing the same initialization, since it's initialized with the correct capacity it shouldn't over allocate nor have to dynamically reallocate.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's allocate more than double. At the very lease we're resizing without sizes, but also: look inside Message to see a .ToArray() which is going to copy as well. I agree it's not awesome to do things manually, but 2x+ allocations aren't awesome either :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I gotcha. TIL

		if (keys == null) throw new ArgumentNullException(nameof(keys));

		var i = 0;
		var values = new RedisValue[1 + keys.Length + (limit > 0 ? 2 : 0)];

Conversation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels