-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Adds support for dense_vector type #1708
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
Conversation
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.
Wow. The first time I get a PR - and that from a first-time contributor - where I have (almost nothing) to remark. Just one thing: check that dims
is set for FieldType.DenseVector
. I commented that at the corresponding place.
@@ -191,6 +197,8 @@ private MappingParameters(InnerField field) { | |||
|| (maxShingleSize >= 2 && maxShingleSize <= 4), // | |||
"maxShingleSize must be in inclusive range from 2 to 4 for field type search_as_you_type"); | |||
positiveScoreImpact = field.positiveScoreImpact(); | |||
dims = field.dims(); | |||
Assert.isTrue(dims <= 2048, "The maximum number of dimensions that can be in a vector should not exceed 2048."); |
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.
Assert.isTrue(dims <= 2048, "The maximum number of dimensions that can be in a vector should not exceed 2048."); | |
Assert.isTrue(dims <= 2048, "The maximum number of dimensions that can be in a vector must not exceed 2048."); |
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.
updated
@@ -153,6 +157,8 @@ private MappingParameters(Field field) { | |||
|| (maxShingleSize >= 2 && maxShingleSize <= 4), // | |||
"maxShingleSize must be in inclusive range from 2 to 4 for field type search_as_you_type"); | |||
positiveScoreImpact = field.positiveScoreImpact(); | |||
dims = field.dims(); | |||
Assert.isTrue(dims <= 2048, "The maximum number of dimensions that can be in a vector should not exceed 2048."); |
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.
Assert.isTrue(dims <= 2048, "The maximum number of dimensions that can be in a vector should not exceed 2048."); | |
Assert.isTrue(dims <= 2048, "The maximum number of dimensions that can be in a vector must not exceed 2048."); |
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.
updated
if (dims >= 1) { | ||
builder.field(FIELD_PARAM_DIMS, dims); | ||
} | ||
|
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.
According to https://www.elastic.co/guide/en/elasticsearch/reference/current/dense-vector.html dims
is a required parameter. So it should be checked that if type == FieldType.DenseVector
then the dims
value must be greater or equal 1.
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.
A dense_vector field stores dense vectors of float values. The maximum number of dimensions that can be in a vector should not exceed 2048. A dense_vector field is a single-valued field. Closes spring-projects#1700
8484b00
to
d9cbaa5
Compare
@sothawo thanks so much for the quick review and help!! 🙏 |
Hi all. We need to support a
dense_vector
field for our elastic search implementation. I saw you have an open ticket to support this here #1700. I only includeddense_vector
sincesparse_vector
is deprecated and bothrank_feature
andrank_features
appear to already be supported.Closes #1700