Fix APPLY/FILTER parser - [MOD-7804] by GuyAv46 · Pull Request #5520 · RediSearch/RediSearch

GuyAv46 · 2025-01-17T13:45:40Z

Describe the changes in the pull request

Refactor parser to follow the expected order of operations.

This PR also enables some internal optimizations to the expression AST, such as:

perform in-place numeric calculations for literals, when possible
Double negation cancels each other and yields the inner node

Which additional issues this PR fixes

MOD-6835

Main objects this PR modified

APPLY/FILTER parser

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

codecov · 2025-01-17T14:19:18Z

Codecov Report

Attention: Patch coverage is 97.60479% with 8 lines in your changes missing coverage. Please review.

Project coverage is 87.27%. Comparing base (861d36d) to head (e937404).
Report is 13 commits behind head on master.

Files with missing lines	Patch %	Lines
src/aggregate/expr/lexer.c	86.36%	6 Missing ⚠️
src/aggregate/expr/expression.c	80.00%	1 Missing ⚠️
src/aggregate/expr/parser.c	99.37%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5520      +/-   ##
==========================================
+ Coverage   87.18%   87.27%   +0.08%     
==========================================
  Files         196      196              
  Lines       35110    35268     +158     
==========================================
+ Hits        30611    30780     +169     
+ Misses       4499     4488      -11

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

src/aggregate/functions/string.c

raz-mon

Very nice!
Some small comments

src/aggregate/expr/expression.c

src/aggregate/expr/parser.y

tests/pytests/test_aggregate.py

tests/pytests/test_issues.py

tests/pytests/test_profile.py

kei-nan

Added one optional comment, besides that looks good.

raz-mon

💯

* 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 (cherry picked from commit eb14b95)

redisearch-backport-pull-request · 2025-01-23T17:31:14Z

Successfully created backport PR for 8.0:

[8.0] Fix APPLY/FILTER parser - [MOD-7804] #5564

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 (cherry picked from commit eb14b95) Co-authored-by: GuyAv46 <[email protected]>

* 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

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

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

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

GuyAv46 added 8 commits January 17, 2025 11:10

fix order of operations

16e3882

minor improvements to the lexer

1061067

improve functions parsing

3502873

optimize "NOT" logic and perform arithmetic operations immediately

2433804

fix flow tests

8386445

fix grammar optimization

ea4a7d5

changed function API

5ed90b0

added a test

74c4995

GuyAv46 added bug backport 8.0 labels Jan 17, 2025

GuyAv46 marked this pull request as draft January 17, 2025 13:45

minor fix

853ecc9

GuyAv46 marked this pull request as ready for review January 17, 2025 13:50

GuyAv46 marked this pull request as draft January 17, 2025 13:51

GuyAv46 added 3 commits January 17, 2025 16:25

fix precedence

0776b15

minor improvement

63424e8

reorder rule and fix leak

1b647fa

GuyAv46 marked this pull request as ready for review January 17, 2025 14:58

GuyAv46 marked this pull request as draft January 17, 2025 14:58

GuyAv46 added 2 commits January 17, 2025 17:34

another fix

26d5dcf

added test

f992405

GuyAv46 marked this pull request as ready for review January 17, 2025 15:34

GuyAv46 marked this pull request as draft January 17, 2025 15:35

more tests for a better coverage

71b235f

GuyAv46 marked this pull request as ready for review January 17, 2025 16:37

GuyAv46 marked this pull request as draft January 17, 2025 16:38

GuyAv46 added 2 commits January 19, 2025 08:58

improved test

2a01abd

fix assertion

d30e72b

GuyAv46 changed the title ~~Fix APPLY/FILTER parser~~ Fix APPLY/FILTER parser - [MOD-7804] Jan 19, 2025

GuyAv46 requested a review from kei-nan January 22, 2025 11:47

address code review

20458ae

kei-nan reviewed Jan 23, 2025

View reviewed changes

src/aggregate/functions/string.c Show resolved Hide resolved

raz-mon reviewed Jan 23, 2025

View reviewed changes

GuyAv46 requested review from kei-nan and raz-mon January 23, 2025 10:43

kei-nan reviewed Jan 23, 2025

View reviewed changes

tests/pytests/test_profile.py Show resolved Hide resolved

kei-nan reviewed Jan 23, 2025

View reviewed changes

tests/pytests/test_profile.py Show resolved Hide resolved

kei-nan previously approved these changes Jan 23, 2025

View reviewed changes

added comments

e937404

GuyAv46 dismissed kei-nan’s stale review via e937404 January 23, 2025 13:32

GuyAv46 enabled auto-merge January 23, 2025 13:33

GuyAv46 requested a review from kei-nan January 23, 2025 13:33

raz-mon approved these changes Jan 23, 2025

View reviewed changes

kei-nan approved these changes Jan 23, 2025

View reviewed changes

GuyAv46 added this pull request to the merge queue Jan 23, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 23, 2025

GuyAv46 added this pull request to the merge queue Jan 23, 2025

Merged via the queue into master with commit eb14b95 Jan 23, 2025
10 checks passed

GuyAv46 deleted the guyav-fix_apply_filter_parser branch January 23, 2025 17:30

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

[8.0] Fix APPLY/FILTER parser - [MOD-7804] #5564

Merged

oshadmi mentioned this pull request Jun 19, 2025

MOD-10176: Fix parser for negation of nonexistent function #6337

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix APPLY/FILTER parser - [MOD-7804]#5520

Fix APPLY/FILTER parser - [MOD-7804]#5520
GuyAv46 merged 20 commits intomasterfrom
guyav-fix_apply_filter_parser

GuyAv46 commented Jan 17, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jan 17, 2025 •

edited

Loading

Uh oh!

Uh oh!

raz-mon left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kei-nan left a comment

Uh oh!

raz-mon left a comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Describe the changes in the pull request

Which additional issues this PR fixes

Main objects this PR modified

Mark if applicable

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

raz-mon left a comment

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!

kei-nan left a comment

Choose a reason for hiding this comment

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 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GuyAv46 commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading