Distance function for aggregation APPLY #1246
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1246 +/- ##
==========================================
- Coverage 81.74% 81.71% -0.03%
==========================================
Files 143 144 +1
Lines 20674 20717 +43
==========================================
+ Hits 16899 16928 +29
- Misses 3775 3789 +14
Continue to review full report at Codecov.
|
src/pytest/test_aggregate.py
Outdated
|
|
||
| env.expect('ft.aggregate', 'idx', 'hilton', | ||
| 'LOAD', '1', '@location', | ||
| 'APPLY', 'distance(@location,-0.15036,51.50566)', 'AS', 'distance', |
There was a problem hiding this comment.
I think its better that the function will get an equivalent arguments type and not (string, num, num). So I think something like distance(string, string) is better (we will try to parse both string as geo). This way we can also check the distance between two fields in the document for example..
There was a problem hiding this comment.
I wanted it this way but the parser automatically treats ',' as separator and therefore splits the string.
We can have another option that will receive two field names.
I should probably add an option for type (meter, feet...). This will make the pressing annoying (combined with the above options). Any idea how to streamline this?
There was a problem hiding this comment.
You mean if I write something like this : func("a,b") it gets it as 2 arguments? This is a bug by itself..
There was a problem hiding this comment.
in agg/expr/parser.y
arglist(A) ::= . [ARGLIST] { A = RS_NewArgList(NULL); }
arglist(A) ::= expr(B) . [ARGLIST] { A = RS_NewArgList(B); }
arglist(A) ::= arglist(B) COMMA expr(C) . [ARGLIST] {
A = RSArgList_Append(B, C);
}
We use it to split args
https://oss.redislabs.com/redisearch/Aggregations/#list_of_string_apply_functions
There was a problem hiding this comment.
@MeirShpilraien what do you mean by two strings?
There was a problem hiding this comment.
Currently, the parser splits arguments with , and therefore, an input distance(@field,1.2,-3.4) will be parsed as (string, num, num).
We can decide on another char for splitting between long and lat but it won't be intuitive.
|
any update on this? |
|
related to #1442 |
|
@MeirShpilraien fixed. |
| | geodistance(field,"lon,lat") | Return distance in meters. | `geodistance(@field,"1.2,-3.4")` | | ||
| | geodistance(lon,lat,field) | Return distance in meters. | `geodistance(1.2,-3.4,@field)` | | ||
| | geodistance("lon,lat",field) | Return distance in meters. | `geodistance("1.2,-3.4",@field)` | | ||
| | geodistance("lon,lat","lon,lat")| Return distance in meters. | `geodistance("1.2,-3.4","5.6,-7.8")` | |
There was a problem hiding this comment.
@ashtul needs the one with (lon1, lat1, lon2, lat2)
* geo files * Apply distance for geo * fix per meir review * docs Co-authored-by: Guy Korland <gkorland@gmail.com>
* Deprecate ft.add, ft.del, ft.drop * Fix tests * review fixes * some more docs fixes * more docs fixes * some more fixes * CircleCI build logs and readies update (#1463) * Explanation for spellcheck score (#1429) * [DOC] explain ft.spellcheck scoring * add assert * fix assert location * if instead of assert * Fix expire during query (#1437) * Distance function for aggregation APPLY (#1246) * geo files * Apply distance for geo * fix per meir review * docs Co-authored-by: Guy Korland <gkorland@gmail.com> * Search2 docs (#1383) * Update index.md * Update Quick_Start.md * Update Commands.md Co-authored-by: Ariel Shtul <ashtul@gmail.com> * fix flaky test * expanding on HSET * clear * extra docs * temporary * geo Co-authored-by: Rafi Einstein <rafi@redislabs.com> Co-authored-by: Ariel Shtul <ashtul@gmail.com> Co-authored-by: Guy Korland <gkorland@gmail.com>
fix #916