Closed MeirionHughes closed 2 years ago
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:
abstract-leveldown
test suite options to opt-in to testslevelup
test suite options to opt-in to testssure 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
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.
Superseded by https://github.com/Level/abstract-leveldown/pull/381
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?