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

create{Key/Read}Stream use the wrong keys #34

Open
bredthau opened this issue May 17, 2020 · 7 comments
Open

create{Key/Read}Stream use the wrong keys #34

bredthau opened this issue May 17, 2020 · 7 comments

Comments

@bredthau
Copy link

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 bredthau changed the title create{Key/Read}Stream create{Key/Read}Stream use the wrong keys May 17, 2020
@bcomnes
Copy link
Owner

bcomnes commented May 17, 2020

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
Copy link
Owner

bcomnes commented May 17, 2020

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.

@bredthau
Copy link
Author

bredthau commented May 17, 2020

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?

@bcomnes
Copy link
Owner

bcomnes commented May 17, 2020

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.

@bredthau
Copy link
Author

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?

@bcomnes
Copy link
Owner

bcomnes commented May 18, 2020

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

bredthau added a commit to bredthau/level-auto-index that referenced this issue May 21, 2020
@bredthau
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants