Automatically map floats as dense vector#98512
Conversation
|
@elasticmachine test this please |
b99e0aa to
30348f5
Compare
|
Hi @kderusso, I've created a changelog YAML for you. |
|
Pinging @elastic/es-search (Team:Search) |
|
Pinging @elastic/ent-search-eng (Team:Enterprise Search) |
server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
Outdated
Show resolved
Hide resolved
|
From this issue: #97532
Where do we check if the dense vector has more than 100 values? 🤔 |
benwtrent
left a comment
There was a problem hiding this comment.
I think adding my suggested tests will show some edges (especially around dynamic_templates).
...yamlRestTest/resources/rest-api-spec/test/search.vectors/60_dense_vector_dynamic_mapping.yml
Outdated
Show resolved
Hide resolved
|
@elasticmachine test this please |
1 similar comment
|
@elasticmachine test this please |
benwtrent
left a comment
There was a problem hiding this comment.
I really like all the additional tests. Covers us well.
...yamlRestTest/resources/rest-api-spec/test/search.vectors/60_dense_vector_dynamic_mapping.yml
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
Outdated
Show resolved
Hide resolved
...yamlRestTest/resources/rest-api-spec/test/search.vectors/60_dense_vector_dynamic_mapping.yml
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
Outdated
Show resolved
Hide resolved
587f052 to
1cccc2d
Compare
...yamlRestTest/resources/rest-api-spec/test/search.vectors/60_dense_vector_dynamic_mapping.yml
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java
Show resolved
Hide resolved
benwtrent
left a comment
There was a problem hiding this comment.
Great work on finding the issue with the dynamic mapping and dims!
Two more comments. This PR is looking good.
server/src/main/java/org/elasticsearch/index/mapper/vectors/DenseVectorFieldMapper.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java
Outdated
Show resolved
Hide resolved
benwtrent
left a comment
There was a problem hiding this comment.
I think this is good. We got some great test coverage and it all seems ✅ .
Once all the tests are green and full-bwc-test run, I think this is good.
…1965 (elastic#101967) After elastic#98512 we incorrectly attempt to map an array of any single value type to dense_vector. Instead, we should validate that ALL mappers are numeric and that ALL of them are `float`. closes: elastic#101965
…1965 (#101967) (#101975) * Fix incorrect dynamic mapping for non-numeric-value arrays #101965 (#101967) After #98512 we incorrectly attempt to map an array of any single value type to dense_vector. Instead, we should validate that ALL mappers are numeric and that ALL of them are `float`. closes: #101965 * Update rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/60_dense_vector_dynamic_mapping.yml
Resolves #97532
Resolves #98512
Automatically maps float arrays between size of 128 and the max dense vector size to
dense_vectortype with a default similarity ofcosine.