Skip to content

Commit e4d8fa0

Browse files
dor-forerlerman25kei-nannafrafraz-mon
authored
Fix Empty Numeric Value - [MOD-7244] (#5566)
* Adding numeric check * changes * change to each one * MOD-6786 Fix search on larger then 128 terms (#5524) * Move length slicing to NOMODIFY if * add py test * fix slicing * fix test * fix text skip cluster * Adding comments * Update test_issues - skip cluster * MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528) * * initial commit * * simplify the fix * * revert to old code to solve edge case * Load config params for Redis 8.0-m03 (#5538) * Load config params for Redis v7.9.226 * Add step to get latest unreleased redis tag * Remove commented-out step `Get Latest Release Tag with Prefix` * Revert: task-get-latest-tag.yml * MOD-8601: Fix error message for LOAD (#5531) * Enhance error message for LOAD * Fix error message * Address review * Fix flakiness in a test (#5541) * fix flakiness * revert whitespace change * Fix Max Frequency Misscalculation - [MOD-8158] (#5553) * fix unrelated test * add a failing test * fix issue * revert whitespace change from test_vecsim.py * revert whitespace change in test_issues.py * Fix APPLY/FILTER parser - [MOD-7804] (#5520) * fix order of operations * minor improvements to the lexer * improve functions parsing * optimize "NOT" logic and perform arithmetic operations immediately * fix flow tests * fix grammar optimization * changed function API * added a test * minor fix * fix precedence * minor improvement * reorder rule and fix leak * another fix * added test * more tests for a better coverage * improved test * fix assertion * review fixes * address code review * added comments * remove unncessary * Added tests for legacy filter empty * Adding numeric check * changes * change to each one * remove unncessary * Added tests for legacy filter empty * * Change the order of params * Add support in GEOFILTER * Forgot one file * * Changed to AC_GetString with no advance * Added comment * change the string check * PR changes * Changes * push the test * change style --------- Co-authored-by: lerman25 <58445352+lerman25@users.noreply.github.com> Co-authored-by: kei-nan <jonathan.keinan@redis.com> Co-authored-by: nafraf <nafraf@users.noreply.github.com> Co-authored-by: Raz Monsonego <74051729+raz-mon@users.noreply.github.com> Co-authored-by: GuyAv46 <47632673+GuyAv46@users.noreply.github.com>
1 parent f981725 commit e4d8fa0

16 files changed

Lines changed: 147 additions & 51 deletions

‎src/aggregate/aggregate_request.c‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -454,18 +454,18 @@ static int parseSortby(PLN_ArrangeStep *arng, ArgsCursor *ac, QueryError *status
454454
return REDISMODULE_ERR;
455455
}
456456

457-
static int parseQueryLegacyArgs(ArgsCursor *ac, RSSearchOptions *options, QueryError *status) {
457+
static int parseQueryLegacyArgs(ArgsCursor *ac, RSSearchOptions *options, bool *hasEmptyFilterValue, QueryError *status) {
458458
if (AC_AdvanceIfMatch(ac, "FILTER")) {
459459
// Numeric filter
460460
NumericFilter **curpp = array_ensure_tail(&options->legacy.filters, NumericFilter *);
461-
*curpp = NumericFilter_LegacyParse(ac, status);
461+
*curpp = NumericFilter_LegacyParse(ac, hasEmptyFilterValue, status);
462462
if (!*curpp) {
463463
return ARG_ERROR;
464464
}
465465
} else if (AC_AdvanceIfMatch(ac, "GEOFILTER")) {
466466
GeoFilter *cur_gf = rm_new(*cur_gf);
467467
array_ensure_append_1(options->legacy.geo_filters, cur_gf);
468-
if (GeoFilter_LegacyParse(cur_gf, ac, status) != REDISMODULE_OK) {
468+
if (GeoFilter_LegacyParse(cur_gf, ac, hasEmptyFilterValue, status) != REDISMODULE_OK) {
469469
return ARG_ERROR;
470470
}
471471
} else {
@@ -509,6 +509,7 @@ static int parseQueryArgs(ArgsCursor *ac, AREQ *req, RSSearchOptions *searchOpts
509509

510510
req->reqflags |= QEXEC_FORMAT_DEFAULT;
511511
bool optimization_specified = false;
512+
bool hasEmptyFilterValue = false;
512513
while (!AC_IsAtEnd(ac)) {
513514
ACArgSpec *errSpec = NULL;
514515
int rv = AC_ParseArgSpec(ac, querySpecs, &errSpec);
@@ -546,7 +547,7 @@ static int parseQueryArgs(ArgsCursor *ac, AREQ *req, RSSearchOptions *searchOpts
546547
req->reqflags |= QEXEC_F_SEND_HIGHLIGHT;
547548

548549
} else if ((req->reqflags & QEXEC_F_IS_SEARCH) &&
549-
((rv = parseQueryLegacyArgs(ac, searchOpts, status)) != ARG_UNKNOWN)) {
550+
((rv = parseQueryLegacyArgs(ac, searchOpts, &hasEmptyFilterValue, status)) != ARG_UNKNOWN)) {
550551
if (rv == ARG_ERROR) {
551552
return REDISMODULE_ERR;
552553
}
@@ -568,6 +569,12 @@ static int parseQueryArgs(ArgsCursor *ac, AREQ *req, RSSearchOptions *searchOpts
568569
}
569570
}
570571

572+
// In dialect 2, we require a non empty numeric filter
573+
if (req->reqConfig.dialectVersion >= 2 && hasEmptyFilterValue){
574+
QERR_MKBADARGS_FMT(status, "Numeric/Geo filter value/s cannot be empty");
575+
return REDISMODULE_ERR;
576+
}
577+
571578
if (!optimization_specified && req->reqConfig.dialectVersion >= 4) {
572579
// If optimize was not enabled/disabled explicitly, enable it by default starting with dialect 4
573580
req->reqflags |= QEXEC_OPTIMIZE;
@@ -1126,15 +1133,17 @@ int AREQ_ApplyContext(AREQ *req, RedisSearchCtx *sctx, QueryError *status) {
11261133
SetSearchCtx(sctx, req);
11271134
QueryAST *ast = &req->ast;
11281135

1129-
int rv = QAST_Parse(ast, sctx, opts, req->query, strlen(req->query), req->reqConfig.dialectVersion, status);
1136+
unsigned long dialectVersion = req->reqConfig.dialectVersion;
1137+
1138+
int rv = QAST_Parse(ast, sctx, opts, req->query, strlen(req->query), dialectVersion, status);
11301139
if (rv != REDISMODULE_OK) {
11311140
return REDISMODULE_ERR;
11321141
}
11331142

1134-
if (QAST_EvalParams(ast, opts, status) != REDISMODULE_OK) {
1143+
if (QAST_EvalParams(ast, opts, dialectVersion, status) != REDISMODULE_OK) {
11351144
return REDISMODULE_ERR;
11361145
}
1137-
if (applyGlobalFilters(opts, ast, sctx, req->reqConfig.dialectVersion, status) != REDISMODULE_OK) {
1146+
if (applyGlobalFilters(opts, ast, sctx, dialectVersion, status) != REDISMODULE_OK) {
11381147
return REDISMODULE_ERR;
11391148
}
11401149

‎src/coord/dist_aggregate.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -706,7 +706,7 @@ void RSExecDistAggregate(RedisModuleCtx *ctx, RedisModuleString **argv, int argc
706706
// Check if we have KNN in the query string, and if so, parse the query string to see if it is
707707
// a KNN section in the query. IN that case, we treat this as a SORTBY+LIMIT step.
708708
if(strcasestr(r->query, "KNN")) {
709-
knnCtx = prepareOptionalTopKCase(r->query, argv, argc, &status);
709+
knnCtx = prepareOptionalTopKCase(r->query, argv, argc, dialect, &status);
710710
if (QueryError_HasError(&status)) {
711711
goto err;
712712
}

‎src/geo_index.c‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,20 @@
1414

1515
static double extractUnitFactor(GeoDistance unit);
1616

17+
static void CheckAndSetEmptyFilterValue(ArgsCursor *ac, bool *hasEmptyFilterValue) {
18+
const char *val;
19+
20+
int rv = AC_GetString(ac, &val, NULL, AC_F_NOADVANCE);
21+
if (rv == AC_OK && !(*val)) {
22+
*hasEmptyFilterValue = true;
23+
}
24+
}
25+
1726
/* Parse a geo filter from redis arguments. We assume the filter args start at argv[0], and FILTER
1827
* is not passed to us.
1928
* The GEO filter syntax is (FILTER) <property> LONG LAT DIST m|km|ft|mi
2029
* Returns REDISMODUEL_OK or ERR */
21-
int GeoFilter_LegacyParse(GeoFilter *gf, ArgsCursor *ac, QueryError *status) {
30+
int GeoFilter_LegacyParse(GeoFilter *gf, ArgsCursor *ac, bool *hasEmptyFilterValue, QueryError *status) {
2231
*gf = (GeoFilter){0};
2332

2433
if (AC_NumRemaining(ac) < 5) {
@@ -32,20 +41,33 @@ int GeoFilter_LegacyParse(GeoFilter *gf, ArgsCursor *ac, QueryError *status) {
3241
QERR_MKBADARGS_AC(status, "<geo property>", rv);
3342
return REDISMODULE_ERR;
3443
}
35-
if ((rv = AC_GetDouble(ac, &gf->lon, 0) != AC_OK)) {
44+
45+
if ((rv = AC_GetDouble(ac, &gf->lon, AC_F_NOADVANCE) != AC_OK)) {
3646
QERR_MKBADARGS_AC(status, "<lon>", rv);
3747
return REDISMODULE_ERR;
3848
}
49+
if (gf->lon == 0) {
50+
CheckAndSetEmptyFilterValue(ac, hasEmptyFilterValue);
51+
}
52+
AC_Advance(ac);
3953

40-
if ((rv = AC_GetDouble(ac, &gf->lat, 0)) != AC_OK) {
54+
if ((rv = AC_GetDouble(ac, &gf->lat, AC_F_NOADVANCE)) != AC_OK) {
4155
QERR_MKBADARGS_AC(status, "<lat>", rv);
4256
return REDISMODULE_ERR;
4357
}
58+
if (gf->lat == 0) {
59+
CheckAndSetEmptyFilterValue(ac, hasEmptyFilterValue);
60+
}
61+
AC_Advance(ac);
4462

45-
if ((rv = AC_GetDouble(ac, &gf->radius, 0)) != AC_OK) {
63+
if ((rv = AC_GetDouble(ac, &gf->radius, AC_F_NOADVANCE)) != AC_OK) {
4664
QERR_MKBADARGS_AC(status, "<radius>", rv);
4765
return REDISMODULE_ERR;
4866
}
67+
if (gf->radius == 0) {
68+
CheckAndSetEmptyFilterValue(ac, hasEmptyFilterValue);
69+
}
70+
AC_Advance(ac);
4971

5072
const char *unitstr = AC_GetStringNC(ac, NULL);
5173
if ((gf->unitType = GeoDistance_Parse(unitstr)) == GEO_DISTANCE_INVALID) {

‎src/geo_index.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ GeoDistance GeoDistance_Parse_Buffer(const char *s, size_t len);
6161
int GeoFilter_Validate(const GeoFilter *gf, QueryError *status);
6262

6363
/* Parse a geo filter from redis arguments. We assume the filter args start at argv[0] */
64-
int GeoFilter_LegacyParse(GeoFilter *gf, ArgsCursor *ac, QueryError *status);
64+
int GeoFilter_LegacyParse(GeoFilter *gf, ArgsCursor *ac, bool *hasEmptyFilterValue, QueryError *status);
6565
void GeoFilter_Free(GeoFilter *gf);
6666
IndexIterator *NewGeoRangeIterator(const RedisSearchCtx *ctx, const GeoFilter *gf, ConcurrentSearchCtx *csx, IteratorsConfig *config);
6767

‎src/module.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1638,7 +1638,7 @@ void setKNNSpecialCase(searchRequestCtx *req, specialCaseCtx *knn_ctx) {
16381638
// Prepare a TOPK special case, return a context with the required KNN fields if query is
16391639
// valid and contains KNN section, NULL otherwise (and set proper error in *status* if error
16401640
// was found).
1641-
specialCaseCtx *prepareOptionalTopKCase(const char *query_string, RedisModuleString **argv, int argc,
1641+
specialCaseCtx *prepareOptionalTopKCase(const char *query_string, RedisModuleString **argv, int argc, uint dialectVersion,
16421642
QueryError *status) {
16431643

16441644
// First, parse the query params if exists, to set the params in the query parser ctx.
@@ -1677,7 +1677,7 @@ specialCaseCtx *prepareOptionalTopKCase(const char *query_string, RedisModuleStr
16771677
goto cleanup;
16781678
}
16791679
if (QueryNode_NumParams(queryNode) > 0) {
1680-
int ret = QueryNode_EvalParamsCommon(params, queryNode, status);
1680+
int ret = QueryNode_EvalParamsCommon(params, queryNode, dialectVersion, status);
16811681
if (ret != REDISMODULE_OK || QueryError_GetCode(status) != QUERY_OK) {
16821682
// Params evaluation failed.
16831683
goto cleanup;
@@ -1811,7 +1811,7 @@ searchRequestCtx *rscParseRequest(RedisModuleString **argv, int argc, QueryError
18111811
if(dialect >= 2) {
18121812
// Note: currently there is only one single case. For extending those cases we should use a trie here.
18131813
if(strcasestr(req->queryString, "KNN")) {
1814-
specialCaseCtx *knnCtx = prepareOptionalTopKCase(req->queryString, argv, argc, status);
1814+
specialCaseCtx *knnCtx = prepareOptionalTopKCase(req->queryString, argv, argc, dialect, status);
18151815
if (QueryError_HasError(status)) {
18161816
searchRequestCtx_Free(req);
18171817
return NULL;

‎src/module.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ typedef struct {
119119
void *reducer;
120120
} searchRequestCtx;
121121

122-
specialCaseCtx *prepareOptionalTopKCase(const char *query_string, RedisModuleString **argv, int argc,
122+
specialCaseCtx *prepareOptionalTopKCase(const char *query_string, RedisModuleString **argv, int argc, uint dialectVersion,
123123
QueryError *status);
124124

125125
void SpecialCaseCtx_Free(specialCaseCtx* ctx);

‎src/numeric_filter.c‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ int parseDoubleRange(const char *s, bool *inclusive, double *target, int isMin,
5656
* Returns a numeric filter on success, NULL if there was a problem with the
5757
* arguments
5858
*/
59-
NumericFilter *NumericFilter_LegacyParse(ArgsCursor *ac, QueryError *status) {
59+
NumericFilter *NumericFilter_LegacyParse(ArgsCursor *ac, bool *hasEmptyFilterValue, QueryError *status) {
6060
if (AC_NumRemaining(ac) < 3) {
6161
QERR_MKBADARGS_FMT(status, "FILTER requires 3 arguments");
6262
return NULL;
@@ -74,11 +74,18 @@ NumericFilter *NumericFilter_LegacyParse(ArgsCursor *ac, QueryError *status) {
7474

7575
// Parse the min range
7676
const char *s = AC_GetStringNC(ac, NULL);
77+
if (!*s) {
78+
*hasEmptyFilterValue = true;
79+
}
7780
if (parseDoubleRange(s, &nf->inclusiveMin, &nf->min, 1, 1, status) != REDISMODULE_OK) {
7881
NumericFilter_Free(nf);
7982
return NULL;
8083
}
84+
8185
s = AC_GetStringNC(ac, NULL);
86+
if (!*s) {
87+
*hasEmptyFilterValue = true;
88+
}
8289
if (parseDoubleRange(s, &nf->inclusiveMax, &nf->max, 0, 1, status) != REDISMODULE_OK) {
8390
NumericFilter_Free(nf);
8491
return NULL;

‎src/numeric_filter.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ typedef struct NumericFilter {
3535

3636
NumericFilter *NewNumericFilter(double min, double max, int inclusiveMin, int inclusiveMax,
3737
bool asc);
38-
NumericFilter *NumericFilter_LegacyParse(ArgsCursor *ac, QueryError *status);
38+
NumericFilter *NumericFilter_LegacyParse(ArgsCursor *ac, bool *hasEmptyFilterValue, QueryError *status);
3939
int NumericFilter_EvalParams(dict *params, QueryNode *node, QueryError *status);
4040
void NumericFilter_Free(NumericFilter *nf);
4141

‎src/query.c‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1568,19 +1568,19 @@ int QAST_Expand(QueryAST *q, const char *expander, RSSearchOptions *opts, RedisS
15681568
return REDISMODULE_OK;
15691569
}
15701570

1571-
int QAST_EvalParams(QueryAST *q, RSSearchOptions *opts, QueryError *status) {
1571+
int QAST_EvalParams(QueryAST *q, RSSearchOptions *opts, unsigned int dialectVersion, QueryError *status) {
15721572
if (!q || !q->root || q->numParams == 0)
15731573
return REDISMODULE_OK;
1574-
QueryNode_EvalParams(opts->params, q->root, status);
1574+
QueryNode_EvalParams(opts->params, q->root, dialectVersion, status);
15751575
return REDISMODULE_OK;
15761576
}
15771577

1578-
int QueryNode_EvalParams(dict *params, QueryNode *n, QueryError *status) {
1578+
int QueryNode_EvalParams(dict *params, QueryNode *n, unsigned int dialectVersion, QueryError *status) {
15791579
int withChildren = 1;
15801580
int res = REDISMODULE_OK;
15811581
switch(n->type) {
15821582
case QN_VECTOR:
1583-
res = VectorQuery_EvalParams(params, n, status);
1583+
res = VectorQuery_EvalParams(params, n, dialectVersion, status);
15841584
break;
15851585
case QN_GEO:
15861586
case QN_TOKEN:
@@ -1596,7 +1596,7 @@ int QueryNode_EvalParams(dict *params, QueryNode *n, QueryError *status) {
15961596
case QN_WILDCARD:
15971597
case QN_WILDCARD_QUERY:
15981598
case QN_GEOMETRY:
1599-
res = QueryNode_EvalParamsCommon(params, n, status);
1599+
res = QueryNode_EvalParamsCommon(params, n, dialectVersion, status);
16001600
break;
16011601
case QN_UNION:
16021602
// no immediately owned params to resolve
@@ -1610,7 +1610,7 @@ int QueryNode_EvalParams(dict *params, QueryNode *n, QueryError *status) {
16101610
// Handle children
16111611
if (withChildren && res == REDISMODULE_OK) {
16121612
for (size_t ii = 0; ii < QueryNode_NumChildren(n); ++ii) {
1613-
res = QueryNode_EvalParams(params, n->children[ii], status);
1613+
res = QueryNode_EvalParams(params, n->children[ii], dialectVersion, status);
16141614
if (res == REDISMODULE_ERR)
16151615
break;
16161616
}
@@ -1790,10 +1790,10 @@ void QueryNode_ClearChildren(QueryNode *n, int shouldFree) {
17901790
}
17911791
}
17921792

1793-
int QueryNode_EvalParamsCommon(dict *params, QueryNode *node, QueryError *status) {
1793+
int QueryNode_EvalParamsCommon(dict *params, QueryNode *node, unsigned int dialectVersion, QueryError *status) {
17941794
if (node->params) {
17951795
for (size_t i = 0; i < QueryNode_NumParams(node); i++) {
1796-
int res = QueryParam_Resolve(&node->params[i], params, status);
1796+
int res = QueryParam_Resolve(&node->params[i], params, dialectVersion, status);
17971797
if (res < 0)
17981798
return REDISMODULE_ERR;
17991799
// If parameter's value is a number, don't expand the node.

‎src/query.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,8 @@ IndexIterator *QAST_Iterate(QueryAST *ast, const RSSearchOptions *options,
124124
int QAST_Expand(QueryAST *q, const char *expander, RSSearchOptions *opts, RedisSearchCtx *sctx,
125125
QueryError *status);
126126

127-
int QAST_EvalParams(QueryAST *q, RSSearchOptions *opts, QueryError *status);
128-
int QueryNode_EvalParams(dict *params, QueryNode *node, QueryError *status);
127+
int QAST_EvalParams(QueryAST *q, RSSearchOptions *opts, unsigned int dialectVersion, QueryError *status);
128+
int QueryNode_EvalParams(dict *params, QueryNode *node, unsigned int dialectVersion, QueryError *status);
129129

130130
int QAST_CheckIsValid(QueryAST *q, IndexSpec *spec, RSSearchOptions *opts, QueryError *status);
131131

0 commit comments

Comments
 (0)