Skip to content
This repository

make reverse easier to use #110

Open
dominictarr opened this Issue · 17 comments

7 participants

Dominic Tarr Paolo Fragomeni Luke Arduini Julian Gruber Rod Vagg Raynos (Jake Verbaten) David Björklund
Dominic Tarr
Collaborator

right now, if you want to read a range in reverse mode,
it's tricky, because you have to reverse the order of the start and end too.

Forward:

db.createReadStream({start: 'A', end: 'Z'})

Reverse:

db.createReadStream({start: 'Z', end: 'A', reverse: true})

this is redundant, this feature would be much easier to use if you could do

db.createReadStream({start: 'A', end: 'Z', reverse: true})

And levelup would just see that start < end, and you are in reverse mode, and switch them around.

Alternatively, you could emit an error if the range was wrong, but I think I prefer just switching the order.

Paolo Fragomeni
Collaborator

+1 This should be a trivial change.

Dominic Tarr
Collaborator

oh... I just discovered this:

https://github.com/rvagg/node-levelup/blob/master/test/read-stream-test.js#L230-L232

This is totally not what I expected. I would consider this a bug.
is this leveldb behaviour?

Also, if I give it a range like

db.createReadStream({start:'~', reverse: true})
  .on('data', console.log)

and the last key in the db is BEFORE '~', say, 'Z', then I get a value not found error.
however, if I do

db.createReadStream({reverse: true})
  .on('data', console.log)

then I will get the last item!

Dominic Tarr
Collaborator

I think when reverse: true it should return exactly the same records that it would when reverse: false,
only, it reverse order.

Dominic Tarr
Collaborator

That means, there needs to be a canonical way to refer to each range.

From the start of the DB to X

{start: null, end: X}

From X to the end of the DB

{start: X, end: null}
Dominic Tarr dominictarr referenced this issue in juliangruber/level-store
Open

Chunk Length based keys #7

Luke Arduini
luk- commented

I think when reverse: true it should return exactly the same records that it would when reverse: false,
only, it reverse order.

This.

Julian Gruber
Collaborator

Big +1 from me, reverse ranges are too weird atm.

Dominic Tarr
Collaborator

the best approach with this may be to add options for cannocal ranges: but using options {min: m, max: M}, which mean the same thing as start and end when reverse=false but will return the same records when in reverse mode.

This could be added in parallel to start, end and which could possibly be depreciated later.

Rod Vagg
Owner
rvagg commented

I find it strange that this has got so much attention from everyone; the current 'start' and 'end' seem perfectly logical to me but this is really just an illustration of the problem of the perspective of an implementer vs a user. I see 'start' and 'end' as relating to iterator positioning but the whole concept of iterators is abstracted away for the user so I guess everyone view them as the start & end of a range.

ATM I think the way to go would be to switch to 'min' and 'max' for clarity, I guess 'start' and 'end' are pretty important to most people so we can leave them in the code (undocumented, or with a deprecation note) where a 'reverse'=true will switch their mapping.

It would go well with @kesla's changes in rvagg/node-leveldown#30 and #111.

Who wants to do the work? The tests can all change to 'min' and 'max' but it'd be nice to have a couple of tests that check the mapping for both forward and reverse with 'start' and 'end'.

While we're at it, is everyone happy with 'max'/'end' being inclusive? There's no reason to switch to exclusivity or add an option to turn that on?

Dominic Tarr
Collaborator

when I started using leveldb I expected start,end to be inclusive of start, but exclusive of end.
I eventually got the hang of that, though...
exclusivity would be implemented by just ignoring the last/first item, right?

exclusiveMin: true, exclusiveMax: true or exclusive: true

Can't quite figure out why I expect max to be exclusive, but not min?

Raynos (Jake Verbaten)
Collaborator

@dominictarr that's how arrays work in javascript. They are 0 indexed.

for (var i = min; i < max; i++) console.log(i)

That's how we write for loops in javascript.

Dominic Tarr
Collaborator

aha, also, if you specify ranges with inclusive of min but exclusive of max then the ranges touch each other but do not overlap

[{min: 0, max: 10}, {min: 10, max: 20}]

etc

you need one inclusive bound, and one exclusive bound to get this property.
I guess you could go either way, but min inclusive feels a lot more natural, hence the array thing, etc.

Also, in everyday life, ranges are typically specified with an exclusive max,
"bridge clearance 3.2 metres" and inclusive min "the minimum drinking age is 18".

I'm +1 on exclusive max.

David Björklund
Collaborator
kesla commented

Using exclusive max would also be consistent with how approximateSize works (https://github.com/rvagg/node-levelup#approximateSize)

Paolo Fragomeni
Collaborator

From the UX/UI perspective (ie: levelweb), reverse makes very little sense the way it works right now. A user expects reverse to be applied to the start and end values as they are.

Rod Vagg
Owner
rvagg commented
##leveldb <hij1nx> rvagg: so... what do you think about this? I think we should figure this out.
##leveldb <rvagg> hij1nx: atm I'm thinking of introducing 'min' and 'max' and deprecating 'start' and 'end'
##leveldb <rvagg> hij1nx: where min/max would do what everyone seems to want in the 'reverse' case
Dominic Tarr
Collaborator

I have a module for this, I can turn it into a pull request -- or do we want to fix it in levelDOWN?

Rod Vagg
Owner
rvagg commented

I think that in an iterator that has a next() makes sense to have a 'start' and 'end' that point to where the iterator actually starts and where it ends, regardless of the direction. So, I think it makes sense to leave LevelDOWN as is and introduce 'min' and 'max' in LevelUP, deprecating 'start' and 'end' (but leaving them as they are).

Go ahead and put in a PR, I'd like to resolve #118 at the same time as this though.

Dominic Tarr
Collaborator

great, will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.