[MOD-8125] Create inverted index in write operations only by meiravgri · Pull Request #5229 · RediSearch/RediSearch

meiravgri · 2024-11-19T14:49:07Z

This PR ensures we create an inverted index only in write operations (when possible. see exceptions below)

Read operations

ft.info
INFO
queries
GC

New in this PR

`FT.DEBUG SPEC_INVIDXES_INFO INDEX_NAME`

Returnes

inverted_indexes_dict_size
inverted_indexes_memory

Exceptions

Some read operations still create the index to avoid breaking changes, or where a fundamental change was required:

debug commands

VecsimInfo and dumpHNSWData we need an index to create an info iterator.
DumpGeometryIndex calls abstract api->dump(idx, ctx). To allow avoiding from creating the index, we need a dump function that can imitate an empty index for idx == NULL.

Queries

Query_EvalGeometryNode: since the query string is validated after we are trying to open the geometry index, in case of a wrong query string and uninitialized index, we will return results as if the index was empty, instead of raising an error. However, If we create the index (as we are doing today), we can proceed to query validation.
So until we change the order of verification, we open the index with CREATE option.

codecov · 2024-11-19T15:13:35Z

Codecov Report

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

Project coverage is 86.68%. Comparing base (d67d645) to head (0c64a3c).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/fork_gc.c	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5229      +/-   ##
==========================================
+ Coverage   86.64%   86.68%   +0.04%     
==========================================
  Files         193      193              
  Lines       34698    34710      +12     
==========================================
+ Hits        30063    30089      +26     
+ Misses       4635     4621      -14

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

add create_if_missing variable to OpenGeometryIndex dont create text index in gc if doesn't exist

…dex_for_read

introduce test_index to test lazy intialziation

src/vector_index.c

GuyAv46 · 2024-11-21T13:45:13Z

tests/pytests/common.py

+                    return False
+        except KeyError:
+            if _assert:
+                env.assertTrue(False, message=msg + f" key {key} exists in {d1_name} but doesn't exist in {d2_name}")


Suggested change

env.assertTrue(False, message=msg + f" key {key} exists in {d1_name} but doesn't exist in {d2_name}")

env.assertContains(key, d2, message=msg + f" key {key} exists in {d1_name} but doesn't exist in {d2_name}")

assertContains doesn't support message :(

We should fix it

GuyAv46 · 2024-11-21T13:47:41Z

tests/pytests/test_common.py

+    d3 = d1.copy()
+    d1["rabbit"] = 5
+    try:
+        compare_numeric_dicts(env, d1, d3, _assert=False)


Is _assert=False causing an exception aligned with the RLTest behavior?

It is used by the function as a hint to avoid assertions (to test expected failures)

tests/pytests/common.py

alonre24

Good catch! So except for vector and geometry, were there any other places where we created an index unnecessarily (except for the tag in the GC)?

tests/pytests/test_index.py

src/tag_index.c

src/debug_commands.c

alonre24 · 2024-11-22T18:56:56Z

src/fork_gc.c

  RedisSearchCtx_LockSpecWrite(sctx);

-  InvertedIndex *idx = Redis_OpenInvertedIndex(sctx, term, len, 1, NULL);
+  InvertedIndex *idx = Redis_OpenInvertedIndex(sctx, term, len, DONT_CREATE_INDEX, NULL);


Do we understand why it was explicitly opened for writing here? Also, I didn't see a test validating the expected behavior with respect to GC...

No. But if we got here the term existed before we forked
Regarding gc - don't we have unit tests for text?

tests/pytests/test_index.py

src/spec.c

add depth arg to pytest callbacks add test_lazy_index_creation_info_modules some tests improvments

…dex_for_read

tests/pytests/test_common.py

GuyAv46 · 2024-11-24T20:06:31Z

src/query.c

+
+  // TODO: open with DONT_CREATE_INDEX once the query string is validated before we get here.
+  // Currently, if  we use DONT_CREATE_INDEX, and the index was not initalized yet, and the query is invalid,
+  // we return results as if the index was empty, instead of raising an error.


What should be done here? Parse the query before getting here? Fix something in our parser? Can we do something in this PR?

Yes. not in the scope of this PR
I'm opening a ticket

redisearch-backport-pull-request · 2024-11-25T09:41:33Z

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-5229-to-2.8 origin/2.8
cd .worktree/backport-5229-to-2.8
git switch --create backport-5229-to-2.8
git cherry-pick -x 7617563a752f9e2ddc49d57e4c2d5b3397be67ef

redisearch-backport-pull-request · 2024-11-25T09:41:34Z

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-5229-to-2.6 origin/2.6
cd .worktree/backport-5229-to-2.6
git switch --create backport-5229-to-2.6
git cherry-pick -x 7617563a752f9e2ddc49d57e4c2d5b3397be67ef

redisearch-backport-pull-request · 2024-11-25T09:41:35Z

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

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

git fetch origin 2.10
git worktree add -d .worktree/backport-5229-to-2.10 origin/2.10
cd .worktree/backport-5229-to-2.10
git switch --create backport-5229-to-2.10
git cherry-pick -x 7617563a752f9e2ddc49d57e4c2d5b3397be67ef

redisearch-backport-pull-request · 2024-11-25T09:41:36Z

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

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

git fetch origin 8.0
git worktree add -d .worktree/backport-5229-to-8.0 origin/8.0
cd .worktree/backport-5229-to-8.0
git switch --create backport-5229-to-8.0
git cherry-pick -x 7617563a752f9e2ddc49d57e4c2d5b3397be67ef

* open vector for read * don't create vector index in read mode * use OPEN_INDEX_READ or OPEN_INDEX_WRITE in tags * remove unused openindex function * create vecsim index to capture info in debug commands add create_if_missing variable to OpenGeometryIndex dont create text index in gc if doesn't exist * use DONT_CREATE_INDEX in NewVectorIterator * introduce FT.DEBUG SPEC_INVIDXES_INFO INDEX_NAME introduce test_index to test lazy intialziation * fix * cosmetics * skip testSpecIndexesInfo in coord * rename openVectorKeysDict to openVectorIndex add depth arg to pytest callbacks add test_lazy_index_creation_info_modules some tests improvments * fix field name in test * pass (cherry picked from commit 7617563)

* [MOD-8125] Create inverted index in write operations only (#5229) * open vector for read * don't create vector index in read mode * use OPEN_INDEX_READ or OPEN_INDEX_WRITE in tags * remove unused openindex function * create vecsim index to capture info in debug commands add create_if_missing variable to OpenGeometryIndex dont create text index in gc if doesn't exist * use DONT_CREATE_INDEX in NewVectorIterator * introduce FT.DEBUG SPEC_INVIDXES_INFO INDEX_NAME introduce test_index to test lazy intialziation * fix * cosmetics * skip testSpecIndexesInfo in coord * rename openVectorKeysDict to openVectorIndex add depth arg to pytest callbacks add test_lazy_index_creation_info_modules some tests improvments * fix field name in test * pass (cherry picked from commit 7617563) * fix debug commands test

* open vector for read * don't create vector index in read mode * use OPEN_INDEX_READ or OPEN_INDEX_WRITE in tags * remove unused openindex function * create vecsim index to capture info in debug commands add create_if_missing variable to OpenGeometryIndex dont create text index in gc if doesn't exist * use DONT_CREATE_INDEX in NewVectorIterator * introduce FT.DEBUG SPEC_INVIDXES_INFO INDEX_NAME introduce test_index to test lazy intialziation * fix * cosmetics * skip testSpecIndexesInfo in coord * rename openVectorKeysDict to openVectorIndex add depth arg to pytest callbacks add test_lazy_index_creation_info_modules some tests improvments * fix field name in test * pass (cherry picked from commit 7617563)

meiravgri · 2024-11-26T14:50:35Z

won't be cherry-picked to 2.6 due to discrepancy in open index functions signature

[MOD-8125] Create inverted index in write operations only (#5229) * open vector for read * don't create vector index in read mode * use OPEN_INDEX_READ or OPEN_INDEX_WRITE in tags * remove unused openindex function * create vecsim index to capture info in debug commands add create_if_missing variable to OpenGeometryIndex dont create text index in gc if doesn't exist * use DONT_CREATE_INDEX in NewVectorIterator * introduce FT.DEBUG SPEC_INVIDXES_INFO INDEX_NAME introduce test_index to test lazy intialziation * fix * cosmetics * skip testSpecIndexesInfo in coord * rename openVectorKeysDict to openVectorIndex add depth arg to pytest callbacks add test_lazy_index_creation_info_modules some tests improvments * fix field name in test * pass (cherry picked from commit 7617563)

* [MOD-8125] Create inverted index in write operations only (#5229) * open vector for read * don't create vector index in read mode * use OPEN_INDEX_READ or OPEN_INDEX_WRITE in tags * remove unused openindex function * create vecsim index to capture info in debug commands add create_if_missing variable to OpenGeometryIndex dont create text index in gc if doesn't exist * use DONT_CREATE_INDEX in NewVectorIterator * introduce FT.DEBUG SPEC_INVIDXES_INFO INDEX_NAME introduce test_index to test lazy intialziation * fix * cosmetics * skip testSpecIndexesInfo in coord * rename openVectorKeysDict to openVectorIndex add depth arg to pytest callbacks add test_lazy_index_creation_info_modules some tests improvments * fix field name in test * pass (cherry picked from commit 7617563) * fix include * fix number of expected args in new debug command remove DUMP_HNSW from test_lazy_index_creation_debug_commands as not introduced to this version

meiravgri added 4 commits November 19, 2024 14:48

open vector for read

aaa507c

don't create vector index in read mode

a96862b

use OPEN_INDEX_READ or OPEN_INDEX_WRITE in tags

d506ddc

remove unused openindex function

cc57b33

meiravgri force-pushed the meiravg_open_inv_index_for_read branch from d1fff41 to cc57b33 Compare November 19, 2024 14:50

meiravgri added 7 commits November 20, 2024 18:27

create vecsim index to capture info in debug commands

6a82e70

add create_if_missing variable to OpenGeometryIndex dont create text index in gc if doesn't exist

use DONT_CREATE_INDEX in NewVectorIterator

357b429

Merge remote-tracking branch 'origin/master' into meiravg_open_inv_in…

d52624c

…dex_for_read

introduce FT.DEBUG SPEC_INVIDXES_INFO INDEX_NAME

95fd9f7

introduce test_index to test lazy intialziation

fix

ec1727c

cosmetics

239beb8

skip testSpecIndexesInfo in coord

1652436

meiravgri changed the title ~~Meiravg_open_inv_index_for_readgi~~ [MOD-8125] Create index in write operations only Nov 21, 2024

meiravgri changed the title ~~[MOD-8125] Create index in write operations only~~ [MOD-8125] Create inverted index in write operations only Nov 21, 2024

meiravgri requested a review from alonre24 November 21, 2024 13:33

GuyAv46 reviewed Nov 21, 2024

View reviewed changes

alonre24 reviewed Nov 22, 2024

View reviewed changes

meiravgri added 3 commits November 24, 2024 17:13

rename openVectorKeysDict to openVectorIndex

4acefb9

add depth arg to pytest callbacks add test_lazy_index_creation_info_modules some tests improvments

Merge remote-tracking branch 'origin/master' into meiravg_open_inv_in…

63bcb9e

…dex_for_read

fix field name in test

b123696

meiravgri requested review from GuyAv46 and alonre24 November 24, 2024 17:40

meiravgri added backport 2.8 backport 2.6 backport 2.10 backport 8.0 labels Nov 24, 2024

GuyAv46 reviewed Nov 24, 2024

View reviewed changes

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

GuyAv46 previously approved these changes Nov 24, 2024

View reviewed changes

pass

0c64a3c

meiravgri dismissed GuyAv46’s stale review via 0c64a3c November 25, 2024 06:27

GuyAv46 approved these changes Nov 25, 2024

View reviewed changes

alonre24 approved these changes Nov 25, 2024

View reviewed changes

meiravgri added this pull request to the merge queue Nov 25, 2024

Merged via the queue into master with commit 7617563 Nov 25, 2024

meiravgri deleted the meiravg_open_inv_index_for_read branch November 25, 2024 09:41

meiravgri mentioned this pull request Nov 26, 2024

[2.10] [MOD-8125] Create inverted index in write operations only #5257

Merged

meiravgri mentioned this pull request Nov 26, 2024

[8.0] [MOD-8125] Create inverted index in write operations only #5259

Merged

meiravgri mentioned this pull request Nov 26, 2024

[2.8] [MOD-8125] Create inverted index in write operations only #5260

Merged

	env.assertTrue(False, message=msg + f" key {key} exists in {d1_name} but doesn't exist in {d2_name}")
	env.assertContains(key, d2, message=msg + f" key {key} exists in {d1_name} but doesn't exist in {d2_name}")

Conversation

meiravgri commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Read operations

New in this PR

FT.DEBUG SPEC_INVIDXES_INFO INDEX_NAME

Exceptions

debug commands

Queries

Uh oh!

codecov bot commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alonre24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redisearch-backport-pull-request bot commented Nov 25, 2024

Uh oh!

redisearch-backport-pull-request bot commented Nov 25, 2024

Uh oh!

redisearch-backport-pull-request bot commented Nov 25, 2024

Uh oh!

redisearch-backport-pull-request bot commented Nov 25, 2024

Uh oh!

meiravgri commented Nov 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

meiravgri commented Nov 19, 2024 •

edited

Loading

`FT.DEBUG SPEC_INVIDXES_INFO INDEX_NAME`

codecov bot commented Nov 19, 2024 •

edited

Loading