Skip to content

Conversation

@itamarhaber
Copy link
Member

@itamarhaber itamarhaber commented Dec 13, 2020

Implements the [count] variant of L/RPOP, provides partial resolution for #766 but leaves the blocking variants untouched for now.

Adds: L/RPOP <key> [count]

Implements no. 2 of the following strategies:

  1. Loop on listTypePop - this would result in multiple calls for memory freeing and allocating (see 769167a)
  2. Iterate the range to build the reply, then call quickListDelRange - this requires two iterations and is the current choice
  3. Refactor quicklist to have a pop variant of quickListDelRange - probably optimal but more complex

Also:

  • There's a historical check for NULL after calling listTypePop that is converted to an assert.
  • This refactors common logic shared between LRANGE and the new form of LPOP/RPOP into addListRangeReply (adds test for b/w compat)
  • Consequently, it may have made sense to have LRANGE l -1 -2 and LRANGE l 9 0 be legit and return a reverse reply. Due to historical reasons that would be, however, a breaking change.
  • Added minimal comments to existing commands to adhere to the style, make core dev life easier and get commit karma, naturally.

@itamarhaber itamarhaber added state:needs-doc-pr requires a PR to redis-doc repository state:major-decision Requires core team consensus labels Dec 13, 2020
@oranagra oranagra added 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 release-notes indication that this issue needs to be mentioned in the release notes labels Dec 20, 2020
@oranagra oranagra added the approval-needed Waiting for core team approval to be merged label Dec 20, 2020
@itamarhaber
Copy link
Member Author

@redis/core-team please review and approve if you haven't already. Thanks.

@itamarhaber itamarhaber requested a review from a team December 21, 2020 14:06
@itamarhaber itamarhaber merged commit f44186e into redis:unstable Dec 25, 2020
@itamarhaber itamarhaber deleted the count-pop branch December 25, 2020 19:49
@oranagra oranagra mentioned this pull request Jan 10, 2021
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Mar 2, 2021
Adds: `L/RPOP <key> [count]`

Implements no. 2 of the following strategies:

1. Loop on listTypePop - this would result in multiple calls for memory freeing and allocating (see redis@769167a)
2. Iterate the range to build the reply, then call quickListDelRange - this requires two iterations and **is the current choice**
3. Refactor quicklist to have a pop variant of quickListDelRange - probably optimal but more complex

Also:
* There's a historical check for NULL after calling listTypePop that was converted to an assert.
* This refactors common logic shared between LRANGE and the new form of LPOP/RPOP into addListRangeReply (adds test for b/w compat)
* Consequently, it may have made sense to have `LRANGE l -1 -2` and `LRANGE l 9 0` be legit and return a reverse reply. Due to historical reasons that would be, however, a breaking change.
* Added minimal comments to existing commands to adhere to the style, make core dev life easier and get commit karma, naturally.
oranagra pushed a commit that referenced this pull request Nov 4, 2021
Introduced in #8179, this fixes the command's replies in the 0 count edge case.
[BREAKING] changes the reply type when count is 0 to an empty array (instead of nil)
Moves LPOP ... 0 fast exit path after type check to reply with WRONGTYPE
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jan 10, 2022
It used to return `$-1` in RESP2, now we will return `*-1`.
This is a bug in redis 6.2 when COUNT was added, the `COUNT`
option was introduced in redis#8179. Fix redis#10089.
oranagra pushed a commit that referenced this pull request Jan 11, 2022
)

It used to return `$-1` in RESP2, now we will return `*-1`.
This is a bug in redis 6.2 when COUNT was added, the `COUNT`
option was introduced in #8179. Fix #10089.

the documentation of [LPOP](https://redis.io/commands/lpop) says
```
When called without the count argument:
Bulk string reply: the value of the first element, or nil when key does not exist.

When called with the count argument:
Array reply: list of popped elements, or nil when key does not exist.
```
oranagra pushed a commit that referenced this pull request Apr 27, 2022
Introduced in #8179, this fixes the command's replies in the 0 count edge case.
[BREAKING] changes the reply type when count is 0 to an empty array (instead of nil)
Moves LPOP ... 0 fast exit path after type check to reply with WRONGTYPE

(cherry picked from commit 06dd202)
oranagra pushed a commit that referenced this pull request Apr 27, 2022
)

It used to return `$-1` in RESP2, now we will return `*-1`.
This is a bug in redis 6.2 when COUNT was added, the `COUNT`
option was introduced in #8179. Fix #10089.

the documentation of [LPOP](https://redis.io/commands/lpop) says
```
When called without the count argument:
Bulk string reply: the value of the first element, or nil when key does not exist.

When called with the count argument:
Array reply: list of popped elements, or nil when key does not exist.
```

(cherry picked from commit 39feee8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged 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:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants