Level/community

Proposal: add map method to abstract-down

MeirionHughes opened this issue · 4 comments

MeirionHughes commented

I'd like to push for getting the multiread map method into the 'downs.

immediate proposal is to add map and _map to abstract-down. where the default (not overridden) map on abstract-down would be a fallback to a simple loop + get.

As per linked issue:;

Con: additional abstract method map on abstract-down to maintain - I doubt this will be a problem as the fallback is using existing methods and it unlikely to ever need touching again.

Pro: allows down's to reduce cross-boundary calls from O(N) to O(1), with further potential saving from using single cache rather than several when returning the result.

Use-case is secondary index lookup. very fast to iterate over primary keys and secondary keys, but the map from secondary key to primary can only currently be implemented with an interator seek + next per row (snapshot) or looping get

just the cross-boundary call reduction (and potential buffering of results) should improve performance, however db's like rocksdb also have native support too

I'd be happy to implement if there are no objections to adding it.

I guess the issue of contention will be the next need to get it into levelup and subleveldown - which might be adding more work as I believe the current plan is to merge up and down?

vweevers commented

For the method name I prefer getAll() for its close relation to get().

where the default (not overridden) map on abstract-down would be a fallback to a simple loop + get

We may want to consider not having a default implementation, because if the default is significantly slower than a userland (iterator-based) approach, I would not recommend using it.

I guess the issue of contention will be the next need to get it into levelup and subleveldown - which might be adding more work as I believe the current plan is to merge up and down?

That plan is progressing slowly, doesn't block this. What would help though is adding a new boolean property (default false) to:

  1. manifests for determining support at runtime
  2. abstract-leveldown test suite options to opt-in to tests
  3. levelup test suite options to opt-in to tests
MeirionHughes commented

sure I can change to getAll()

The default is to ensure it doesn't break at least - clear has a default fallback too. Having a default that uses the iterator (seek/next) would probably be better though as it would be a snapshot. If its undesirable, can remove it.

I'll rework the PR + the review issues

vweevers commented

The default is to ensure it doesn't break at least

It can instead yield an empty array. Consumers that need getAll() can check e.g. if (db.supports.getAll), at least until all implementations have caught up. This approach allows us to add non-functional features to the abstract-leveldown interface as semver-minor.

clear has a default fallback too

True, though the difference is that _clear() is equally performant to what userland code could do.

as it would be a snapshot

Good point - that's IMO another reason not to have a default implementation. Though we could in theory use an iterator, that would require seek() support, and encodings could be tricky.