Fix Empty Numeric Value - [MOD-7244] by dor-forer · Pull Request #5566 · RediSearch/RediSearch

dor-forer · 2025-01-26T08:26:59Z

Describe the changes in the pull request

Added check for empty numeric string before legacy SEARCH FILTER (only if dialect version is greater than 2).
Added check for an empty string when evaluating pramas (only if dialect version is greater than 2).

Main objects this PR modified

An empty string param doesn't evaluate as 0.

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

CLAassistant · 2025-01-26T08:27:04Z

All committers have signed the CLA.

codecov · 2025-01-26T08:49:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.28%. Comparing base (eb14b95) to head (5a5f246).
Report is 3 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5566   +/-   ##
=======================================
  Coverage   87.28%   87.28%           
=======================================
  Files         196      196           
  Lines       35267    35292   +25     
=======================================
+ Hits        30783    30805   +22     
- Misses       4484     4487    +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* Move length slicing to NOMODIFY if * add py test * fix slicing * fix test * fix text skip cluster * Adding comments * Update test_issues - skip cluster

* * initial commit * * simplify the fix * * revert to old code to solve edge case

* 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

* Enhance error message for LOAD * Fix error message * Address review

* fix flakiness * revert whitespace change

* fix unrelated test * add a failing test * fix issue * revert whitespace change from test_vecsim.py * revert whitespace change in test_issues.py

* 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

GuyAv46 · 2025-01-26T13:56:51Z

src/aggregate/aggregate_request.c

      return REDISMODULE_ERR;
+    } else if (strcmp(s, "*")) {
+      QERR_MKBADARGS_FMT(status, "Bad arguments for LOAD: Expected number of fields or `*`");
+      return REDISMODULE_ERR;


What was changed here? Is it relevant for the PR?

src/aggregate/aggregate_request.c

src/query.c

src/query_node.h

src/query_param.c

tests/pytests/test_empty.py

…RediSearch into dor-fix-parse-double

* Add support in GEOFILTER

src/geo_index.c

src/numeric_filter.c

src/query_param.c

* Added comment * change the string check

raz-mon

Nicely done.
See a few comments

src/aggregate/aggregate_request.c

src/query_param.c

raz-mon · 2025-01-28T16:13:28Z

src/geo_index.c

    return REDISMODULE_ERR;
  }
+
+  CheckAndSetEmptyFilterValue(ac, hasEmptyFilterValue);


Consider putting these checks after the parsing (i.e., 4 lines below), and conditioning it on the value of the argument. I.e., if it is not 0, we don't need this extra parsing.

The issue is that AC_GetDouble already advances the curser so the check is not relevant anymore.
Do you think I should pass the AC_F_NOADVANCE to AC_GetDouble, and advance the curser afterward?

Yeah that sounds good, since most of the times this won't be the case and we'll just advance the cursor by 1.

src/module.c

raz-mon

LGTM 👍
Small style comment

src/geo_index.c

* 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 <[email protected]> Co-authored-by: kei-nan <[email protected]> Co-authored-by: nafraf <[email protected]> Co-authored-by: Raz Monsonego <[email protected]> Co-authored-by: GuyAv46 <[email protected]> (cherry picked from commit e4d8fa0)

redisearch-backport-pull-request · 2025-01-30T12:14:22Z

Successfully created backport PR for 8.0:

[8.0] Fix Empty Numeric Value - [MOD-7244] #5580

* 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 <[email protected]> Co-authored-by: kei-nan <[email protected]> Co-authored-by: nafraf <[email protected]> Co-authored-by: Raz Monsonego <[email protected]> Co-authored-by: GuyAv46 <[email protected]> (cherry picked from commit e4d8fa0) * Change python to fit python3.7 --------- Co-authored-by: dor-forer <[email protected]>

dor-forer added 3 commits January 22, 2025 16:40

Adding numeric check

9b0a2b2

changes

cc54c8a

change to each one

aed0ba3

dor-forer requested review from GuyAv46 and alonre24 January 26, 2025 08:26

dor-forer added backport 2.10 backport 2.8 backport 2.6 labels Jan 26, 2025

lerman25 and others added 9 commits January 26, 2025 12:59

MOD-6786 Fix search on larger then 128 terms (#5524)

92ac7dc

* 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)

cf2a566

* * initial commit * * simplify the fix * * revert to old code to solve edge case

Load config params for Redis 8.0-m03 (#5538)

b9f6f28

* 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)

1170c1c

* Enhance error message for LOAD * Fix error message * Address review

Fix flakiness in a test (#5541)

68b8159

* fix flakiness * revert whitespace change

Fix Max Frequency Misscalculation - [MOD-8158] (#5553)

c57616e

* fix unrelated test * add a failing test * fix issue * revert whitespace change from test_vecsim.py * revert whitespace change in test_issues.py

remove unncessary

59c48bd

Added tests for legacy filter empty

29adfee

GuyAv46 reviewed Jan 26, 2025

View reviewed changes

alonre24 requested review from raz-mon and removed request for alonre24 January 26, 2025 14:34

dor-forer added 8 commits January 26, 2025 16:44

Adding numeric check

509d2c9

changes

da3bea3

change to each one

3c556a5

remove unncessary

f56b93a

Added tests for legacy filter empty

e26e173

Merge branch 'dor-fix-parse-double' of https://github.com/RediSearch/…

e785ecf

…RediSearch into dor-fix-parse-double

* Change the order of params

7f9fcbd

* Add support in GEOFILTER

Forgot one file

c3f89c9

dor-forer requested a review from GuyAv46 January 27, 2025 09:30

GuyAv46 reviewed Jan 27, 2025

View reviewed changes

src/geo_index.c Outdated Show resolved Hide resolved

src/numeric_filter.c Outdated Show resolved Hide resolved

src/numeric_filter.c Outdated Show resolved Hide resolved

src/query_param.c Show resolved Hide resolved

* Changed to AC_GetString with no advance

c523266

* Added comment * change the string check

dor-forer requested a review from GuyAv46 January 28, 2025 07:24

raz-mon reviewed Jan 28, 2025

View reviewed changes

dor-forer added 3 commits January 29, 2025 10:38

PR changes

8d08fc3

Changes

b45e317

push the test

556bece

dor-forer requested a review from raz-mon January 29, 2025 13:51

raz-mon reviewed Jan 29, 2025

View reviewed changes

src/geo_index.c Outdated Show resolved Hide resolved

change style

5a5f246

GuyAv46 approved these changes Jan 29, 2025

View reviewed changes

raz-mon approved these changes Jan 29, 2025

View reviewed changes

dor-forer added backport 8.0 and removed backport 2.8 backport 2.6 backport 2.10 labels Jan 30, 2025

dor-forer added this pull request to the merge queue Jan 30, 2025

Merged via the queue into master with commit e4d8fa0 Jan 30, 2025
13 checks passed

dor-forer deleted the dor-fix-parse-double branch January 30, 2025 12:14

redisearch-backport-pull-request bot mentioned this pull request Jan 30, 2025

[8.0] Fix Empty Numeric Value - [MOD-7244] #5580

Merged

Conversation

dor-forer commented Jan 26, 2025

Describe the changes in the pull request

Main objects this PR modified

Mark if applicable

Uh oh!

CLAassistant commented Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

GuyAv46 Jan 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raz-mon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

raz-mon Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

dor-forer Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

raz-mon Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raz-mon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Jan 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CLAassistant commented Jan 26, 2025 •

edited

Loading

codecov bot commented Jan 26, 2025 •

edited

Loading