level-auto-index icon indicating copy to clipboard operation
level-auto-index copied to clipboard

create{Key/Read}Stream use the wrong keys

Open bredthau opened this issue 5 years ago • 7 comments

According to the documentation createKeyStream and createValueStream should use the indices as keys and the indexed values as values. However in reality it uses the keys of the original db on which the index has been created.

const autoLevelIdx  = require("level-auto-index");
const level         = require("level-mem");
async function printStream(text, stream, print) {
    await new Promise((res) => {
        stream.on("data", e => text += " | " + print(e));
        stream.on("end", res);
    });
    console.log(text);
}
(async function() {
    const db     = level();
    const byTitle = autoLevelIdx(db, level(), val => val);
    await Promise.all([db.put("A", "X"), db.put("B", "Y"), db.put("C", "Z")]);
    await printStream("ReadStream", byTitle.createReadStream(),  ({key, value}) => `${key}: ${value}`);
    await printStream("KeyStream",  byTitle.createKeyStream(),   (key) => key);
})();

According to the documentation this code should output:

ReadStream | X: X | Y: Y | Z: Z
KeyStream | X | Y | Z

In reality it generates the following output:

ReadStream | A: X | B: Y | C: Z
KeyStream | A | B | C

bredthau avatar May 17 '20 08:05 bredthau

Looks like it’s working as intended. It looks like you are confusing how the underlying index db works and how the higher level index api is presenting that data.

If you want you can hold reference to the level instance you pass in when creating the index, then create read streams off that. Don’t write to it though because you will bypass the lifecycle hooks.

I would write an example but I’m mobile atm.

I think there are tests on this as well but it’s been while since I wrote it.

bcomnes avatar May 17 '20 17:05 bcomnes

The index is generating key variances for various sorting and grouping properties. But it all points back to a master document.

The high level api saves you the time of having to do the mapping yourself.

You just read stream the indexed let’s but then always get back the correct documents.

bcomnes avatar May 17 '20 17:05 bcomnes

The documentation states Create a readable stream that has indexes as keys and indexed data as values. So from my understanding of that sentence the keys of the readable stream should be from the indices and not the keys used in the original database. There is indeed a test for the current behavior (read-stream.js). I actually found that before opening the issue, but that doesn't change my issue with the discrepancy (at least how I read it) between the documentation and the behavior.

Which behavior is the correct one is of course a matter of opinion with valid arguments for both sides. The documentation gives more intuitive streams considering that both borders (lt, gt options) and the order of the elements in the stream match the keys given by the index and not the original database. On the other hand not having the original keys can obviously be less than ideal. The is obviously also the possibilty of putting both the key and the index-key into the stream elements by adding another object field in line 163.

In my specific scenario I have database-entries getting multiple index-entries, so knowing which index-key a specific occurence corresponds to is a very important part, so I did actually end up doing the mapping by hand. However if the behaviour is as intended or at least too much of a breaking change to modify, I would at least suggesting to change the documentation to be more clear about the behavior. Personally I would favor an option to switch between the two options. Would you be open to merging such a feature if I implemented it?

bredthau avatar May 17 '20 20:05 bredthau

Maybe we can add a third value like indexKey to createReadStream?

Would that cut it?

iirc I copied the original design from Julian Grubers module.

bcomnes avatar May 17 '20 23:05 bcomnes

Sure, that would also work nicely. Do you want to add it (should be indexKey: idbKey at line 162 + Readme and test modifications) or should I create a pull request?

bredthau avatar May 18 '20 06:05 bredthau

You can add if your want. Otherwise I can try and take a look this week

bcomnes avatar May 18 '20 13:05 bcomnes

I have added an indexKey field and updated Readme and tests accordingly (#35 ). I haven't updated the changeglog though, since I assumed that you might want to update the dependencies in the same bumb.

bredthau avatar May 21 '20 10:05 bredthau