Comparator helpers keyin and nkeyin don't work as expected

[alexandernst]

Will submit a PR to fix it. (I'm creating this issue just as a reminder to myself)

[VladimirTechMan]

@alexandernst As the original author of $keyin and $nkeyin who uses them extensively, I can say that they work correctly for me. (Having said that, they were not documented publicly yet, thus the interpretation of what they do could be different on your side.) What was your expectation here? You did not detail it, thus I can only guess.

The intended usages of those two operators is to have an alternative to the original $in and $nin operators. The latter two use an array of values and allow to check if a field value is (not) one of those array values. The former two, recently introduced operators $keyin and $nkeyin, allow to check if a field value is (not) an existing key of an object passed as the operator's parameter. The operators are designed to work with any "dictionary" objects. They are also designed so that any existing keys with value undefined are treated as non-existent.

Thus, please, detail what you wanted (expected) to get.

[alexandernst]

@VladimirTechMan They are also designed so that any existing keys with value undefined are treated as non-existent.

Ah, so it was designed that way. I'd say this shouldn't happen, as {'foo': undefined} is a completely valid object and it does have a foo key ($keyin would return false).

IMHO this should be done with hasOwnProperty and the function should retrun true if the key exists, no matter what the values is, because, as the name of the function says, it queries the keys, not the values.

[alexandernst]

@VladimirTechMan @techfort @obeliskos If you agree with my previos comment I can make a PR.

[VladimirTechMan]

@alexandernst Yeah, when I saw your issue report first, I guessed you raised it because of that specific point. The reason I implemented it like that was performance. Checks using direct strict comparison with undefined work faster in modern JS engines compared to hasOwnProperty. In my case, with huge doc sets of up to 40-50K docs that was important. But I would not necessarily sacrifice the "true property test" ability just because of that reason itself (I want to make Loki as fast as possible for my purposes, but not in any way that would negatively affect other projects / intended usages).

The expected use-case for me is that a developer builds (or "generates" in more complex cases) the query for Loki "from scratch" and thus he/she can easily build the "dictionary" object for this operator in a way that does not need to use undefined for key values. @alexandernst, do you have a use case in your app where you actually need to pass arbitrary "external" dictionary objects inside the query (and in case if those are used as part of DynamicView, they would have to stay immutable then, as changing them on-the-fly would affect the correctness of the dynamic view updates)? Please, let me know. I am very open to discuss.

One more thing to mention: A while ago I had a discussion wth @obeliskos and @techfort over another potential "ambiguity" related to the interpretation of undefined for existing vs. non-existing keys in the collection docs. A different topic, but somewhat related to this one from a higher-level perspective. Thus, I thought it would be good for you to know about it (esp. as you are actively contributing to Loki now). Look here for more details:

#321

[alexandernst]

@VladimirTechMan Indeed, hasOwnProperty is a little bit slower, but we're hitting here the exact same problem we're hitting in another of my PRs, where we had to make the lt/gt Helper slower because they just needed to compare booleans, as they were misbehaving in some scenarios.

I might have a case were I need to build a query object, but it still depends on some other things. I'll probably post a link here about what I'm doing with Loki, so you can pick ideas if you want.

Good points in #321, but I feel I'm skipping something, I should probably re-read it once or twice.

[VladimirTechMan]

@alexandernst It would be good to know more about your project and the ways you use / want to use Loki (esp. that you previously mentioned Lunr.js). Then, understanding those details, I may feel better about switching to hasOwnProperty as I know the practical case where pre-building a dictionary object for $keyin or $nkeyin cannot be done or would not work well.

As for my project / use case (not sure if you're interested, but just to mention), I gave a brief description of it to Joe in the following thread: #319.

About #321, the comments were lengthy, indeed. :) But the basic point is: Assume you have a collection of docs with a non-uniform field structure. Some of them have a certain, specific field with a numeric type of value. Other docs do not have that field at all. Now, you want to query all docs where that field's value is less than (or less-than-or-equal-to) a given number. Doing that, would you expect that the result set will also contain all the docs that do not actually have that field in them? – that will be the case with Loki. That discussion was my attempt to define how we document that for Loki users and how much (and in which specific way) we want to support the docs with key values set to undefined. MongoDB, for example, seems to start going away from that. (Loki is not Mongo, but as it does take a part of its inspiration and a part of query language from there, it is worth taking that into consideration.) Anyway, that is just a general comment.

[alexandernst]

@VladimirTechMan Sure, I'm creating a multi-select angular directive. Will post github link once it's done, but it's basically this: https://raw.githubusercontent.com/isteven/angular-multi-select/master/screenshot.png

[alexandernst]

Ah, the lunr.js thing is because I need to search in the items and show only the ones that match with the searched text. I'm still not very sure how to do that. If build some sort of bridge between loki and lunr or just run both in parallel and bridge the results. Must decide in a few days.

[alexandernst]

Btw, I have given this a second thought. We could use in instead of hasOwnProperty. If is not that expensive and it will work as expected as the objects we're dealing with here are mere data containers, not complex class-objects with inherited properties.

[VladimirTechMan]

@alexandernst Honestly, the performance difference between the native in operator in JS and hasOwnProperty is rather minor in most of the modern browsers, while both of them happen to be dramatically slower when compared to the native key-value access operation followed by the strict comparison with undefined. You can check it yourself here:

https://jsperf.com/compare-different-ways-to-check-whether-an-object-s-pro/17

As an example, running the above test on my systems (OS X and Windows), with the recent public versions of Chrome and Firefox , both the in operator and hasOwnProperty are about 96-98% slower than the direct access to objects' key values with the follow-up comparison to undefined. :-( Interestingly, on MS Edge it is even worse for in, being 99% slower (wow!).

(To note: The only browser where in is more or less on par with the strict comparison of key values against undefined, in my tests, is Safari.)

To me, that sacrifice in performance would be really-really sensitive, honestly. It would be great if the existing browser engines did better job to optimize the in operator. But right now, they do not, alas.

Having said that, again, what's your practical case where you need to check dictionary keys with undefined values on them? (Thanks, for sharing a brief desc about your project, BTW! Appreciate it. And it does look good and interesting! Though, without more details, it would still be difficult for me to figure out, for example, your need to examine undefined values associated with object keys, in your case. I will need more input from you here.)

Let me know. I also keep thinking on my side how to help you better.

[alexandernst]

I know that both operators are rather slow, but I'm trying to make you understand that the comparator, as-is now, just doesn't behave as it should. The current speed is based on a buggy behavior. The correct behavior would decrease the speed, but it will remove the bug. Expecting to keep a buggy behavior in a project just because your particular app makes use of it is not a nice thing to do.

That said, I refuse to believe your application can't circumvent this slowness. Maybe you can create a simple pastebin with the problem and we can think about it?

[VladimirTechMan]

@alexandernst Alright, here is what I suggest – so that both of us can get decent options for our specific purposes. We can change the implementation of $keyin and $nkeyin to your proposed approach, based on the in JS operator. Then, I will introduce two "sibling" operators, called $definedin and $ndefinedin, that use my initial implementation approach. Hopefully the two new names will also better explain the intended behavior of the two new operators (but I'll need to work on documenting them anyways).

Let me know if that sounds reasonable to you. If so, I will make the change later today.

[alexandernst]

That indeed sounds goods. An operator that would return true if a key doesn't exist or if it exists and the value is undefined. Anyways, I'm still on for a review for your particular problem, you got my attention and curiosity!

[VladimirTechMan]

Well, I am equally on for review of your problem, too, @alexandernst! We could Skype / Hangout sometime, maybe. ;-))

In the meantime, I will work on the code changes.

[alexandernst]

Sure thing! Give me a few more days until my baby gets uploaded to github :D 👍