New ZRANGESTORE Command #7844

jonahharris · 2020-09-24T19:12:22Z

Add ZRANGESTORE command, and improve ZSTORE command to deprecated Z[REV]RANGE[BYSCORE|BYLEN].

Syntax for the new ZRANGESTORE command:
ZRANGESTORE [BYSCORE | BYLEX] [REV] [LIMIT offset count]

New syntax for ZRANGE:
ZRANGE [BYSCORE | BYLEX] [REV] [WITHSCORES] [LIMIT offset count]

Old syntax for ZRANGE:
ZRANGE [WITHSCORES]

Other ZRANGE commands remain unchanged.

The implementation uses common code for all of these, by utilizing a consumer interface that in one
command response to the client, and in the other command stores a zset key.

oranagra · 2020-09-25T17:14:10Z

@jonahharris thank you for this PR.
As i stated earlier i think the approach you took is good.
It reduces duplicate code and make it easier to maintain (decreasing chance for inconsistencies between the various similar commands and chance for bugs that are unspotted)

However, this PR is quite impossible to review (and merge), it looks like your editor re-formatted all the code, so every line is changed.
For instance compare zslFree to it's old version (indentation, line breaks etc).

The obvious problems with this are:

harder to review the changes
harder to later track the history of the code in the blame log
will create merge conflicts with any other PR or fork.

Besides, this is not the redis style, although it may not be to anyone's liking, it's better to have consistent style across the project.

It also looks like you changed the structure of the file (although i can't be sure), please avoid unnecessary changes, for an easier review.

I know it would be complicated to change it at this stage, but it would be a necessary effort in order to move forward.
I also know that even without this re-formatting many functions were refactored to share code, so the code review will be hard anyway, but the formatting changes need to be undone.

one other request, please copy most the detailed description of the change that you posted in the issue to the PR's top comment, i'll use that as the commit comment when i squash-merge this.

Thanks again for the efforts.

jonahharris · 2020-09-25T17:19:03Z

Yeah. I mentioned the formatting stuff in the issue. I'm happy to copy the changes into the normal Redis format - I just can't work with that format when I'm adding functionality as it's not very clean/organized. I'll get that updated and resubmitted shortly.

jonahharris · 2020-09-25T19:39:36Z

Merged changes and made most formatting changes. Still need to correct indentation for new code.

oranagra · 2020-09-26T07:36:31Z

@jonahharris I saw some question about function prototypes in my email but I can't find it here, maybe you deleted it.
IMO these should be at the top of the file, unless they're not needed (if the functions are ordered that way).
So for new code you can order it (put them near the top), but I'd like to avoid moving exiting code (don't generate a diff on code that didn't change), so in these cases please put the prototypes near the top.

jonahharris · 2020-09-26T12:49:53Z

I deleted it as I reordered everything to not really need them, but the one I couldn’t remove will go to the top. Now that the diff is readable, let me know what else you’d like changed and I’ll try to get to it ASAP.

oranagra

@jonahharris i reviewed all the code and added many comments.
some are about styling, and if you prefer not to deal with it, i can make a quick sweep to change it before merging.

other comments are about the implementation and bugs.

but most important one is about the command syntax, i think we had some misunderstanding, if my comment makes sense to you, please revise the code, if not, we should open it for a wider discussion..

lastly, we'll need to add some test coverage and documentation for the new commands, but we can deal with it later.

src/help.h

src/server.c

src/t_zset.c

oranagra · 2020-10-05T10:41:18Z

@jonahharris just want to confirm that the ball is at your court.
i.e. i'm waiting for you to implement the remaining suggested changes (most importantly the command syntax).
right?

jonahharris · 2020-10-06T15:05:48Z

Hey @oranagra - yes, I'm currently working on that. I only had a few consecutive days I could work on it and now I'm back to piecemeal. I'm almost done with all the changes you requested and hope to have that up in the next day or so.

jonahharris · 2020-10-16T04:06:42Z

@oranagra assuming all goes well, what would you predict the release timeline looks like? I want to make sure this doesn't fall off and, with that timeline in mind, I'll figure out how to devote time to it. I probably have only an hour or so left, but have been pretty busy. I'll try to get to it this weekend, but will rebalance the stuff on my plate to ensure it does if I need to.

oranagra · 2020-10-16T08:52:43Z

@jonahharris we aim to release 6.2 at the end of the year, or at least have release candidate.
big things better be merged ahead of time so they get to stabilize in unstable.
so you still have plenty of time, but considering you only have a few hours left, better nail it while it's fresh.
Also IIRC you had some GEORADIUS related PR you wanted to make after it which can also fit into 6.2.

oranagra · 2020-11-10T20:28:28Z

@jonahharris, friendly reminder: we have about a month before putting out a release candidate.
hope you can find the time to push this through.
thanks.

oranagra · 2020-12-02T11:58:49Z

@jonahharris we probably missed the mark of 6.2 RC1, but maybe these changes can still make it to RC2.
do you wanna try to pick this up, or maybe someone else can take over?

jonahharris · 2020-12-02T14:18:22Z

Damn. Time flies. I will definitely get this done by EOW.

oranagra · 2020-12-15T08:30:06Z

@jonahharris gentle nudge about this. in case you want someone else to try and pick this up from where you left it.

oranagra · 2020-12-29T15:26:05Z

i'm picking this up. (posting to avoid collision)

oranagra · 2020-12-31T17:08:42Z

@redis/core-team please approve a new ZRANGESTORE command and improvements to ZRANGE (see top comment)

oranagra · 2021-01-01T18:08:23Z

initial draft for the documentation PR: redis/redis-doc#1482
note that i'm not very comfortable with how it came out.
documenting all the nuances and instructions of BYSCORE / BYLEX / BYRANK in one page can make it quite complex.
instead i added a note to refer to the documentation of the deprecated commands for details (which sounds like a bad idea).
maybe that's a hint that this change is all wrong?
i.e. there are a lot of details about inclusive / exclusive searches and how the lexicographic ordering works, maybe unifying them into one command is wrong?

jonahharris · 2021-01-01T18:14:15Z

I don't know. It all makes sense to me and seems easy enough to understand. Most people I know who look at the Redis docs tend to just copy the examples and tailor them to their own case. As long as there are examples with the descriptions, as there are for the current commands, I don't see how it's any different.

madolson

Minor comment, API LGTM

src/t_zset.c

yossigo

API LGTM, only had a quick look at the code.

src/t_zset.c

Co-authored-by: Yossi Gottlieb <yossigo@gmail.com>

sundb · 2021-01-11T08:53:13Z

src/t_zset.c

-            addReplyBulkCBuffer(c,ln->ele,sdslen(ln->ele));
-            if (withscores) addReplyDouble(c,ln->score);
+            handler->emitResultFromCBuffer(handler, ln->ele, sdslen(ln->ele),
+              ((withscores) ? ln->score : ln->score));


@oranagra ((withscores) ? ln->score : ln->score) Shouldn't it be changed to ln->score

you're right, the ternary op can be dropped. but at least that's not a bug.
the store variant always passes true for withscores, and the non-store variant ignores the score if it wasn't requested.

sundb · 2021-01-11T09:01:27Z

src/t_zset.c

+        break;
+    }
+
+    if (opt_withscores || store) {


@oranagra I read the zunionInterDiffGenericCommand function, it should be similar, when there is a flag store, only the number of results will be replied, without specific data.
If there is a store flag, zrangeResultHandlerScoreEmissionEnable should not effect.

i'm not sure what you mean by this comment.
in case store is set, the call to zrangeResultHandlerScoreEmissionEnable is in some way redundant. since the two variables it sets are only used by the non-store variant handlers, so we can remove the || store from the above condition, and this function will never be called.

there's something a bit ugly in my final changes to this PR, is that i added the store argument to this function.
in theory, the handler should know what to do.
but i wanted to give syntax error on the WITHSCORES argument on the store variant, and added that store argument, that's where the mess started.

struct zrange_result_handler { zrange_consumer_type type; client *client; robj *dstkey; robj *dstobj; void *userdata; int withscores; int should_emit_array_length; int store; <- here what I add zrangeResultBeginFunction beginResultEmission; zrangeResultFinalizeFunction finalizeResultEmission; zrangeResultEmitCBufferFunction emitResultFromCBuffer; zrangeResultEmitLongLongFunction emitResultFromLongLong; };

Your idea should be exactly what I am trying to do. zrange_result_handler should know what it should do.

oranagra

@sundb thanks for this review.
if i understand you correctly, you're suggesting some cleanups (not reporting bugs or improvements).

since this one is merged already, feel free to open a PR for these.

oranagra · 2021-01-11T10:00:02Z

src/t_zset.c

-            addReplyBulkCBuffer(c,ln->ele,sdslen(ln->ele));
-            if (withscores) addReplyDouble(c,ln->score);
+            handler->emitResultFromCBuffer(handler, ln->ele, sdslen(ln->ele),
+              ((withscores) ? ln->score : ln->score));


you're right, the ternary op can be dropped. but at least that's not a bug.
the store variant always passes true for withscores, and the non-store variant ignores the score if it wasn't requested.

oranagra · 2021-01-11T10:49:19Z

src/t_zset.c

+        break;
+    }
+
+    if (opt_withscores || store) {


i'm not sure what you mean by this comment.
in case store is set, the call to zrangeResultHandlerScoreEmissionEnable is in some way redundant. since the two variables it sets are only used by the non-store variant handlers, so we can remove the || store from the above condition, and this function will never be called.

there's something a bit ugly in my final changes to this PR, is that i added the store argument to this function.
in theory, the handler should know what to do.
but i wanted to give syntax error on the WITHSCORES argument on the store variant, and added that store argument, that's where the mess started.

Add ZRANGESTORE command, and improve ZSTORE command to deprecated Z[REV]RANGE[BYSCORE|BYLEX]. Syntax for the new ZRANGESTORE command: ZRANGESTORE [BYSCORE | BYLEX] [REV] [LIMIT offset count] New syntax for ZRANGE: ZRANGE [BYSCORE | BYLEX] [REV] [WITHSCORES] [LIMIT offset count] Old syntax for ZRANGE: ZRANGE [WITHSCORES] Other ZRANGE commands remain unchanged. The implementation uses common code for all of these, by utilizing a consumer interface that in one command response to the client, and in the other command stores a zset key. Co-authored-by: Oran Agra <oran@redislabs.com>

…10337) Avoid deferred array reply on genericZrangebyrankCommand() when consumer type is client. I.e. any ZRANGE / ZREVRNGE (when tank is used). This was a performance regression introduced in #7844 (v 6.2) mainly affecting pipelined workloads. Co-authored-by: Oran Agra <oran@redislabs.com>

…10337) Avoid deferred array reply on genericZrangebyrankCommand() when consumer type is client. I.e. any ZRANGE / ZREVRNGE (when tank is used). This was a performance regression introduced in #7844 (v 6.2) mainly affecting pipelined workloads. Co-authored-by: Oran Agra <oran@redislabs.com> (cherry picked from commit 1dc89e2)

jonahharris added 4 commits September 24, 2020 15:10

Initial commit of zrangestore support

7b0cd00

inclusion fixes and debug removal

ab3f9d1

Fixed uninitialized flags and pass-down withscores for byscore

e8354cb

Misc. fptr renaming.

db70f2e

Merged into Redis code - fixed some formatting

54d558e

oranagra reviewed Sep 26, 2020

View reviewed changes

oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository state:needs-test-code the PR is missing test code labels Sep 26, 2020

oranagra added this to the Redis 6.2 milestone Sep 26, 2020

oranagra self-assigned this Sep 26, 2020

Updated several feedback items.

912cb68

oranagra linked an issue Oct 19, 2020 that may be closed by this pull request

Feature request, zrevrangebyscore with STORE option #678

Closed

oranagra removed this from the Next minor backlog milestone Oct 19, 2020

mp911de mentioned this pull request Nov 8, 2020

Add support for ZRANGESTORE command redis/lettuce#1506

Closed

oranagra added approval-needed Waiting for core team approval to be merged state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten and removed state:needs-test-code the PR is missing test code labels Dec 31, 2020

oranagra added 2 commits January 1, 2021 20:57

give explicit error msg on invalid combinations

6350b03

fix comment - WITHSCORES not supported on the STORE variant

91e713d

itamarhaber previously approved these changes Jan 5, 2021

View reviewed changes

madolson previously approved these changes Jan 6, 2021

View reviewed changes

src/t_zset.c Outdated Show resolved Hide resolved

src/t_zset.c Outdated Show resolved Hide resolved

minor cleanup

7fd0943

oranagra dismissed stale reviews from madolson and itamarhaber via 7fd0943 January 6, 2021 07:23

madolson previously approved these changes Jan 6, 2021

View reviewed changes

yossigo previously approved these changes Jan 7, 2021

View reviewed changes

src/t_zset.c Outdated Show resolved Hide resolved

src/t_zset.c Show resolved Hide resolved

comment typo.

3e76af4

Co-authored-by: Yossi Gottlieb <yossigo@gmail.com>

oranagra dismissed stale reviews from yossigo and madolson via 3e76af4 January 7, 2021 08:47

oranagra approved these changes Jan 7, 2021

View reviewed changes

oranagra merged commit b5029df into redis:unstable Jan 7, 2021

oranagra mentioned this pull request Jan 10, 2021

Redis 6.2 rc2 #8305

Merged

sundb reviewed Jan 11, 2021

View reviewed changes

oranagra reviewed Jan 11, 2021

View reviewed changes

sazzad16 mentioned this pull request May 12, 2021

[NEW] REV option in XRANGE to deprecate XREVRANGE #8936

Open

This was referenced Feb 20, 2022

[BUG] ZREVRANGE 50% slower after upgrading from 5.0.7 to 6.2.6 #10310

Closed

Reduce system calls of write for client->reply by introducing writev #9934

Merged

Optimization: Avoid deferred array reply on ZRANGE commands BYRANK #10337

Merged

New ZRANGESTORE Command #7844

New ZRANGESTORE Command #7844

Uh oh!

Conversation

jonahharris commented Sep 24, 2020 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Sep 25, 2020

Uh oh!

jonahharris commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahharris commented Sep 25, 2020

Uh oh!

oranagra commented Sep 26, 2020

Uh oh!

jonahharris commented Sep 26, 2020

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Oct 5, 2020

Uh oh!

jonahharris commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahharris commented Oct 16, 2020

Uh oh!

oranagra commented Oct 16, 2020

Uh oh!

oranagra commented Nov 10, 2020

Uh oh!

oranagra commented Dec 2, 2020

Uh oh!

jonahharris commented Dec 2, 2020

Uh oh!

oranagra commented Dec 15, 2020

Uh oh!

oranagra commented Dec 29, 2020

Uh oh!

oranagra commented Dec 31, 2020

Uh oh!

oranagra commented Jan 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahharris commented Jan 1, 2021

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yossigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sundb Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

oranagra Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

sundb Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

oranagra Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

sundb Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

jonahharris commented Sep 24, 2020 •

edited by oranagra

Loading

jonahharris commented Sep 25, 2020 •

edited

Loading

jonahharris commented Oct 6, 2020 •

edited

Loading

oranagra commented Jan 1, 2021 •

edited

Loading

sundb Jan 11, 2021 •

edited

Loading