-
Notifications
You must be signed in to change notification settings - Fork 25.5k
[ES|QL] Optimize vector similarity when one param is constant #135602
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
[ES|QL] Optimize vector similarity when one param is constant #135602
Conversation
…ctor_similarity_when_constant
…github.com:pmpailis/elasticsearch into pb_134210_optimize_vector_similarity_when_constant
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 LGTM - a question about creating multiple subclasses
public final EvalOperator.ExpressionEvaluator.Factory toEvaluator(EvaluatorMapper.ToEvaluator toEvaluator) { | ||
VectorValueProvider.Builder leftVectorProviderBuilder = new VectorValueProvider.Builder(); | ||
VectorValueProvider.Builder rightVectorProviderBuilder = new VectorValueProvider.Builder(); | ||
if (left() instanceof Literal && left().dataType() == DENSE_VECTOR) { |
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.
I think you can count on the arguments being type dense_vector, it is already checked as part of checkDenseVectorParam()
and would have been caught at the Analyzer level
private FloatBlock block; | ||
private float[] scratch; | ||
|
||
VectorValueProvider(ArrayList<Float> constantVector, EvalOperator.ExpressionEvaluator expressionEvaluator) { |
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.
Good idea to use a class to wrap the different ways of retrieving vectors!
Would it make sense to create two separate subclasses, so we don't need to keep checking for what has been provided?
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.
That's a nice suggestion and will make things easier to follow, thanks! Will refactor to have 2 subclasses.
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
assumeTrue("Similarity function is not enabled", capability().isEnabled()); | ||
} | ||
|
||
public abstract String getBaseEvaluatorName(); |
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.
@carlosdelest trying to find a better way to access this as we already pass it in the similarityParameters
, but it was getting a bit too complicated for a small-scoped change, as the PR only affects the similarity functions. Happy to discuss alternatives :)
run elasticsearch-ci/part-3 |
|
||
/** | ||
* Fields with this type are dense vectors, represented as an array of double values. | ||
* Fields with this type are dense vectors, represented as an array of float values. |
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.
😁 thanks for fixing!
} | ||
|
||
@Override | ||
public String toString() { |
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.
What do you think about creating a different factory for each type of VectorValueProvider, both of them implementing the same interface?
That would help with not having to check which of the two fields need to be used. Each of the classes would have its build() and toString() methods.
I think it makes sense as the factories create two different objects - makes sense to make them different.
Maybe it¡s a bit convoluted for what we're doing but probably cleaner. WDYT?
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.
++ makes sense. Didn't want to have a ton of new factories/interfaces, but it will be cleaner. Will refactor as suggested.
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 in ee019f2
…github.com:pmpailis/elasticsearch into pb_134210_optimize_vector_similarity_when_constant
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.
Great work! Your first PR achieved such a speedup 🚀
public final EvalOperator.ExpressionEvaluator.Factory toEvaluator(EvaluatorMapper.ToEvaluator toEvaluator) { | ||
VectorValueProviderFactory leftVectorProviderFactory; | ||
VectorValueProviderFactory rightVectorProviderFactory; | ||
if (left() instanceof Literal) { |
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.
Nit - we could extract this to a private method
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 in 9f47ab2
This PR aims to optimize the way we handle constant vectors in ES|QL vector similarity functions. The main idea is to reuse a single object and avoid duplicating and creating huge
FloatBlock
instances and identical float arrays.Benchmark results show a nice improvement for the script-score functions using the
so_vector
rally track:Closes #134210