adding new operators #369
adding new operators #369
Conversation
There was a problem hiding this comment.
Sorry for the long response. It is definitely a shame. To be honest, the main reason I've postponed my review is that features introduced is this PR are non-trivial. For a start, their API must be well-thought-out and compatible with existing API. API proposed in this PR, at least for multivalue select, is not satisfying enough. We also should consider other factors, like crud API in language connectors:
- https://github.com/tarantool/go-tarantool/blob/master/crud
- https://github.com/tarantool/tarantool-python/blob/66e53bca472fec0efacd690ebe6c54c33a7df3f9/tarantool/connection.py#L2132-L2971
- https://github.com/tarantool/cartridge-java/tree/167bd38c92ec3e581a9d087c6e29a0a2c66309de/src/main/java/io/tarantool/driver/core/proxy
New API should either be supported out-of-box or after some well-compatible updates of existing API.
The simplest solution to the question is to use some other operator for multivalue select and do not change the API for new operators.
The other approach to introduce conditions with more than three fields. But this one is badly compatible with tarantool-python and go-tarantool CRUD API (likely cartridge-java too, I'm not familiar with this one).
So you may either implement the "simplest" one or propose a different solution. I'm not sure I will be able to design a better API anytime soon.
We also need to justify new operators. If it is some existing API in other databases, we'll need to check whether the meaning is the same.
The other question is tests. This features definitely should be covered by new tests. You can use existing tests for a reference -- crud is one of the best modules here regarding the quantity of luatest tests.
Both "big" questions can be covered in any order (just keep in mind that you may need to rework tests a bit after possible API change). And both of them requires a lot of resources. It is fine if you won't be able to provide them. It is likely that one day someone from the Tarantool Ecosystem Team will finish the PR. But it is also likely that this one day may be really postponed until Product Team will give its approval.
(Just in case: I will be on vacation for the next two weeks, so I'm not sure I will be able to answer even minor questions for this time.)
There was a problem hiding this comment.
Added a way to search by multiple values:
Sample:crud.select('users', {{'=', 'name', {'Andrey', 'Sergey'}})
Tarantool supports multi-part indexes. For example, let's consider index full_name with two string fields name and surname. In this case, select request will look like this.
crud.select('users', {{'=', 'full_name', {'Andrey', 'Andreev'}})So, with this approach the same API is used for two different cases, which can worsen the debug and potentially break existing multipart key API.
There was a problem hiding this comment.
Tarantool also supports RTREE indexes, which can index arrays as keys. crud doesn't support RTREE as a primary condition, but it seems that it is possible to use it as a secondary condition.
Unfortunately, there are no tests for RTREE now, but it shouldn't be hard to cover this case too.
| @@ -13,6 +13,11 @@ conditions.operators = { | |||
| LE = '<=', | |||
| GT = '>', | |||
| GE = '>=', | |||
| NO = '@<>', | |||
There was a problem hiding this comment.
Is there a reason why operators here start with @ and below they are not?
| BTW = '@>><<', | ||
| OUT = '@<<>>', | ||
| OUTN = '@<<!>>', | ||
| INC = '@>)', | ||
| } | ||
|
|
||
| local tarantool_iter_by_cond_operators = { |
There was a problem hiding this comment.
It seems rather weird that these table isn't updated after adding new operators.
| @@ -75,6 +85,11 @@ local funcs_by_symbols = { | |||
| ['<='] = conditions.funcs.le, | |||
| ['>'] = conditions.funcs.gt, | |||
| ['>='] = conditions.funcs.ge, | |||
| ['<>'] = conditions.funcs.no, | |||
There was a problem hiding this comment.
Does this syntax inspired by some existing database API or not? Not all of these are intuitive.
There was a problem hiding this comment.
crud.select('users, {{ '>><<', 'activated_at', { dateFrom, dateTo } } }, { fields = { '_id' }})
Again, here we have a clash with multipart key API.
| @@ -469,6 +553,15 @@ local function gen_filter_code(filter_conditions) | |||
| } | |||
There was a problem hiding this comment.
Such new feature definitely requires
- tests,
- documentation update,
- changelog update.
| local function format_eq(cond) | ||
| local cond_strings = {} | ||
| local values_opts = cond.values_opts or {} | ||
|
|
||
| if #cond.values == 1 then |
There was a problem hiding this comment.
Multivalue equity select and range operators are different features, so it's better to implement them in separate commits (or maybe even separate PRs).
|
I'll also need to fix the CI to run on external PRs. |
Added a way to search by multiple values:
Sample:
crud.select('users', {{'=', 'name', {'Andrey', 'Sergey'}}}, {fields = {'_id', 'name'}});
Added new operators such as:
'<>' - not
'>><<' - between
'<<>>' - out of
'<<!>>' - out of or null
'>)' - include
Sample:
crud.select('users, {{ '>><<', 'activated_at', { dateFrom, dateTo } } }, { fields = { '_id' }})