Add support for key ranges #40
Conversation
There was a problem hiding this comment.
The code looks great, but I am a little confused as to the state of iterators. Currently, it looks like this is using iterator.ts, but I thought this was supposed to be using harperdb/extended-iterable. And the description says it is blocked by the PR in the extended-iterable repo, is that true? Is the PR necessary for this to work?
The iterable work started in iterator.ts, but I'm around 99% donee moving it to extended-iterable. I read that extended-iterable not as high priority as dupSort. I'll find time to get the iterable work over the finish line. We should not merge this PR until the extended-iterable is ready. I am now focused on the dupSort functionality which is high priority. I'm working on in a branch from this key ranges branch. I'm testing |
|
Sorry if I have been unclear with priorities. I would say:
* Getting key ranges working is still high priority
* I had setup the extended-iterable repo so we could reuse the existing
code that we knew was working with Harper and expedite key ranges by
leveraging existing work
* Adding new functionality to extended-iterable (at, take, etc.) is very
cool and other refactorings may have benefits, but hard to justify as a
priority; we've never had any users ask for this. And these
additions/changes haven't been tested with Harper, which is likely to incur
some extra effort to ensure no regressions (in behavior or performance),
which isn't something I had planned in the prioritized schedule of work.
Anyway, you are welcome to finish IndexStore/IndexDBI/dupSort stuff. And
then naturally it would be nice to finish this as you have time. If we can
leverage existing extended-iterable to get this finished, that's great. And
if it makes more sense to finish extended-iterable work to finish this PR
that is fine as well.
…On Wed, Jun 18, 2025 at 9:42 AM Chris Barber ***@***.***> wrote:
*cb1kenobi* left a comment (HarperFast/rocksdb-js#40)
<#40 (comment)>
The code looks great, but I am a little confused as to the state of
iterators. Currently, it looks like this is using iterator.ts, but I
thought this was supposed to be using harperdb/extended-iterable. And the
description says it is blocked by the PR in the extended-iterable repo, is
that true? Is the PR necessary for this to work?
The iterable work started in iterator.ts, but I'm around 99% donee moving
it to extended-iterable. I read that extended-iterable not as high priority
as dupSort. I'll find time to get the iterable work over the finish line.
We should not merge this PR until the extended-iterable is ready.
I am now focused on the dupSort functionality which is high priority. I'm
working on in a branch from this key ranges branch. I'm testing
getValues() and finding the key range isn't working as I expected. I'm
actively working on it and will ensure this PR gets updated.
—
Reply to this email directly, view it on GitHub
<#40 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAIKBQEMH7LMYRSN3QQPXL3EGCGLAVCNFSM6AAAAAB6NLZJE2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSOBUG42TGOBVGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
There is considerable complexity involved in trying to replace lmdb-js with rocksdb-js and trying to replace the iterable module at the same time. When I tried using https://github.com/HarperDB/extended-iterable/pull/1/files with lmdb-js it immediately failed due to premature creation of the iterator on construction. I believe there is a considerable amount of work required of us to fully test and evaluate all the new extended-iterable code in the three places it will be used: lmdb-js (as used in Harper), Harper's query engine, and rocksdb-js (as used in Harper), and at least I don't have a lot of extra time for that right now. I think it would greatly beneficial to us if we could get this PR working with extended-iterable v1. And then once we have rocksdb working Harper (minimizing other changes involved), we could then apply extended-iterable v2 upgrades in more isolated and planned manner, rather than trying to do it all at once. |
|
I've removed the initial iterator implementation and replaced it with the complete extended-iterable v2. |
There was a problem hiding this comment.
The original request for this functionality was to use the lmdb-js's RangeIterable code. #29 explicitly pulled the RangeIterable code into a shared package for use for range iteration, and defined that we use v1 (^1.0.0).
After reviewing HarperFast/extended-iterable#1, I do not feel that v2 is ready for use at this time (although certainly hope to use in the future), and it is not proposed or part of the plan for ranges. Please adjust this PR to use v1 of the extended-iterable. This PR should not be blocked by HarperFast/extended-iterable#1.
| napi_value value; | ||
|
|
||
| rocksdb::Slice keySlice = itHandle->iterator->key(); | ||
| NAPI_STATUS_THROWS(::napi_create_buffer_copy( |
There was a problem hiding this comment.
These buffer creation, object creation, and property setting calls are likely pretty expensive, and the iterator.next function is an extremely hot code path. We don't necessarily need to address this in this PR, and we might want to benchmark/profile to better understand the performance of this, but likely we will have follow-up ticket(s) for addressing this.
There was a problem hiding this comment.
I agree 100%. I want to create a benchmark suite asap and so we have a baseline for optimizations. In this instance, I could see us using a shared buffer for keys.
Support for key range related database methods:
db.getRange()db.getKeys()db.getKeysCount()There's a new
NativeIteratorinternal class that exposes the RocksDB iterator instance along with a plethora of RocksDB specific iterator config settings.Note: version support has not be implemented, so
getValuesCount()is technically incomplete.