Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add db.keys(), .values(), iterator.nextv() and .all() #12

Merged
merged 2 commits into from
Jan 16, 2022

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Jan 9, 2022

I'm squeezing this in for similar reasons as #8. I wondered whether these additions would be breaking. The short answer is no. In addition, adding this now means level-read-stream can make use of it without conditional code paths based on db.supports.*.

Ref Level/abstract-leveldown#380

Adds key and value iterators: db.keys() and db.values(). As a replacement for levelup's db.create{Key,Value}Stream(). Example:

for await (const key of db.keys({ gte: 'a' }) {
  console.log(key)
}

Adds two new methods to all three types of iterators: nextv() for getting many items (entries, keys or values) in one call and all() for getting all (remaining) items. These methods have functional but suboptimal defaults: all() falls back to repeatedly calling nextv() and that in turn falls back to next(). Example:

const values = await db.values({ limit: 10 }).all()

Adds a lot of new code, with unfortunately some duplicate code because I wanted to avoid mixins and other forms of complexity, which means key and value iterators use classes that are separate from preexisting iterators. For example, a method like _seek() must now be implemented on three classes: AbstractIterator, AbstractKeyIterator and AbstractValueIterator. This (small?) problem extends to implementations and their subclasses, if they choose to override key and value iterators to improve performance.

On the flip side, the new methods are supported across the board: on sublevels, with encodings, with deferred open, and fully functional. This may demonstrate one of the major benefits of abstract-level over abstract-leveldown paired with levelup.

Yet todo:

  • Tests
  • Replace use of level-concat-iterator in existing tests
  • After that, try another approach with a mode property on iterators, that is one of entries, keys or values (moving logic to if-branches... I already don't like it but it may result in cleaner logic downstream).

@vweevers vweevers added the enhancement New feature or request label Jan 9, 2022
@vweevers
Copy link
Member Author

vweevers commented Jan 9, 2022

@Level/core Side note: I'm concerned about the bus factor here, adding code to an already-refactored code base that no one else is familiar with. Realistically, there's not much I can do about that, besides spending more time not writing code (i.e. looking for contributors, funding, doing that awful thing called self-marketing). I'm still having fun here though, so I'm moving ahead. Just let me know if there's anything I can do to facilitate code reviews.

@juliangruber
Copy link
Member

Hey @vweevers, thanks for all the work you're putting in and the clarity of communication! I personally currently don't have the time to look at large PRs, so if it made sense to make smaller PRs that would help me but also I don't think the bus factor is critical here as everything looks to be well organized and your direction is clear

@vweevers
Copy link
Member Author

vweevers commented Jan 9, 2022

@juliangruber Noted, thanks! Once I'm past the current refactoring/forking stage, smaller PRs will be easier. For now I will remind myself to make less (unrelated) tweaks, like how this PR includes README changes that I could easily do separately.

Ref Level/abstract-leveldown#380

Adds key and value iterators: `db.keys()` and `db.values()`. As
a replacement for levelup's `db.create{Key,Value}Stream()`. Example:

```
for await (const key of db.keys({ gte: 'a' }) {
  console.log(key)
}
```

Adds two new methods to all three types of iterators: `nextv()` for
getting many items (entries, keys or values) in one call and `all()`
for getting all (remaining) items. These methods have functional
but suboptimal defaults: `all()` falls back to repeatedly calling
`nextv()` and that in turn falls back to `next()`. Example:

```
const values = await db.values({ limit: 10 }).all()
```
@vweevers
Copy link
Member Author

vweevers commented Jan 16, 2022

Added tests and needed a few more things to make those pass:

  • For the default nextv() and all() to honor the limit option, abstract iterators now count how many items they yielded, which may remove the need for implementations to do so on their own. Iterators have new public count and limit properties. The limit option now also accepts Infinity, with the same meaning as -1 (technically that was already the case, but it's now explicit).
  • Added tests that gte and lte range options take precedence over gt and lt respectively - on iterators, clear() and seek(). This is incompatible with ltgt but aligns with subleveldown, level-option-wrap and half of leveldown. There was no good choice.

To improve coverage, test/self.js now includes a minimal abstract-level implementation, which only supports utf8 but is otherwise fully compliant, and is used to test the abstract test suite. Without it, coverage is 91%. With it's 98%. Also useful just to demonstrate the new API's and the problem that I outlined in the OP:

abstract-level/test/util.js

Lines 121 to 278 in 273de0d

class MinimalLevel extends AbstractLevel {
constructor (options) {
super({ encodings: { utf8: true }, seek: true }, options)
this[kEntries] = new Map()
}
_put (key, value, options, callback) {
this[kEntries].set(key, value)
this.nextTick(callback)
}
_get (key, options, callback) {
const value = this[kEntries].get(key)
if (value === undefined) {
return this.nextTick(callback, new ModuleError(`Key ${key} was not found`, {
code: 'LEVEL_NOT_FOUND'
}))
}
this.nextTick(callback, null, value)
}
_getMany (keys, options, callback) {
const values = keys.map(k => this[kEntries].get(k))
this.nextTick(callback, null, values)
}
_del (key, options, callback) {
this[kEntries].delete(key)
this.nextTick(callback)
}
_clear (options, callback) {
for (const [k] of sliceEntries(this[kEntries], options, true)) {
this[kEntries].delete(k)
}
this.nextTick(callback)
}
_batch (operations, options, callback) {
const entries = new Map(this[kEntries])
for (const op of operations) {
if (op.type === 'put') entries.set(op.key, op.value)
else entries.delete(op.key)
}
this[kEntries] = entries
this.nextTick(callback)
}
_iterator (options) {
return new MinimalIterator(this, options)
}
_keys (options) {
return new MinimalKeyIterator(this, options)
}
_values (options) {
return new MinimalValueIterator(this, options)
}
}
class MinimalIterator extends AbstractIterator {
constructor (db, options) {
super(db, options)
this[kEntries] = sliceEntries(db[kEntries], options, false)
this[kOptions] = options
this[kPosition] = 0
}
}
class MinimalKeyIterator extends AbstractKeyIterator {
constructor (db, options) {
super(db, options)
this[kEntries] = sliceEntries(db[kEntries], options, false)
this[kOptions] = options
this[kPosition] = 0
}
}
class MinimalValueIterator extends AbstractValueIterator {
constructor (db, options) {
super(db, options)
this[kEntries] = sliceEntries(db[kEntries], options, false)
this[kOptions] = options
this[kPosition] = 0
}
}
for (const Ctor of [MinimalIterator, MinimalKeyIterator, MinimalValueIterator]) {
const mapEntry = Ctor === MinimalIterator ? e => e : Ctor === MinimalKeyIterator ? e => e[0] : e => e[1]
Ctor.prototype._next = function (callback) {
const entry = this[kEntries][this[kPosition]++]
if (entry === undefined) return this.nextTick(callback)
if (Ctor === MinimalIterator) this.nextTick(callback, null, entry[0], entry[1])
else this.nextTick(callback, null, mapEntry(entry))
}
Ctor.prototype._nextv = function (size, options, callback) {
const entries = this[kEntries].slice(this[kPosition], this[kPosition] + size)
this[kPosition] += entries.length
this.nextTick(callback, null, entries.map(mapEntry))
}
Ctor.prototype._all = function (options, callback) {
const end = this.limit - this.count + this[kPosition]
const entries = this[kEntries].slice(this[kPosition], end)
this[kPosition] = this[kEntries].length
this.nextTick(callback, null, entries.map(mapEntry))
}
Ctor.prototype._seek = function (target, options) {
this[kPosition] = this[kEntries].length
if (!outOfRange(target, this[kOptions])) {
// Don't care about performance here
for (let i = 0; i < this[kPosition]; i++) {
const key = this[kEntries][i][0]
if (this[kOptions].reverse ? key <= target : key >= target) {
this[kPosition] = i
}
}
}
}
}
const outOfRange = function (target, options) {
if ('gte' in options) {
if (target < options.gte) return true
} else if ('gt' in options) {
if (target <= options.gt) return true
}
if ('lte' in options) {
if (target > options.lte) return true
} else if ('lt' in options) {
if (target >= options.lt) return true
}
return false
}
const sliceEntries = function (entries, options, applyLimit) {
entries = Array.from(entries)
.filter((e) => !outOfRange(e[0], options))
.sort((a, b) => a[0] > b[0] ? 1 : a[0] < b[0] ? -1 : 0)
if (options.reverse) entries.reverse()
if (applyLimit && options.limit !== -1) entries = entries.slice(0, options.limit)
return entries
}

This PR is good to go, but the amount of code makes me want to sleep on it.

@vweevers
Copy link
Member Author

I'm happy with the public API. Less so with the private API and tests (which in short are "structurally inconsistent") but it's easy enough (for implementations) to work around. In addition, I want to start benchmarking (#4) and I'm making it harder for myself to benchmark fairly when I'm moving further away from abstract-leveldown and levelup. So, I'm opening an issue to later refactor the private API (likely in a next major).

@vweevers vweevers marked this pull request as ready for review January 16, 2022 12:19
@vweevers vweevers merged commit 7113ad1 into main Jan 16, 2022
@vweevers vweevers deleted the keys-values-nextv-all branch January 16, 2022 12:19
vweevers added a commit to Level/classic-level that referenced this pull request Feb 19, 2022
On the C++ side:

- Replace asBuffer options with encoding options
- Refactor iterator_next to work for nextv(). We already had a
  iterator.ReadMany(size) method in C++, with a hardcoded size.
  Now size is taken from the JS argument to _nextv(size). The
  cache logic for next() is the same as before.
  Ref Level/community#70
  Ref Level/abstract-level#12
- Use std::vector<Entry> in iterator.cache_ instead of
  std::vector<std::string> so that the structure of the cache
  matches the desired result of nextv() in JS.

On the JS side:

- Use classes for ChainedBatch, Iterator, ClassicLevel
- Defer approximateSize() and compactRange()
- Encode arguments of approximateSize() and compactRange(). Ref
  Level/community#85
- Add promise support to additional methods
- Remove tests that were copied to abstract-level.

This is the most of it, a few more changes are needed in follow-up
commits.
vweevers added a commit to Level/classic-level that referenced this pull request Feb 19, 2022
On the C++ side:

- Replace asBuffer options with encoding options
- Refactor iterator_next to work for nextv(). We already had a
  iterator.ReadMany(size) method in C++, with a hardcoded size.
  Now size is taken from the JS argument to _nextv(size). The
  cache logic for next() is the same as before.
  Ref Level/community#70
  Ref Level/abstract-level#12
- Use std::vector<Entry> in iterator.cache_ instead of
  std::vector<std::string> so that the structure of the cache
  matches the desired result of nextv() in JS.

On the JS side:

- Use classes for ChainedBatch, Iterator, ClassicLevel
- Defer approximateSize() and compactRange()
- Encode arguments of approximateSize() and compactRange(). Ref
  Level/community#85
- Add promise support to additional methods
- Remove tests that were copied to abstract-level.

This is the most of it, a few more changes are needed in follow-up
commits.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants