ilariapet commented on code in PR #5246:
URL: https://github.com/apache/solr/pull/5246#discussion_r4399709776
##########
solr/core/src/java/org/apache/solr/schema/DenseVectorField.java:
##########
@@ -341,6 +344,30 @@ public void checkSchemaField(final SchemaField field)
throws SolrException {
}
break;
}
+
+ try {
+ // format does not expose any validation, however by constructing
+ // we will run lucene level argument validation
+ buildKnnVectorsFormat();
+ } catch (IllegalArgumentException e) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ getClass().getSimpleName()
+ + " "
+ + toSolrParamNames(e.getMessage())
+ + ": "
+ + field.getName(),
+ e);
+ }
+ }
+
+ /** Replaces Lucene's HNSW argument names with the corresponding Solr
parameter names. */
Review Comment:
Rewriting Lucene's error message with regexes works, but what happens if
Lucene changes its wording in the future? The regexes would no longer match, so
users would see Lucene's parameter names again, and our tests would fail
(invalid values would still be rejected, only the error message would be
affected).
##########
solr/solr-ref-guide/modules/query-guide/pages/dense-vector-search.adoc:
##########
@@ -310,6 +315,8 @@ As a specific type of DenseVectorField, this field type
supports `knnAlgorithm`
NOTE: Scalar quantization applies to `FLOAT32` vectors only.
`vectorEncoding="BYTE"` vectors are stored unquantized, and with
`knnAlgorithm="flat"` they do not support the KNN query parsers. Use
`vectorSimilarity()` function queries instead.
+NOTE: Quantization requires the default
xref:configuration-guide:codec-factory.adoc#solr-schemacodecfactory-default[`SchemaCodecFactory`].
With `LuceneDefaultCodecFactory` or `SimpleTextCodecFactory`, this field type
is accepted but its vectors are stored unquantized, without any warning.
Review Comment:
`Quantization requires the default SchemaCodecFactory`
Isn't this a bit unclear? It may suggest that SchemaCodecFactory is the only
codec factory that supports quantization. Could we rephrase it?
##########
solr/solr-ref-guide/modules/query-guide/pages/dense-vector-search.adoc:
##########
@@ -100,12 +101,14 @@ Accepted values:
* `euclidean`: https://en.wikipedia.org/wiki/Euclidean_distance[Euclidean
distance]
* `cosine`: https://en.wikipedia.org/wiki/Cosine_similarity[Cosine similarity]
* `dot_product`: https://en.wikipedia.org/wiki/Dot_product[Dot product]
+* `maximum_inner_product`: dot product without the unit length requirement;
use it when the vector magnitude carries information.
[NOTE]
====
* the preferred way to perform `cosine` similarity is to normalize all vectors
to unit length, and instead use `doc_product`. You should only specify `cosine`
if you need to preserve the original vectors and cannot normalize them in
advance.
Review Comment:
typo: `doc_product` instead of `dot_product`
##########
solr/solr-ref-guide/modules/configuration-guide/pages/codec-factory.adoc:
##########
@@ -59,6 +59,23 @@ Example:
</codecFactory>
----
+=== org.apache.solr.cuvs.CuVSCodecFactory
+
+`CuVSCodecFactory`, provided by the `cuvs` module, uses an NVIDIA GPU to build
the HNSW graph of `DenseVectorField` fields configured with
`knnAlgorithm="cagra_hnsw"`.
+All other fields are handled as by `solr.SchemaCodecFactory`, and accepts the
same options.
Review Comment:
Does this also apply to DenseVectorField fields with `knnAlgorithm="flat"`?
From reading `CuVSCodec.getKnnVectorsFormatForField()`, it seems only hnsw
fields are delegated, while other algorithms such as flat are rejected with
"<algorithm> KNN algorithm is not supported". Should we mention this exception?
Also in `…and accepts the same options`, the subject seems to be "All other
fields". Could we make it clearer that the subject is the codec factory?
##########
solr/solr-ref-guide/modules/query-guide/pages/dense-vector-search.adoc:
##########
@@ -174,10 +177,12 @@ It is the number of nearest neighbor candidates to track
while searching the gra
For more details, refer to the official https://arxiv.org/pdf/1603.09320[2018
paper], where this parameter `efConstruction` is defined.
+
Accepted values:
-Any integer.
+An integer between `1` and `3200`.
`DenseVectorField` supports the attributes: `indexed`, `stored`, `multivalued`.
+A `DenseVectorField` can be the source of a `copyField` only if the
destination is also a dense vector field with the same `vectorDimension`;
otherwise indexing fails. `copyField` rules with `source="*"` skip vector
fields.
Review Comment:
What do you think about `"...source="*" silently skips dense vector fields
instead of failing."`
##########
solr/solr-ref-guide/modules/query-guide/pages/dense-vector-search.adoc:
##########
@@ -325,7 +332,13 @@ s|Optional |Default: `7`
+
The number of bits to use for each quantized dimension value
+
-Accepted values: 4 (half byte) or 7 (unsigned byte).
+Accepted values:
++
+* `1`: one bit per dimension. It is the same encoding used by
`BinaryQuantizedDenseVectorField`.
+* `2`: two bits per dimension.
+* `4`: half a byte per dimension.
+* `7`: seven bits per dimension (signed byte).
+* `8`: one unsigned byte per dimension.
Review Comment:
What about mentioning the difference between 7 and 8?
https://github.com/apache/lucene/blob/9983b7ce7fdd04f4d357688fb85c14277c15ea8d/lucene/core/src/java/org/apache/lucene/codecs/lucene104/Lucene104ScalarQuantizedVectorsFormat.java#L119-L131
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
