MOD-6750 Fix numeric range syntax #4505

nafraf · 2024-03-05T18:52:36Z

Description
This PR includes the following changes, which were included in parser v3 DIALECT 5:

Allow signed params (also negative), e.g., [-$param1 +$param1]
Allow signed exponent (also positive), e.g., [1 20e+5]
Allow case sensitive infinity (also negative), e.g., [-INF Inf]

Additional changes:

FILTER now also accepts inf (without sign) and not only +inf or -inf.
FILTER now allows case sensitive infinity. e.g. filter score -INF Inf

This PR requires:
#4604 which creates the parser-v3

Not included in this PR:

Disallow an exclusive (open ended) numeric, e.g., [10 (((20]
(should only have a single leading open parenthesis for “exclusive” number)
will be done in MOD-6623: Querying NUMERIC with exact matching #4486
Fail on range with 2 exclusive identical values
[(10 (10] - Breaking - keep current behavior - it means a range with no values - can
[(10]- Breaking
will be done in MOD-6623: Querying NUMERIC with exact matching #4486

Which issues this PR fixes

MOD-6750

Main objects this PR modified

parser v3

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

codecov · 2024-03-06T16:47:26Z

Codecov Report

Attention: Patch coverage is 81.83486% with 99 lines in your changes are missing coverage. Please review.

Project coverage is 85.94%. Comparing base (1466c3e) to head (e167fc1).

Files	Patch %	Lines
src/query_parser/v3/lexer.c	81.17%	99 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4505      +/-   ##
==========================================
+ Coverage   85.80%   85.94%   +0.14%     
==========================================
  Files         188      190       +2     
  Lines       33107    34435    +1328     
==========================================
+ Hits        28406    29595    +1189     
- Misses       4701     4840     +139

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

oshadmi

Looks good 👍🏼 Few comments

tests/pytests/test_numbers.py

tests/pytests/test_search_params.py

oshadmi

Few more comments

oshadmi · 2024-03-09T21:08:06Z

src/numeric_filter.c

+                      int sign, QueryError *status) {
+  if (isMin && (
+        (sign == 1 && !strcasecmp(s, "-inf")) ||
+        (sign == -1 && (!strcasecmp(s, "+inf") || !strcasecmp(s, "inf"))))) {


Consider avoiding 2 library calls

Suggested change

(sign == -1 && (!strcasecmp(s, "+inf") || !strcasecmp(s, "inf"))))) {

(sign == -1 && !strcasecmp((*s == '+' ? s + 1 : s), "inf")))) {

Done. Thanks.

oshadmi · 2024-03-09T21:08:54Z

src/numeric_filter.c

    *target = NF_NEGATIVE_INFINITY;
    return REDISMODULE_OK;
-  } else if (!isMin && !strcasecmp(s, "+inf")) {
+  } else if (!isMin && (
+        (sign == 1 && (!strcasecmp(s, "+inf") || !strcasecmp(s, "inf"))) ||


Consider avoiding 2 library calls

Suggested change

(sign == 1 && (!strcasecmp(s, "+inf") || !strcasecmp(s, "inf"))) ||

(sign == 1 && !strcasecmp((*s == '+' ? s + 1 : s), "inf")) ||

Done. Thanks.

oshadmi · 2024-03-09T21:17:19Z

src/numeric_filter.c

+  if(sign == -1) {
+    *target = -(*target);
+  }


Should move to after the following error check

tests/pytests/test.py

tests/pytests/test_numbers.py

src/numeric_filter.c

oshadmi

👍🏼 😎

This reverts commit 6c3ec77.

raz-mon

Nice!
See few comments

tests/pytests/test.py

tests/pytests/test_numbers.py

tests/pytests/test_search_params.py

…raf_fix-num-range-syntax

raz-mon

Looks good.
See 2 small comments.

raz-mon · 2024-05-09T11:25:18Z

tests/pytests/test_search_params.py

-    env = Env(moduleArgs = 'DEFAULT_DIALECT 2')
+    env = Env(moduleArgs = 'DEFAULT_DIALECT 5')


Consider adding a for loop over dialects 2 and 5 (as you did before) for the part that is common, and then adding another section with the part relevant only for dialect 5. This may help with clarifying the difference between them later on.

OK. Done. Thanks.

raz-mon · 2024-05-09T11:27:11Z

tests/pytests/test_search_params.py

+    env.expect('FT.SEARCH', 'idx', '@n:[+-$n -+$n]', 'PARAMS', 2, 'n', 1).error()
+    env.expect('FT.SEARCH', 'idx', '@n:[++$n --$m]', 'PARAMS', 2, 'n', 1).error()


Is there any special error that is raised here (I suspect no)?
You can add the error().contains('...)` function to verify the error thrown.

No. there is not an special error message, only Syntax error

#4604) * Create parser v3 * Test dialect: DEFAULT_DIALECT as module arg * More tests for text queries * Improve invalid syntax text * Fix parser v3, unary op after field name * Add missint test with modifierlist * WIP: Test isempty() with DIALECT 5 * test_v1_vs_v2_vs_v5() * Add tests to improve codecov using dialect 5 * One more test to improve codecov * Add WITHCOUNT to fix test with DIALECT 5 * Fix testEmptyValueTags() for DIALECT > 2 * Fix test_tags * Test float without leading zero * Fix wrong float number test * Test ragel minization at the end of compilation * Revert "Test ragel minization at the end of compilation" This reverts commit 0dae78b. * Fix make-parser.mk * cpp-test parser v3 * Fix float number syntax, leading zero is optional * Test number format * Support numbers with multiple signs * Create macros in parser.y v3 * Use set_max_dialect * Fix testEmptyValueTags() * MOD-6750 Fix numeric range syntax (#4505) Fix numeric range syntax * MOD-6749: Querying numeric fields using simple operators (#4516)

Fix numeric range syntax (cherry picked from commit c05367b)

* Add query_parser/v3 * Add more tag tests * Add test for TEXT testExact * autoescaping single tag between brackets * Fix tests COORD=1 * Fix wildcard support * wildcard + prefix/infix/suffix is invalid * Test backward compatibility * Test some uncovered cases * Test prefix/infix/suffix with TEXT field * Remove temporary debug messages * Test: escape 'w' single_tag * Add more tag tests * WIP: Tests to increase coverage parser v3 * Add more tests * Fix lexer.c v3 * Fix test invalid syntax * Test punct and cntrl characters * split unescaped_tag rule * Add one more test UNESCAPED_TAG * Fix expected test format in cluster * Update src/query_parser/v3/lexer.rl - Fix description Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com> * Test pipe with dialect < 5, add comment about backslack escaping * Test escaping $ * One more test escaping $ * lexer v3 - remove leading and trailing spaces * Test short tags * Add JSON tests * Use comma separator for JSON tests * Add test testTagUNF() * Test tag autoescaping using DEFAULT_DIALECT 5 * More test using DEFAULT DIALECT 5 in test_search_params: test_geo, test_attr, test_binary_data. * Revert changes to test_search_params:test_geo * testTagUNF: Create index before hashes * Revert change in QueryNode_DumpSds() * Test aggregate with TAG autoescaping * Fix testTagAutoescaping, remove additional right curly brace * Update testDialect5InvalidSyntax() * update parser/v3 taking latest parser/v2 * Create parser v3 * Test dialect: DEFAULT_DIALECT as module arg * More tests for text queries * Improve invalid syntax text * Fix parser v3, unary op after field name * Add missint test with modifierlist * WIP: Test isempty() with DIALECT 5 * test_v1_vs_v2_vs_v5() * Add tests to improve codecov using dialect 5 * One more test to improve codecov * Add WITHCOUNT to fix test with DIALECT 5 * Fix testEmptyValueTags() for DIALECT > 2 * Fix test_tags * Test float without leading zero * Fix wrong float number test * Test ragel minization at the end of compilation * Revert "Test ragel minization at the end of compilation" This reverts commit 0dae78b. * Fix make-parser.mk * cpp-test parser v3 * Update parser v3 * Update lexer * Fix tests * Fix float number syntax, leading zero is optional * Test number format * Support numbers with multiple signs * Create macros in parser.y v3 * Use set_max_dialect * Fix testEmptyValueTags() * MOD-6750 Fix numeric range syntax (#4505) Fix numeric range syntax * MOD-6749: Querying numeric fields using simple operators (#4516) * Simplify single_tag * Remove unescaped_tag2 * lexer v3 - remove colon from tag expressions * Create unescaped_tag2 to create UNESCAPED_TAG without escape * Fix leading/trailing spaces deletion * Validate tok.len * Remove debugging code * Fix lexer.rl v3 format * Temp: Try to fix sanitizer * Revert "Temp: Try to fix sanitizer" This reverts commit 66002f1. * Test tag with * as literal * Fix parser: tag rules * Update tests/pytests/test_tags.py - minor typo --------- Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>

* Add query_parser/v3 * Add more tag tests * Add test for TEXT testExact * autoescaping single tag between brackets * Fix tests COORD=1 * Fix wildcard support * wildcard + prefix/infix/suffix is invalid * Test backward compatibility * Test some uncovered cases * Test prefix/infix/suffix with TEXT field * Remove temporary debug messages * Test: escape 'w' single_tag * Add more tag tests * WIP: Tests to increase coverage parser v3 * Add more tests * Fix lexer.c v3 * Fix test invalid syntax * Test punct and cntrl characters * split unescaped_tag rule * Add one more test UNESCAPED_TAG * Fix expected test format in cluster * Update src/query_parser/v3/lexer.rl - Fix description Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com> * Test pipe with dialect < 5, add comment about backslack escaping * Test escaping $ * One more test escaping $ * lexer v3 - remove leading and trailing spaces * Test short tags * Add JSON tests * Use comma separator for JSON tests * Add test testTagUNF() * Test tag autoescaping using DEFAULT_DIALECT 5 * More test using DEFAULT DIALECT 5 in test_search_params: test_geo, test_attr, test_binary_data. * Revert changes to test_search_params:test_geo * testTagUNF: Create index before hashes * Revert change in QueryNode_DumpSds() * Test aggregate with TAG autoescaping * Fix testTagAutoescaping, remove additional right curly brace * Update testDialect5InvalidSyntax() * update parser/v3 taking latest parser/v2 * Create parser v3 * Test dialect: DEFAULT_DIALECT as module arg * More tests for text queries * Improve invalid syntax text * Fix parser v3, unary op after field name * Add missint test with modifierlist * WIP: Test isempty() with DIALECT 5 * test_v1_vs_v2_vs_v5() * Add tests to improve codecov using dialect 5 * One more test to improve codecov * Add WITHCOUNT to fix test with DIALECT 5 * Fix testEmptyValueTags() for DIALECT > 2 * Fix test_tags * Test float without leading zero * Fix wrong float number test * Test ragel minization at the end of compilation * Revert "Test ragel minization at the end of compilation" This reverts commit 0dae78b. * Fix make-parser.mk * cpp-test parser v3 * Update parser v3 * Update lexer * Fix tests * Fix float number syntax, leading zero is optional * Test number format * Support numbers with multiple signs * Create macros in parser.y v3 * Use set_max_dialect * Fix testEmptyValueTags() * MOD-6750 Fix numeric range syntax (#4505) Fix numeric range syntax * MOD-6749: Querying numeric fields using simple operators (#4516) * Simplify single_tag * Remove unescaped_tag2 * lexer v3 - remove colon from tag expressions * Create unescaped_tag2 to create UNESCAPED_TAG without escape * Fix leading/trailing spaces deletion * Validate tok.len * Remove debugging code * Fix lexer.rl v3 format * Temp: Try to fix sanitizer * Revert "Temp: Try to fix sanitizer" This reverts commit 66002f1. * Test tag with * as literal * Fix parser: tag rules * Update tests/pytests/test_tags.py - minor typo --------- Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com> (cherry picked from commit e907ece)

nafraf added 5 commits March 5, 2024 12:19

Allow case unsensitive infinity, allow signed exponent

7106c33

Fail on range with 2 exclusive identical values

0556544

Allow signed params

f433c30

Allow range with 2 exclusive identical values

c20de4f

Revert breaking change: allow multiple '(' before number

e6be663

nafraf marked this pull request as ready for review March 6, 2024 15:53

nafraf requested review from oshadmi and raz-mon March 6, 2024 15:56

Support numbers and parameters preceded by PLUS

065b16f

oshadmi reviewed Mar 8, 2024

View reviewed changes

tests/pytests/test_numbers.py Outdated Show resolved Hide resolved

tests/pytests/test_numbers.py Outdated Show resolved Hide resolved

tests/pytests/test_search_params.py Outdated Show resolved Hide resolved

Test ranges with +/-inf and parameters = +/- inf

122056c

nafraf requested a review from oshadmi March 9, 2024 04:58

oshadmi reviewed Mar 9, 2024

View reviewed changes

Add FILTER tests

63cb465

nafraf requested a review from oshadmi March 10, 2024 23:06

Test explain numeric range

fdf458e

oshadmi previously approved these changes Mar 12, 2024

View reviewed changes

nafraf added this pull request to the merge queue Mar 12, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 12, 2024

nafraf added this pull request to the merge queue Mar 12, 2024

Test range: spaces between sign and the number are not valid

6c3ec77

nafraf removed this pull request from the merge queue due to a manual request Mar 12, 2024

nafraf dismissed oshadmi’s stale review via 6c3ec77 March 12, 2024 17:17

nafraf and others added 2 commits March 12, 2024 17:33

Revert "Test range: spaces between sign and the number are not valid"

c7ff9d2

This reverts commit 6c3ec77.

Merge branch 'master' into nafraf_fix-num-range-syntax

1cc632f

raz-mon reviewed Mar 19, 2024

View reviewed changes

tests/pytests/test.py Outdated Show resolved Hide resolved

tests/pytests/test.py Outdated Show resolved Hide resolved

tests/pytests/test_numbers.py Outdated Show resolved Hide resolved

tests/pytests/test_search_params.py Show resolved Hide resolved

oshadmi mentioned this pull request Mar 19, 2024

MOD-6749: Querying numeric fields using simple operators #4516

Merged

2 tasks

Merge branch 'master' into nafraf_fix-num-range-syntax

14f8aef

nafraf marked this pull request as draft April 15, 2024 18:31

nafraf mentioned this pull request Apr 18, 2024

MOD-6933 MOD-6750 MOD-6749: Lexer consumes some punctuation characters #4604

Merged

2 tasks

nafraf added 4 commits April 26, 2024 11:54

Merge branch 'nafraf_parser-v3-punct' into nafraf_fix-num-range-syntax

38aaf70

Move changes to parser v3

62111b1

Delete test testExplainNumericRange(), tests moved to test:testExplain()

a448913

test.py clean up

3e7205a

nafraf marked this pull request as ready for review April 27, 2024 10:45

nafraf and others added 5 commits April 28, 2024 11:32

Fix parseDoubleRange()

a515ea3

Merge branch 'nafraf_parser-v3-punct' into nafraf_fix-num-range-syntax

f84cc5f

Merge branch 'master' into nafraf_fix-num-range-syntax

4f9055d

Merge remote-tracking branch 'origin/nafraf_parser-v3-punct' into naf…

deb9609

…raf_fix-num-range-syntax

Merge remote-tracking branch 'origin/nafraf_parser-v3-punct' into naf…

76eca6d

…raf_fix-num-range-syntax

raz-mon reviewed May 9, 2024

View reviewed changes

Test numeric range using dialect 2 and 5

e167fc1

nafraf changed the base branch from master to nafraf_parser-v3-punct May 10, 2024 18:32

oshadmi approved these changes May 13, 2024

View reviewed changes

nafraf merged commit c05367b into nafraf_parser-v3-punct May 15, 2024
10 checks passed

nafraf deleted the nafraf_fix-num-range-syntax branch May 15, 2024 12:10

github-actions bot pushed a commit that referenced this pull request May 16, 2024

MOD-6750 Fix numeric range syntax (#4505)

d1347e8

Fix numeric range syntax (cherry picked from commit c05367b)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MOD-6750 Fix numeric range syntax #4505

MOD-6750 Fix numeric range syntax #4505

nafraf commented Mar 5, 2024 •

edited

codecov bot commented Mar 6, 2024 •

edited

oshadmi left a comment

oshadmi left a comment

oshadmi Mar 9, 2024

nafraf Mar 10, 2024

oshadmi Mar 9, 2024

nafraf Mar 10, 2024

oshadmi Mar 9, 2024

nafraf Mar 10, 2024

oshadmi left a comment

raz-mon left a comment

raz-mon left a comment

raz-mon May 9, 2024

nafraf May 9, 2024

raz-mon May 9, 2024

nafraf May 9, 2024

	(sign == -1 && (!strcasecmp(s, "+inf") \|\| !strcasecmp(s, "inf"))))) {
	(sign == -1 && !strcasecmp((*s == '+' ? s + 1 : s), "inf")))) {

	(sign == 1 && (!strcasecmp(s, "+inf") \|\| !strcasecmp(s, "inf"))) \|\|
	(sign == 1 && !strcasecmp((*s == '+' ? s + 1 : s), "inf")) \|\|

		env = Env(moduleArgs = 'DEFAULT_DIALECT 2')
		env = Env(moduleArgs = 'DEFAULT_DIALECT 5')

		env.expect('FT.SEARCH', 'idx', '@n:[+-$n -+$n]', 'PARAMS', 2, 'n', 1).error()
		env.expect('FT.SEARCH', 'idx', '@n:[++$n --$m]', 'PARAMS', 2, 'n', 1).error()

MOD-6750 Fix numeric range syntax #4505

MOD-6750 Fix numeric range syntax #4505

Conversation

nafraf commented Mar 5, 2024 • edited

codecov bot commented Mar 6, 2024 • edited

Codecov Report

oshadmi left a comment

Choose a reason for hiding this comment

oshadmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshadmi left a comment

Choose a reason for hiding this comment

raz-mon left a comment

Choose a reason for hiding this comment

raz-mon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nafraf commented Mar 5, 2024 •

edited

codecov bot commented Mar 6, 2024 •

edited