You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/12/29 15:59:20 UTC
[GitHub] [solr] alessandrobenedetti opened a new pull request, #1255: Fix/solr 16588 set default knn algorithm
alessandrobenedetti opened a new pull request, #1255:
URL: https://github.com/apache/solr/pull/1255
https://issues.apache.org/jira/browse/SOLR-16588
# Description
Bugfix for Hnsw algorithm
# Solution
Better param validation
# Tests
minor test reformat
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] henrik242 commented on a diff in pull request #1255: SOLR-16588: set default knn algorithm
Posted by GitBox <gi...@apache.org>.
henrik242 commented on code in PR #1255:
URL: https://github.com/apache/solr/pull/1255#discussion_r1063397791
##########
solr/core/src/java/org/apache/solr/core/SchemaCodecFactory.java:
##########
@@ -124,18 +124,18 @@ public KnnVectorsFormat getKnnVectorsFormatForField(String field) {
if (fieldType instanceof DenseVectorField) {
DenseVectorField vectorType = (DenseVectorField) fieldType;
String knnAlgorithm = vectorType.getKnnAlgorithm();
- if (knnAlgorithm != null) {
- if (knnAlgorithm.equals(DenseVectorField.HNSW_ALGORITHM)) {
- int maxConn = vectorType.getHnswMaxConn();
- int beamWidth = vectorType.getHnswBeamWidth();
- return new Lucene94HnswVectorsFormat(maxConn, beamWidth);
- }
+ if (DenseVectorField.HNSW_ALGORITHM.equals(knnAlgorithm)) {
+ int maxConn = vectorType.getHnswMaxConn();
+ int beamWidth = vectorType.getHnswBeamWidth();
+ return new Lucene94HnswVectorsFormat(maxConn, beamWidth);
Review Comment:
I would assume so! :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti merged pull request #1255: SOLR-16588: set default knn algorithm
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti merged PR #1255:
URL: https://github.com/apache/solr/pull/1255
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] henrik242 commented on a diff in pull request #1255: SOLR-16588: set default knn algorithm
Posted by GitBox <gi...@apache.org>.
henrik242 commented on code in PR #1255:
URL: https://github.com/apache/solr/pull/1255#discussion_r1062245190
##########
solr/core/src/java/org/apache/solr/core/SchemaCodecFactory.java:
##########
@@ -124,18 +124,18 @@ public KnnVectorsFormat getKnnVectorsFormatForField(String field) {
if (fieldType instanceof DenseVectorField) {
DenseVectorField vectorType = (DenseVectorField) fieldType;
String knnAlgorithm = vectorType.getKnnAlgorithm();
- if (knnAlgorithm != null) {
- if (knnAlgorithm.equals(DenseVectorField.HNSW_ALGORITHM)) {
- int maxConn = vectorType.getHnswMaxConn();
- int beamWidth = vectorType.getHnswBeamWidth();
- return new Lucene94HnswVectorsFormat(maxConn, beamWidth);
- }
+ if (DenseVectorField.HNSW_ALGORITHM.equals(knnAlgorithm)) {
+ int maxConn = vectorType.getHnswMaxConn();
+ int beamWidth = vectorType.getHnswBeamWidth();
+ return new Lucene94HnswVectorsFormat(maxConn, beamWidth);
Review Comment:
With regards to including this in Solr: 9.1.1: Doesn't `Lucene94HnswVectorsFormat` require Lucene 9.4? 9.1.1 will stay on Lucene 9.3 afaik.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on pull request #1255: SOLR-16588: set default knn algorithm
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on PR #1255:
URL: https://github.com/apache/solr/pull/1255#issuecomment-1371444495
thanks @risdenk ! All committed, adding the Changes.txt as soon as I know this is coming in 9.1.1!
Cheers
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] alessandrobenedetti commented on a diff in pull request #1255: SOLR-16588: set default knn algorithm
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on code in PR #1255:
URL: https://github.com/apache/solr/pull/1255#discussion_r1062893565
##########
solr/core/src/java/org/apache/solr/core/SchemaCodecFactory.java:
##########
@@ -124,18 +124,18 @@ public KnnVectorsFormat getKnnVectorsFormatForField(String field) {
if (fieldType instanceof DenseVectorField) {
DenseVectorField vectorType = (DenseVectorField) fieldType;
String knnAlgorithm = vectorType.getKnnAlgorithm();
- if (knnAlgorithm != null) {
- if (knnAlgorithm.equals(DenseVectorField.HNSW_ALGORITHM)) {
- int maxConn = vectorType.getHnswMaxConn();
- int beamWidth = vectorType.getHnswBeamWidth();
- return new Lucene94HnswVectorsFormat(maxConn, beamWidth);
- }
+ if (DenseVectorField.HNSW_ALGORITHM.equals(knnAlgorithm)) {
+ int maxConn = vectorType.getHnswMaxConn();
+ int beamWidth = vectorType.getHnswBeamWidth();
+ return new Lucene94HnswVectorsFormat(maxConn, beamWidth);
Review Comment:
You are absolutely right!
Can't we then cherry-pick and change it to Lucene93HnswVectorsFormat for the 9.1.1 branch?
I think it's a nice to have bugfix!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1255: Fix/solr 16588 set default knn algorithm
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1255:
URL: https://github.com/apache/solr/pull/1255#discussion_r1061858513
##########
solr/core/src/java/org/apache/solr/core/SchemaCodecFactory.java:
##########
@@ -124,18 +124,18 @@ public KnnVectorsFormat getKnnVectorsFormatForField(String field) {
if (fieldType instanceof DenseVectorField) {
DenseVectorField vectorType = (DenseVectorField) fieldType;
String knnAlgorithm = vectorType.getKnnAlgorithm();
- if (knnAlgorithm != null) {
- if (knnAlgorithm.equals(DenseVectorField.HNSW_ALGORITHM)) {
- int maxConn = vectorType.getHnswMaxConn();
- int beamWidth = vectorType.getHnswBeamWidth();
- return new Lucene94HnswVectorsFormat(maxConn, beamWidth);
- }
+ if (knnAlgorithm.equals(DenseVectorField.HNSW_ALGORITHM)) {
Review Comment:
nit the following will avoid any NPE if `knnAlgorithm` is ever null.:
```suggestion
if (DenseVectorField.HNSW_ALGORITHM.equals(knnAlgorithm)) {
```
##########
solr/core/src/java/org/apache/solr/schema/DenseVectorField.java:
##########
@@ -104,7 +106,7 @@ public void init(IndexSchema schema, Map<String, String> args) {
.orElse(DEFAULT_SIMILARITY);
args.remove(KNN_SIMILARITY_FUNCTION);
- this.knnAlgorithm = args.get(KNN_ALGORITHM);
+ this.knnAlgorithm = Optional.ofNullable(args.get(KNN_ALGORITHM)).orElse(DEFAULT_KNN_ALGORITHM);
Review Comment:
use getOrDefault? https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#getOrDefault-java.lang.Object-V-
```suggestion
this.knnAlgorithm = args.getOrDefault(KNN_ALGORITHM, DEFAULT_KNN_ALGORITHM);
```
##########
solr/core/src/java/org/apache/solr/schema/DenseVectorField.java:
##########
@@ -104,7 +106,7 @@ public void init(IndexSchema schema, Map<String, String> args) {
.orElse(DEFAULT_SIMILARITY);
args.remove(KNN_SIMILARITY_FUNCTION);
- this.knnAlgorithm = args.get(KNN_ALGORITHM);
+ this.knnAlgorithm = Optional.ofNullable(args.get(KNN_ALGORITHM)).orElse(DEFAULT_KNN_ALGORITHM);
Review Comment:
This should cleanup the imports too.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org