maritz / nohm

node.js object relations mapper (orm) for redis
http://maritz.github.io/nohm/
MIT License
500 stars 64 forks source link

[WIP] Find regexp feature #104

Open sam2x opened 8 years ago

sam2x commented 8 years ago

The find_regexp_feature :

You may still pass a string to your indexed/uniques field, but to trigger the regexp you need to pass an object, with key as 'regexp', your pattern as value :

Model.find({
  email:{
    regexp:'*@domain.*'
  }
}, cb)

Some explanation about logic:

The find function is iterating each property, looking at the property of the key in the Model to define the kind of lookup to apply. I added 3 main things :

1- If the property is an object and has regexp key, i add it to a dedicated list of regexp key. There are two lists of regexp key, the ones who has 'index' property, and the ones who has 'uniques' property. 2- For each list i fetch the appropiate redis prefix, and run keys functions, then i fetch ids of these keys with the appropriate command: smembers or get (because index is a redis set datatype, and unique is a redis string datatype). 3- Once i have fetched all the keys, find function have 4 arrays :

3bis- What i do from there, i use reduce function which lookup for values of each arrays (set/zsetsKeys since regexp are already resolved), and make an intersection of Ids found for each array. Logical 'AND' is still applied as initially for the find function. I wanted to add "operator OR/AND" feature (where find argument is not obj but array of obj with a key 'operator' defined to OR or AND), but in future (not in my needs actually)

ps: Despite my grammar who is wrong (sorry!), there is a typo on regexp error message, i say "object must be {pattern:'regexp'}", pattern should be replaced by "regexp".

maritz commented 8 years ago

Travis was borking, had to close/reopen.

sam2x commented 8 years ago

Hey, spotted broken/useless code (i guess it's when i started to modify the file and trying to figure out how to pass 'regexp'), line 231 :

else if (searches.regexp)
{
  searches = {}
}

Can you remove it ? Else it will create edge case when user has 'regexp' as parameter name.

sam2x commented 7 years ago

Please don't merge the current patch. I grealy improved it due to a sidecase. I need to push my patch to reflect the final change.

fatfatson commented 6 years ago

is it in working? I think the fuzzy search is very necessary!

sam2x commented 6 years ago

Yes it's working, I just didnt clean my code and sync it. Thought nobody was interested about this feature, will do it soon ;)

maritz commented 6 years ago

@sam2x before you do that: I will not pull this into the 0.9.x branch, I think. So it'd have to be re-written in the 2.x branch.

sam2x commented 6 years ago

@maritz Before looking for 2.x code, Is there any importants points/directions to consider like breaking API, major change, current state of 2.x (is that usable right now?) I would like to start using 2.x and submit PR. May also start to check if others redis client works "out of the box" (or with minors modifications).

maritz commented 6 years ago

I've been using v2 in my own small projects, so I'd say it is mostly usable. There are a few things not yet implemented properly in v2, like custom validation files.

There are quite a few breaking changes. The biggest one for you as a contributor: it's Typescript now.

The docs are my main focus right now, I have a branch with most things updated to v2 and a mostly-complete changelog of breaking changes. Haven't pushed it yet, because I'm currently only working on that while in the train where I don't have internet. I'm hoping to have that finished and pushed sometime in the next week.

The tests are all converted to v2, so if in doubt you can look at them.

zzswang commented 5 years ago

Do we already have this feature in v2?

maritz commented 5 years ago

Do we already have this feature in v2?

No. This PR would have to be converted to typescript and included into the current code.

I'm also not sure if I want this in the main code, to be honest. It uses a lot of KEYS commands which is not really something you're supposed to do.

To quote redis.io:

Warning: consider KEYS as a command that should only be used in production environments with extreme care. It may ruin performance when it is executed against large databases. This command is intended for debugging and special operations, such as changing your keyspace layout. Don't use KEYS in your regular application code. If you're looking for a way to find keys in a subset of your keyspace, consider using SCAN or sets.

Implementing this in a better way would probably be a bit more difficult, so making it a different layer seems reasonable to me.

If this is just about having a fuzzy search option, I would recommend an external indexing method. I've previously used https://github.com/tj/reds with good results. More comprehensive full-text search solutions exist but are usually more involved and completely out of scope here.

zzswang commented 5 years ago

Yes, previously we use SCAN to implement regex search.

I'll take a look at reds, thanks.

zzswang commented 5 years ago

@maritz

Any suggestions about how to integrate with external indexing method.

How to execute Nohm's find query within a given ids array ? For supporting pagination.