Level / abstract-leveldown

An abstract prototype matching the leveldown API.
MIT License
146 stars 53 forks source link

Invalid Reverse Iterator Seek Test #356

Closed VannaDii closed 4 years ago

VannaDii commented 4 years ago

It seems the test for iterator#seek() on reverse iterator is invalid. This test uses make which batches three items with keys one, two, three, then creates an iterator with { reverse: true, limit: 1 }, then attempts to seek to key three! expecting to find it and for the key to be three.

Possibly invalid assumptions:

  1. Items are stored in the order they are received: { reverse: true, limit: 1 }
  2. Key seeking is done by subset matching: three! != three but does start with three

Possible changes:

  1. Remove { limit: 1 } to seek all items in reverse and tolerate the subset match
  2. Correct the seek('three!') call to be seek('three')

If both changes are made then this test passes. Am I missing some reason why this is a valid test?

vweevers commented 4 years ago

Following LevelDB logic, forwards seeks land on the target, or the key after it. In reverse mode, we reversed that logic: a backwards seek lands on the target or the key before it.

VannaDii commented 4 years ago

@vweevers that makes sense, however, it also makes assumption 1 - items are stored in the order received. It seems problematic to impose this storage rule on the underlying persistence layer, no?

Also, in this case, the key provided to seek doesn't exist at all, it was never put.

vweevers commented 4 years ago

Also, in this case, the key provided to seek doesn't exist at all, it was never put.

I'll rephrase: seeks land on the target or - if that key does not exist - the key after/before it.

Items are sorted lexicographically. In this case, the inserted keys one, two, three sort as one, three, two. If you imagine the key three! in between them, they sort as one, three, three!, two. Seeking backwards to three! would land on three! if that key existed, but it doesn't, so it lands on three.

VannaDii commented 4 years ago

Ahhh, I see now. Thank you for that explanation. This might be a good explanation to add to the iterator documentation? I'll close this issue. Sorry for the trouble.

vweevers commented 4 years ago

No trouble at all.

VannaDii commented 4 years ago

Wait, but doesn't limit: 1 mean that only two would be retrieved in reverse landing the seek on undefined?

vweevers commented 4 years ago

The limit applies to the total number of items retrieved, not to what's on disk. It does a seek first, then from it where landed, retrieves items until the limit is reached.