[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456:
URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r422397622
##
File path: solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java
##
@@ -242,6 +243,7 @@ public void testBackCompatForSolrDocumentWithChildDocs()
throws IOException {
}
@Test
+ @Ignore("This test compares binaries, which change due to SOLR-13289")
Review comment:
I fixed this test.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456: URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r422255574 ## File path: solr/core/src/java/org/apache/solr/search/QueryResultKey.java ## @@ -65,6 +70,7 @@ public QueryResultKey(Query query, List filters, Sort sort, int nc_flags) h = h*29 + sf.hashCode(); ramSfields += BASE_SF_RAM_BYTES_USED + RamUsageEstimator.sizeOfObject(sf.getField()); } +h = h*31 + minExactHits; Review comment: Added a test 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456:
URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r419791807
##
File path: solr/core/src/test/org/apache/solr/search/SolrIndexSearcherTest.java
##
@@ -0,0 +1,200 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import org.apache.lucene.index.Term;
+import org.apache.lucene.search.TermQuery;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.HitCountRelation;
+import org.junit.Before;
+import org.junit.BeforeClass;
+
+import java.io.IOException;
+
+public class SolrIndexSearcherTest extends SolrTestCaseJ4 {
+
+ private final static int NUM_DOCS = 20;
+
+ @BeforeClass
+ public static void setUpClass() throws Exception {
+initCore("solrconfig.xml", "schema.xml");
+for (int i = 0 ; i < NUM_DOCS ; i ++) {
+ assertU(adoc("id", String.valueOf(i), "field1_s", "foo", "field2_s",
String.valueOf(i % 2), "field3_s", String.valueOf(i)));
+ assertU(commit());
Review comment:
Same as above, wanted multiple segments
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456:
URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r419791575
##
File path: solr/core/src/test/org/apache/solr/request/TestFaceting.java
##
@@ -931,5 +934,28 @@ public void testListedTermCounts() throws Exception {
"//lst[@name='facet_fields']/lst[@name='title_ws']/int[2][@name='Book2']",
"//lst[@name='facet_fields']/lst[@name='title_ws']/int[3][@name='Book3']");
}
+
+ @Test
+ public void testFacetCountsWithMinExactHits() throws Exception {
+final int NUM_DOCS = 20;
+for (int i = 0; i < NUM_DOCS ; i++) {
+ assertU(adoc("id", String.valueOf(i), "title_ws", "Book1"));
+ assertU(commit());
Review comment:
Actually, I wanted to have multiple segments. I could do something like
"sometimes()", but since the number of docs is low, I didn't think it was
needed to add any randomization or more complex logic.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456: URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r418772556 ## File path: solr/core/src/java/org/apache/solr/response/JSONWriter.java ## @@ -132,11 +133,16 @@ public void writeSolrDocument(String name, SolrDocument doc, ReturnFields return // that the size could not be reliably determined. // + /** + * This method will be removed in Solr 9 Review comment: I really don't think we need to keep this around longer than 9.0. People upgrading a major version expect to have to change some code. This is a trivial change, they'll get a compile-time error and notice there is a new parameter added. This doesn't even change the client, just plugins. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456:
URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r418756851
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -401,6 +403,14 @@ public void process(ResponseBuilder rb) throws IOException
doProcessUngroupedSearch(rb, cmd, result);
}
+ private int getMinExactHits(SolrParams params) {
+long minExactHits = params.getLong(CommonParams.MIN_EXACT_HITS,
Integer.MAX_VALUE);
+if (minExactHits < 0 || minExactHits > Integer.MAX_VALUE) {
Review comment:
This is intentional. See the discussion with Adrien in this same PR. We
allow longs just because the `minExactHits` is in relation to the `numFound`,
which is a long, however, any value greater than `Integer.MAX_VALUE` doesn't
make sense, since Lucene doesn't allow more than that in a single shard. In
case of distributed queries, the `minExactHits` is used by every shard
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456: URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r418752582 ## File path: solr/core/src/java/org/apache/solr/search/QueryResultKey.java ## @@ -65,6 +70,7 @@ public QueryResultKey(Query query, List filters, Sort sort, int nc_flags) h = h*29 + sf.hashCode(); ramSfields += BASE_SF_RAM_BYTES_USED + RamUsageEstimator.sizeOfObject(sf.getField()); } +h = h*31 + minExactHits; Review comment: ah! Good question. I guess yes? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456:
URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r418750165
##
File path: solr/core/src/java/org/apache/solr/search/MaxScoreCollector.java
##
@@ -37,9 +37,7 @@ public float getMaxScore() {
@Override
public ScoreMode scoreMode() {
-// Should be TOP_SCORES but this would wrap the scorer unnecessarily since
-// this collector is only used in a MultiCollector.
-return ScoreMode.COMPLETE;
+return ScoreMode.TOP_SCORES;
Review comment:
There is a previous discussion in this PR about it:
> tflobbe:
> @jpountz, What did you mean with this comment? MultiCollector will set the
scoreMode to COMPLETE in the case of the main collector being something other
than TOP_SCORES, right?
> jpountz:
> Yeah I think that at some point we tried not to wrap the scorer when all
score modes were COMPLETE but apparently now we do, so this comment is stale.
https://github.com/apache/lucene-solr/blob/master/lucene/core/src/java/org/apache/lucene/search/MultiCollector.java#L158
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456: URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r418749058 ## File path: solr/core/src/java/org/apache/solr/response/JSONWriter.java ## @@ -132,11 +133,16 @@ public void writeSolrDocument(String name, SolrDocument doc, ReturnFields return // that the size could not be reliably determined. // + /** + * This method will be removed in Solr 9 Review comment: My understanding is that it's OK to change methods across major versions? Unless it's something needed for rolling upgrade or something. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456:
URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r418748102
##
File path:
solr/core/src/java/org/apache/solr/response/GeoJSONResponseWriter.java
##
@@ -292,11 +293,18 @@ else if(geo instanceof WriteableGeoJSON) {
}
}
+ @Deprecated
@Override
public void writeStartDocumentList(String name,
- long start, int size, long numFound, Float maxScore) throws IOException
+ long start, int size, long numFound, Float maxScore) throws IOException {
+throw new UnsupportedOperationException();
Review comment:
This method Shouldn't be called at all for writing a response. In other
ResponseWriters, I left the code untouched for support in case someone is
extending the Writer. In this case, since it's an inner class, I decided to
throw an exception to catch potential bugs (internal code calling this)
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456:
URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r416956723
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -401,6 +403,14 @@ public void process(ResponseBuilder rb) throws IOException
doProcessUngroupedSearch(rb, cmd, result);
}
+ private int getMinExactHits(SolrParams params) {
+long minExactHits = params.getLong(CommonParams.MIN_EXACT_HITS,
Integer.MAX_VALUE);
Review comment:
I plan to move the default value to a solrconfig config in a future PR,
for now, MAX_VALUE (disabled)
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] tflobbe commented on a change in pull request #1456: SOLR-13289: Support for BlockMax WAND
tflobbe commented on a change in pull request #1456:
URL: https://github.com/apache/lucene-solr/pull/1456#discussion_r416796194
##
File path: solr/core/src/java/org/apache/solr/search/MaxScoreCollector.java
##
@@ -39,7 +39,7 @@ public float getMaxScore() {
public ScoreMode scoreMode() {
// Should be TOP_SCORES but this would wrap the scorer unnecessarily since
// this collector is only used in a MultiCollector.
-return ScoreMode.COMPLETE;
+return ScoreMode.TOP_SCORES;
Review comment:
@jpountz, What did you mean with this comment? MultiCollector will set
the `scoreMode` to `COMPLETE` in the case of the main collector being something
other than `TOP_SCORES`, right?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
