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

make reverse easier to use #110

Closed
dominictarr opened this Issue Mar 26, 2013 · 18 comments

Comments

Projects
None yet
8 participants
Owner

dominictarr commented Mar 26, 2013

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.

Owner

0x00A commented Mar 26, 2013

+1 This should be a trivial change.

Owner

dominictarr commented Mar 26, 2013

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!

Owner

dominictarr commented Mar 26, 2013

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

Owner

dominictarr commented Mar 26, 2013

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}

@dominictarr dominictarr referenced this issue in juliangruber/level-store Mar 26, 2013

Open

Chunk Length based keys #7

luk- commented Mar 27, 2013

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

This.

Owner

juliangruber commented Apr 1, 2013

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

Owner

dominictarr commented Apr 1, 2013

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.

Owner

rvagg commented Apr 1, 2013

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?

Owner

dominictarr commented Apr 4, 2013

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?

Owner

Raynos commented Apr 4, 2013

@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.

Owner

dominictarr commented Apr 4, 2013

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.

Owner

kesla commented Apr 5, 2013

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

Owner

0x00A commented Apr 14, 2013

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.

Owner

rvagg commented Apr 14, 2013

##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
Owner

dominictarr commented Apr 14, 2013

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

Owner

rvagg commented Apr 14, 2013

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.

Owner

dominictarr commented Apr 14, 2013

great, will do.

Owner

ralphtheninja commented May 13, 2015

I believe this was resolved by introducing lte and gte. Please reopen if needed.

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