-
Notifications
You must be signed in to change notification settings - Fork 738
SOLR-17780: Add support for scalar quantized dense vectors #3385
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
base: main
Are you sure you want to change the base?
Conversation
I still need to add tests and update documentation |
solr/core/src/java/org/apache/solr/schema/ScalarQuantizedDenseVectorField.java
Outdated
Show resolved
Hide resolved
this.confidenceInterval = Lucene99ScalarQuantizedVectorsFormat.DYNAMIC_CONFIDENCE_INTERVAL; | ||
} | ||
|
||
this.confidenceInterval = ofNullable(args.get(CONFIDENCE_INTERVAL)) |
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 overwrites the confidenceInterval set when useDynamicConfidenceInterval==true above
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 catch. I was hoping to make any explicit confidence interval override the dynamic behavior, however with both absence of arg and the default being null I think I will flip the behavior (use dynamic if specified otherwise confidence interval if specified otherwise default confidence interval if none 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.
Okay but then let's check if both are set and throw an exception.
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.
If neither are set, it should default to the default behavior (scaled confidence interval) - which according to lucene spec is passing a null confidence interval
@@ -126,16 +131,41 @@ public DocValuesFormat getDocValuesFormatForField(String field) { | |||
public KnnVectorsFormat getKnnVectorsFormatForField(String field) { | |||
final SchemaField schemaField = core.getLatestSchema().getFieldOrNull(field); | |||
FieldType fieldType = (schemaField == null ? null : schemaField.getType()); | |||
if (fieldType instanceof DenseVectorField vectorType) { | |||
KnnVectorsFormat delegate; |
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 instanceof checks can be a code smell. I look at all this and wonder: wouldn't it make sense for a method DenseVectorField.buildKnnVectorsFormat() to exist? Or just a getter if it's built in the field type's init()... which would also mean no need to have each of those fields (e.g. no bits, ...) with their getters either.
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.
reworked it. Let me know if this is along the lines of what you were thinking
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.
Much better. Did you consider init() also creating the codec to thus avoid the need corresponding fields, getters, setters?
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'm not quite sure I follow. You mean the ScalarQuantizedDenseVectorField init() or the SchemaCodecFactory init()?
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.
ScalarQuantizedDenseVectorField.init()
(and its subclass, DenseVectorField) creating a KnnVectorsFormat
that later can simply be returned.
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 probably true. However it does look like the getters are used in the DenseVectorField tests as KnnVectorsFormat doesn't expose the internal setting values. Personally I think it's ok to leave patterned as-is and having getters affords some level of flexibility and transparency.
But I can make this change if you feel strongly about it
al params and use map remove
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 believe the overall idea to be ok, having the quantised format under a dedicated new field type, given the amount of parameters make sense from a Solr code perspective (the user on the schema won't see much difference).
I left some comments, the 'vectorField.buildKnnVectorsFormat' doesn't look all right to me, for the quantised one.
I think it's just a misunderstanding of how the formats work, so it should be easily solvable.
Once tests are added I think this Pull Request would be quickly good to go!
@@ -64,6 +65,7 @@ | |||
public class DenseVectorField extends FloatPointField { | |||
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); | |||
public static final String HNSW_ALGORITHM = "hnsw"; | |||
public static final String FLAT_ALGORITHM = "flat"; |
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.
'VECTOR_STORAGE_ALGORITHM' maybe?
Is it used somewhere?
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.
reading it more we are mixing up the 'knn' algorithm (only HNSW supported right now), with the 'vector storage' (flat, scalarQuantised and binaryQuantised
public static final String BITS = "bits"; // | ||
public static final String CONFIDENCE_INTERVAL = "confidenceInterval"; | ||
public static final String DYNAMIC_CONFIDENCE_INTERVAL = "dynamicConfidenceInterval"; | ||
public static final String COMPRESS = "compress"; // can only be enabled when bits = 4 per Lucene codec spec |
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.
these first four lines are param names if I'm not mistaken.
Maybe we can call them '_PARAM' e.g. 'BITS_PARAM'
Or add a comment line at the beginning that clearly group them as param names,
it's a minor though, but can increase code readability
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.
to be fair, checking the original DenseVectorField, the same renaming could help there as well I suspect
@Override | ||
public KnnVectorsFormat buildKnnVectorsFormat() { | ||
final String knnAlgorithm = getKnnAlgorithm(); | ||
if (KNN_ALGORITHM.equals(knnAlgorithm)) { |
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.
mmmm not sure about this part:
The only knn algorithm supported right now is 'HNSW'.
While for vector storage, there's flat (what currently supported), scalar quantised and binary quantised.
Effectively in Lucene HNSW vector formats define how to build and read the HNSW graph (where each vector ID is a node).
Vector storage formats define how to store the vectors themselves.
The vector storage format is directly (and only) used by a the graph format parent ( I had in my to-do list to re-organise the package to easily differentiate graph from storage formats)
Lucene99HnswVectorsFormat uses Lucene99FlatVectorsFormat.
• Store for each field the vector metadata (.vemf) and vector data (vec).
• It has a dedicated writer and reader, dependent on the vector encoding (BYTE or
FLOAT32).
• It has its scorer (that just uses the supplied similarity).
Lucene99HnswScalarQuantizedVectorsFormat uses Lucene99ScalarQuantizedVectorsFormat (for scalar quantisation).
• Lossy compression from float32 to a number of bits (4 or 7)
• Raw disk increases (raw + quantized vectors).
• Off-heap memory decreases as quantised vectors are loaded.
• Search is faster.
• It has a dedicated writer and reader, dependent on the bits to use for quantisation.
Given that, I suspect with this contribution we want to differentiate between:
Lucene99HnswVectorsFormat and Lucene99HnswScalarQuantizedVectorsFormat (without manipulating directly the vector storage).
-> the if then else should be changed I suspect, I don't understand it as it is right now as we are mixing a graph format (Lucene99HnswScalarQuantizedVectorsFormat already using the scalar quantised storage format) and a vectore storage format (Lucene99ScalarQuantizedVectorsFormat)
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.
From an initial review the ScalarQuantizedDenseVectorField should always return the Lucene99HnswScalarQuantizedVectorsFormat.
The traditional DenseVectorField, should always return Lucene99HnswVectorsFormat
https://issues.apache.org/jira/browse/SOLR-17780
Description
Lucene 9.9 codec includes an implementation of scalar quantized dense vectors. This is not yet supported in Solr.
Solution
Tests
TODO
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.