-
Notifications
You must be signed in to change notification settings - Fork 516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support FLOAT64 as vector data type MOD-3982 #3129
Conversation
This pull request introduces 1 alert and fixes 24 when merging 9f74b3b into 2144521 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 24 when merging 4393d59 into 2144521 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 24 when merging 9d5b398 into 2144521 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 24 when merging 863507d into 7991f4a - view on LGTM.com new alerts:
fixed alerts:
|
Codecov ReportBase: 82.71% // Head: 82.74% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3129 +/- ##
==========================================
+ Coverage 82.71% 82.74% +0.03%
==========================================
Files 180 180
Lines 29654 29660 +6
==========================================
+ Hits 24528 24542 +14
+ Misses 5126 5118 -8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -127,7 +127,7 @@ FT.CREATE my_index2 | |||
SCHEMA vector_field VECTOR | |||
HNSW | |||
14 | |||
TYPE FLOAT32 | |||
TYPE FLOAT64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add another example instead of changing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FLAT index example is with FLOAT32... Didn't want to overload with examples...
# For FLOAT64, this block size exceeds 10% of system memory, but not for FLOAT32 | ||
block_size = system_memory // (dim*float64_byte_size) // 9 | ||
if data_type == 'FLOAT32': | ||
env.expect('FT.CREATE', currIdx, 'SCHEMA', 'v', 'VECTOR', 'FLAT', '10', 'TYPE', data_type, | ||
'DIM', dim, 'DISTANCE_METRIC', 'L2', 'INITIAL_CAP', 0, 'BLOCK_SIZE', block_size).ok() | ||
else: | ||
env.expect('FT.CREATE', currIdx, 'SCHEMA', 'v', 'VECTOR', 'FLAT', '10', 'TYPE', data_type, | ||
'DIM', dim, 'DISTANCE_METRIC', 'L2', 'INITIAL_CAP', 0, 'BLOCK_SIZE', block_size).error().contains( | ||
f'Vector index block size {block_size} exceeded server limit') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing that one type is passing and the other is not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but that's the exact purpose of the test... to test the difference in memory estimation between the two types
This pull request introduces 1 alert and fixes 24 when merging 20a0b6e into 7991f4a - view on LGTM.com new alerts:
fixed alerts:
|
Integrating the support of FLOAT64 vectors into RediSearch and add flow tests. The enablement required two small modifications (due to preparation work done in advanced):
ft.create
- allow specifyingFLOAT64
as the value for theTYPE
argument of a vector field in a schema - for example:FT.CREATE idx SCHEMA v VECTOR HNSW 6 TYPE FLOAT64 DIM 4096 DISTANCE_METRIC L2
float64
in addition tofloat32
)Rest of this PR handles testing, including refactoring of
vecsim_test.py
making it more generic, thus allowing test to run over more than one hard-coded data type.