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

Already on GitHub? Sign in to your account

Variadic list commands (RPUSHX, LPUSHX, LINSERT) #1232

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

catwell commented Aug 6, 2013

Following this thread I set out to implement variadic LINSERT.

I have started by separating the implementation of LINSERT from that of RPUSHX and LPUSHX since they were not so similar in the first place and would need to become even more different. Then I noticed making RPUSHX and LPUSHX variadic (like RPUSH / LPUSH) as well would be easy so I did that.

I am rather confident in those first commits, the most challenging one is the last one: actually making LINSERT variadic.

I had not expected that list iterators would break when used with ziplists (it is impossible to re-use an iterator after insertion). That means (unless I have missed something) that sometimes I need to iterate a ziplist up to the pivot as many times as there are elements to be inserted. I decided to try as hard as possible to avoid this case by converting to a regular list early if I could. This is why that code can look a bit complex and should probably be reviewed with attention...

Contributor

catwell commented Aug 6, 2013

Note: an optimization I could make is to increment a counter when I iterate and feed it back to the later calls (2nd argument of listTypeInitIterator) to skip to the right place faster. I could use that in two ways:

  • count when iterating a ziplist to convert it, feed the result to the iterator on the regular list;
  • count on the first iteration on a ziplist, feed the result to the following ones.

That being said it would make that code even more complicated...

Another possibility would be to change the way iterators work on ziplists so that inserting never breaks but I am not sure that is feasible.

Owner

antirez commented Aug 21, 2013

Milestone -> 3.0

Contributor

catwell commented Oct 7, 2014

Weird, why is this PR referencing stuff from RethinkDB? Is that a GitHub bug?

Collaborator

badboy commented Oct 7, 2014

Indeed, this looks like a GitHub bug

Contributor

mattsta commented Oct 7, 2014

Yeah, it has been happening for a few weeks now across many issues. You can see they even show commits based on the wrong repo (this isn't a redis commit: cce7c6e)

Contributor

catwell commented Jun 5, 2016

This is superseded by #3296. It only makes RPUSHX and LPUSHX variadic, not LINSERT. The LINSERT code will have to be rewritten from scratch because the list type has changed too much in the meantime.

@antirez antirez closed this Jun 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment