- Notifications
You must be signed in to change notification settings - Fork 25.5k
Add basic implementations of float-byte script comparisons #122381
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
| ||
@Override | ||
public float ipFloatByte(float[] q, byte[] d) { | ||
return DefaultESVectorUtilSupport.ipFloatByteImpl(q, d); |
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.
Panama implementation can be added as a separate PR
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.
See #123270
We could do this the other way too - add implementations of |
Hi @thecoop, I've created a changelog YAML for you. |
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
I think this looks good as a first step. Could you add some tests here: modules/lang-painless/src/yamlRestTest/resources/rest-api-spec/test/painless/145_dense_vector_byte_basic.yml Obviously, a component will need to be added. But it will help increase our coverage for free. |
...nless/src/yamlRestTest/resources/rest-api-spec/test/painless/145_dense_vector_byte_basic.yml Outdated Show resolved Hide resolved
More heterogenous methods can be introduced in later PRs using this infrastructure |
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 great! I have one concern about cosine.
server/src/main/java/org/elasticsearch/script/VectorScoreScriptUtils.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/script/VectorScoreScriptUtils.java Outdated Show resolved Hide resolved
4374e6c
to 1d99668
Compare 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.
Almost there :). I think this latest change does a little bit too much work, its fine to keep the previous magnitude calculation as the individual component values do not change (we simply cast them).
server/src/main/java/org/elasticsearch/script/VectorScoreScriptUtils.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/script/VectorScoreScriptUtils.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/script/VectorScoreScriptUtils.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/script/VectorScoreScriptUtils.java Outdated Show resolved Hide resolved
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.
❤️
Add comparisons of
float[]
queries withbyte[]
vectorsThis fixes #117274