MOD-8561: Fix Inverted Index SeekTo Edge Case by kei-nan · Pull Request #5528 · RediSearch/RediSearch

kei-nan · 2025-01-20T12:34:04Z

Describe the changes in the pull request

A clear and concise description of what the PR is solving, including:

Current: Seeking to the last doc id in the last block when the reader is already pointing at it can crash.
Change: Avoid advancing to the next block if the id we need is in the end of the current block.
Outcome: Avoid crashing in this edge case.

We revert part of the changes from MOD-8255, PR #5350

Main objects this PR modified

Inverted index

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

codecov · 2025-01-20T13:00:24Z

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.90%. Comparing base (5f3e051) to head (5db341e).
Report is 4 commits behind head on master.

❗ Current head 5db341e differs from pull request most recent head 94e4b47

Please upload reports for the commit 94e4b47 to get more accurate results.

Files with missing lines	Patch %	Lines
src/index.c	80.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (5f3e051) and HEAD (5db341e). Click for more details.

HEAD has 2 uploads less than BASE

Flag BASE (5f3e051) HEAD (5db341e)

3 1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5528      +/-   ##
==========================================
- Coverage   87.21%   80.90%   -6.32%     
==========================================
  Files         196      196              
  Lines       35226    35221       -5     
==========================================
- Hits        30723    28494    -2229     
- Misses       4503     6727    +2224

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

raz-mon

Nice catch

raz-mon · 2025-01-20T13:34:46Z

src/inverted_index.c

+  const t_docId lastIdOfCurrentBlock = IR_CURRENT_BLOCK(ir).lastId;
+  if (lastIdOfCurrentBlock < docId || (BufferReader_AtEnd(&ir->br) && lastIdOfCurrentBlock != docId)) {
+    // We need to skip to a next block since the requested doc id isn't in the current block


Why not just remove the || BufferReader_AtEnd(&ir->br) from the condition? Is there something special we should do in that case?

redisearch-backport-pull-request · 2025-01-22T18:05:50Z

Backport failed for 2.8, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.8
git worktree add -d .worktree/backport-5528-to-2.8 origin/2.8
cd .worktree/backport-5528-to-2.8
git switch --create backport-5528-to-2.8
git cherry-pick -x 50e3f01dc9ad5b272a11a5053f5d0327d5904b13

redisearch-backport-pull-request · 2025-01-22T18:05:51Z

Backport failed for 2.6, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.6
git worktree add -d .worktree/backport-5528-to-2.6 origin/2.6
cd .worktree/backport-5528-to-2.6
git switch --create backport-5528-to-2.6
git cherry-pick -x 50e3f01dc9ad5b272a11a5053f5d0327d5904b13

* * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01)

redisearch-backport-pull-request · 2025-01-22T18:05:58Z

Successfully created backport PR for 2.10:

[2.10] MOD-8561: Fix Inverted Index SeekTo Edge Case #5536

* * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01)

redisearch-backport-pull-request · 2025-01-22T18:06:03Z

Successfully created backport PR for 8.0:

[8.0] MOD-8561: Fix Inverted Index SeekTo Edge Case #5537

* * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01)

MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528) * * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01) Co-authored-by: kei-nan <[email protected]>

MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528) * * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01)

* MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528) * * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01) * * small fix * * small fix

* MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528) * * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01) * * compilation fix --------- Co-authored-by: kei-nan <[email protected]>

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

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

* initial commit

05a5e31

kei-nan added backport 2.8 backport 2.6 backport 2.10 backport 8.0 labels Jan 20, 2025

kei-nan requested a review from GuyAv46 January 20, 2025 12:34

kei-nan self-assigned this Jan 20, 2025

kei-nan requested a review from raz-mon January 20, 2025 13:22

raz-mon reviewed Jan 20, 2025

View reviewed changes

kei-nan added 2 commits January 20, 2025 15:37

* simplify the fix

9dd0d3c

* revert to old code to solve edge case

94e4b47

GuyAv46 previously approved these changes Jan 20, 2025

View reviewed changes

GuyAv46 added the bug label Jan 20, 2025

kei-nan dismissed GuyAv46’s stale review via 5db341e January 20, 2025 16:33

kei-nan force-pushed the master_jk_fix_mod_8561 branch from 5db341e to 94e4b47 Compare January 22, 2025 11:45

kei-nan requested review from GuyAv46 and raz-mon January 22, 2025 12:09

GuyAv46 approved these changes Jan 22, 2025

View reviewed changes

raz-mon approved these changes Jan 22, 2025

View reviewed changes

kei-nan added this pull request to the merge queue Jan 22, 2025

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

kei-nan added this pull request to the merge queue Jan 22, 2025

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

kei-nan added this pull request to the merge queue Jan 22, 2025

Merged via the queue into master with commit 50e3f01 Jan 22, 2025
18 checks passed

kei-nan deleted the master_jk_fix_mod_8561 branch January 22, 2025 18:05

redisearch-backport-pull-request bot pushed a commit that referenced this pull request Jan 22, 2025

MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528)

29a3931

* * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01)

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

[2.10] MOD-8561: Fix Inverted Index SeekTo Edge Case #5536

Merged

redisearch-backport-pull-request bot pushed a commit that referenced this pull request Jan 22, 2025

MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528)

b50692c

* * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01)

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

[8.0] MOD-8561: Fix Inverted Index SeekTo Edge Case #5537

Merged

kei-nan added a commit that referenced this pull request Jan 23, 2025

MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528)

fb90f9a

* * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01)

kei-nan added a commit that referenced this pull request Jan 23, 2025

MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528)

0b564a9

* * initial commit * * simplify the fix * * revert to old code to solve edge case (cherry picked from commit 50e3f01)

This was referenced Jan 23, 2025

[2.8] MOD-8561: Fix Inverted Index SeekTo Edge Case #5542

Merged

[2.6] MOD-8561: Fix Inverted Index SeekTo Edge Case #5543

Merged

raz-mon mentioned this pull request Jan 23, 2025

[BUG] Redis 7.4.2 crashed by signal: 11, si_code: 128 - intermittently when running FT.SEARCH #5532

Closed

dor-forer pushed a commit that referenced this pull request Jan 26, 2025

MOD-8561: Fix Inverted Index SeekTo Edge Case (#5528)

cf2a566

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

GuyAv46 mentioned this pull request Feb 4, 2025

Revert inverted index iterator changes #5596

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MOD-8561: Fix Inverted Index SeekTo Edge Case#5528

MOD-8561: Fix Inverted Index SeekTo Edge Case#5528
kei-nan merged 3 commits intomasterfrom
master_jk_fix_mod_8561

kei-nan commented Jan 20, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jan 20, 2025 •

edited

Loading

Uh oh!

raz-mon left a comment •

edited

Loading

Uh oh!

raz-mon Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

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

Uh oh!

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

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kei-nan commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the changes in the pull request

Main objects this PR modified

Mark if applicable

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

raz-mon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raz-mon Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

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

Uh oh!

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

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kei-nan commented Jan 20, 2025 •

edited

Loading

codecov bot commented Jan 20, 2025 •

edited

Loading

raz-mon left a comment •

edited

Loading