-
Notifications
You must be signed in to change notification settings - Fork 482
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
Comparator helpers keyin and nkeyin don't work as expected #362
Comments
@alexandernst As the original author of The intended usages of those two operators is to have an alternative to the original Thus, please, detail what you wanted (expected) to get. |
@VladimirTechMan Ah, so it was designed that way. I'd say this shouldn't happen, as IMHO this should be done with |
@VladimirTechMan @techfort @obeliskos If you agree with my previos comment I can make a PR. |
@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 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 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 |
@VladimirTechMan Indeed, 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. |
@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 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 |
@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 |
Ah, the |
Btw, I have given this a second thought. We could use |
@alexandernst Honestly, the performance difference between the native 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 (To note: The only browser where 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 Having said that, again, what's your practical case where you need to check dictionary keys with Let me know. I also keep thinking on my side how to help you better. |
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? |
@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 Let me know if that sounds reasonable to you. If so, I will make the change later today. |
That indeed sounds goods. An operator that would return |
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. |
Sure thing! Give me a few more days until my baby gets uploaded to github :D 👍 |
Good. Mine will (possibly) be on GitHub a little bit later than just in a couple of days. More work that I need to do, even for a beta / preview release. But it won't be too far from now anyway (fingers crossed). :-) |
👍 😃 |
Will submit a PR to fix it. (I'm creating this issue just as a reminder to myself)
The text was updated successfully, but these errors were encountered: