You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/01/07 12:27:31 UTC

[GitHub] [lucene-solr] gerlowskija opened a new pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

gerlowskija opened a new pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151

# Description

The "docValuesTermsFilter" method for TermsQParserPlugin uses per-segment docvalue structures to find matches. This works well when the number of query terms is small, but as it gets over a few hundred, this quickly becomes less efficient than using a "top-level" or global docvalues structure.

# Solution

This PR introduces a query that uses the top-level structure and takes advantage of the Two Phase Iterator (TPI) interface to handle queries with large numbers of terms much more efficiently than any of the existing methods.

# Tests

This PR adds `TestTermsQParserPlugin` with some tests to cover the new code added here as well as the rest of the terms qparser (which was largely untested). I also wrote a JUnit performance driver to exhibit perf gains, but that no longer appears in this PR. See patches on the JIRA issue for that.

# Checklist

Please review the following and check all that apply:

- [x] 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.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] 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)
- [x] I have developed this patch against the `master` branch.
- [x] I have run `ant precommit` and the appropriate test suite.
- [x] I have added tests for my changes.
- [x] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).

----------------------------------------------------------------
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:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364302655
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -142,4 +158,112 @@ public Query parse() throws SyntaxError {
       }
     };
   }
+
+  private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery implements ExtendedQuery{
+    private final String fieldName;
+    private SortedSetDocValues topLevelDocValues;
+    private LongBitSet topLevelTermOrdinals;
+    private boolean matchesAtLeastOneTerm = false;
+    private boolean cache = true;
+    private boolean cacheSeparately = false;
+    private int cost;
+
+
+    public TopLevelDocValuesTermsQuery(String field, BytesRef... terms) {
+      super(field, terms);
+      this.fieldName = field;
+    }
+
+    public Weight createWeight(IndexSearcher searcher, final ScoreMode scoreMode, float boost) throws IOException {
+      topLevelDocValues = DocValues.getSortedSet(((SolrIndexSearcher)searcher).getSlowAtomicReader(), fieldName);
 
 Review comment:
   If not a SolrIndexSearcher than return super.createWeight    (your idea based on Mikhail's comment)

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364299857

##########
File path: solr/solr-ref-guide/src/other-parsers.adoc
##########
@@ -1031,13 +1031,17 @@ The field on which to search. This parameter is required.
Separator to use when parsing the input. If set to " " (a single blank space), will trim additional white space from the input terms. Defaults to a comma (`,`).

`method`::
-An optional parameter used to determine which of several query implementations should be used by Solr. Options are restricted to: `termsFilter`, `booleanQuery`, `automaton`, or `docValuesTermsFilter`. If unspecified, the default value is `termsFilter`. Each implementation has its own performance characteristics, and users are encouraged to experiment to determine which implementation is most performant for their use-case. Heuristics are given below.
+An optional parameter used to determine which of several query implementations should be used by Solr. Options are restricted to: `termsFilter`, `booleanQuery`, `automaton`, `docValuesTermsFilterPerSegment`, `docValuesTermsFilterTopLevel` or `docValuesTermsFilter`. If unspecified, the default value is `termsFilter`. Each implementation has its own performance characteristics, and users are encouraged to experiment to determine which implementation is most performant for their use-case. Heuristics are given below.
+
`booleanQuery` creates a `BooleanQuery` representing the request. Scales well with index size, but poorly with the number of terms being searched for.
+
`termsFilter` the default `method`. Uses a `BooleanQuery` or a `TermInSetQuery` depending on the number of terms. Scales well with index size, but only moderately with the number of query terms.
+
-`docValuesTermsFilter` uses doc values data structures to process the request. This method scales well to a large numbers of query terms. It encompasses two implementations or submethods. Solr uses heuristics to choose between these at runtime, but users can also pick explicitly by providing a `submethod` parameter with either `toplevel` or `persegment` as a value. The `persegment` implementation is more general purpose, while `toplevel` is geared for anyone with particularly high numbers of query terms (several hundred to several thousand). The `toplevel` submethod relies on data structures which are lazily populated after each commit. If you use this submethod and commit frequently, you may benefit from adding a static warming query to `solrconfig.xml` so that this is done as a part of the commit, and doesn't slow down user requests.
+`docValuesTermsFilter` chooses between the `docValuesTermsFilterTopLevel` and `docValuesTermsFilterPerSegment` methods (see below) using the number of query terms as a rough heuristic. Users should typically use this method instead of using `docValuesTermsFilterTopLevel` or `docValuesTermsFilterPerSegment` directly, unless they've done performance testing to validate one or the other methods on queries of all sizes. Depending on the implementation picked, this method may rely on expensive data structures which are lazily populated after each commit. If you commit frequently, you may benefit from adding a static warming query to `solrconfig.xml` so that this is done as a part of the commit itself and not attached directly to user requests.

Review comment:
Maybe right up front here first declare that this method is only appropriate when the field has docValues? That's more important and differentiates from terms index based methods.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] ctargett commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

ctargett commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365406299

##########
File path: solr/solr-ref-guide/src/other-parsers.adoc
##########
@@ -1037,11 +1037,11 @@ An optional parameter used to determine which of several query implementations s
+
`termsFilter` the default `method`. Uses a `BooleanQuery` or a `TermInSetQuery` depending on the number of terms. Scales well with index size, but only moderately with the number of query terms.
+
-`docValuesTermsFilter` can only be used on fields with docValues data. Chooses between the `docValuesTermsFilterTopLevel` and `docValuesTermsFilterPerSegment` methods using the number of query terms as a rough heuristic. Users should typically use this method instead of using `docValuesTermsFilterTopLevel` or `docValuesTermsFilterPerSegment` directly, unless they've done performance testing to validate one of the methods on queries of all sizes. Depending on the implementation picked, this method may rely on expensive data structures which are lazily populated after each commit. If you commit frequently and your use-case can tolerate a static warming query, consider adding one to `solrconfig.xml` so that this work is done as a part of the commit itself and not attached directly to user requests.
+`docValuesTermsFilter` can only be used on fields with docValues data. The `cache` parameter is false by default. Chooses between the `docValuesTermsFilterTopLevel` and `docValuesTermsFilterPerSegment` methods using the number of query terms as a rough heuristic. Users should typically use this method instead of using `docValuesTermsFilterTopLevel` or `docValuesTermsFilterPerSegment` directly, unless they've done performance testing to validate one of the methods on queries of all sizes. Depending on the implementation picked, this method may rely on expensive data structures which are lazily populated after each commit. If you commit frequently and your use-case can tolerate a static warming query, consider adding one to `solrconfig.xml` so that this work is done as a part of the commit itself and not attached directly to user requests.

Review comment:
When I think of it, I've been changing to one sentence per line when I'm editing also (but I often forget). I don't know that it really changes that much to have a whole page consistent - we should probably encourage one sentence per line, but if it's a big file and you can only do a little, I think that's fine and maybe someone else will get inspired later to do the rest.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365315479
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -88,7 +91,20 @@ Query makeFilter(String fname, BytesRef[] byteRefs) {
     docValuesTermsFilter {//on 4x this is FieldCacheTermsFilter but we use the 5x name any way
       @Override
       Query makeFilter(String fname, BytesRef[] byteRefs) {
-        return new DocValuesTermsQuery(fname, byteRefs);//constant scores
+        // TODO Further tune this heuristic number
+        return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
+      }
+    },
+    docValuesTermsFilterTopLevel {
+      @Override
+      Query makeFilter(String fname, BytesRef[] byteRefs) {
+        return new TopLevelDocValuesTermsQuery(fname, byteRefs);
 
 Review comment:
   I'd forgotten about this, done.
   
   FWIW I buy Joel's argument that most real-world use-cases are going to require `cache=false`.  But I worry that users who want caching will be bitten by this.  I guess it depends how hands-on you expect users to be with their query tuning.  For hands-off users, this change is best. But for hands-on users, we've just created a "gotcha".
   
   I'll make sure to add this to the ref-guide docs at the least.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363740102
 
 

 ##########
 File path: lucene/core/src/java/org/apache/lucene/search/TwoPhaseIterator.java
 ##########
 @@ -64,7 +64,7 @@ public static TwoPhaseIterator unwrap(DocIdSetIterator iterator) {
 
     TwoPhaseIteratorAsDocIdSetIterator(TwoPhaseIterator twoPhaseIterator) {
       this.twoPhaseIterator = twoPhaseIterator;
-      this.approximation = twoPhaseIterator.approximation;
+      this.approximation = twoPhaseIterator.approximation();
 
 Review comment:
   Is this just a minor improvement or is it necessary?  If just some minor improvement, I recommend you don't touch Lucene in a Solr PR.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363740267
 
 

 ##########
 File path: solr/CHANGES.txt
 ##########
 @@ -187,6 +187,11 @@ Improvements
   hl.fragsizeIsMinimum, with defaults that aim to better center matches in fragments than previously. See the ref guide.
   Regardless of the settings, the passages may be sized differently than before. (Nándor Mátravölgyi, David Smiley)
 
+* SOLR-13890: Add "top-level" DV implementation for {!terms} queries.  This approach tends to be more efficient for
+  queries withvery large numbers of terms.  The new implementation is used by default for method=docValuesTermsFilter
 
 Review comment:
   "withvery" -- add space.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365066879
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -88,7 +91,20 @@ Query makeFilter(String fname, BytesRef[] byteRefs) {
     docValuesTermsFilter {//on 4x this is FieldCacheTermsFilter but we use the 5x name any way
       @Override
       Query makeFilter(String fname, BytesRef[] byteRefs) {
-        return new DocValuesTermsQuery(fname, byteRefs);//constant scores
+        // TODO Further tune this heuristic number
+        return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
+      }
+    },
+    docValuesTermsFilterTopLevel {
+      @Override
+      Query makeFilter(String fname, BytesRef[] byteRefs) {
+        return new TopLevelDocValuesTermsQuery(fname, byteRefs);
 
 Review comment:
   I believe Joel argued strongly we should default both DV impls to cache=false?  You can do that here (and in the other method) easily via WrappedQuery wrapper and setting cache=false.  I mentioned another approach that messes with this.localParams but perhaps that might feel hacky compared to WrappedQuery.  Especially so since makeFilter doesn't have access to change the field this.localParams.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364298797

Review comment:
The advice about commit frequently and thus add a static warming query is very debatable. "commit frequently" ~= NRT and NRT users want to dial down caching, even eliminate. But but maybe they can tolerate longer latency for freshness provided the query response time isn't incurring costs post-commit. There's no universal answer. Yet you did say "you *may* benefit" so I guess what you wrote is fine.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363745450
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -138,8 +164,128 @@ public Query parse() throws SyntaxError {
           bytesRefs[i] = term.toBytesRef();
         }
 
-        return method.makeFilter(fname, bytesRefs);
+        return method.makeFilter(fname, bytesRefs, localParams);
       }
     };
   }
+
+  private static abstract class TopLevelTwoPhaseIterator extends TwoPhaseIterator {
 
 Review comment:
   I don't think you need this class; it seems more confusing than it is useful

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364192491
 
 

 ##########
 File path: lucene/core/src/java/org/apache/lucene/search/TwoPhaseIterator.java
 ##########
 @@ -64,7 +64,7 @@ public static TwoPhaseIterator unwrap(DocIdSetIterator iterator) {
 
     TwoPhaseIteratorAsDocIdSetIterator(TwoPhaseIterator twoPhaseIterator) {
       this.twoPhaseIterator = twoPhaseIterator;
-      this.approximation = twoPhaseIterator.approximation;
+      this.approximation = twoPhaseIterator.approximation();
 
 Review comment:
   It was necessary with the TopLevelTwoPhaseIterator approach in previous versions of this patch, but is not necessary now.  Removing.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] mkhludnev commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

mkhludnev commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363774049
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -138,8 +164,128 @@ public Query parse() throws SyntaxError {
           bytesRefs[i] = term.toBytesRef();
         }
 
-        return method.makeFilter(fname, bytesRefs);
+        return method.makeFilter(fname, bytesRefs, localParams);
       }
     };
   }
+
+  private static abstract class TopLevelTwoPhaseIterator extends TwoPhaseIterator {
+    private final int docBase;
+    private final int nextDocBase;
+    public TopLevelTwoPhaseIterator(DocIdSetIterator approximation, int docBase, int nextDocBase) {
+      super(new PerSegmentViewDocIdSetIterator(approximation, docBase, nextDocBase));
+
+      this.docBase = docBase;
+      this.nextDocBase = nextDocBase;
+    }
+  }
+
+  private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery implements ExtendedQuery{
+    private final String fieldName;
+    private SortedSetDocValues values;
+    private LongBitSet queryTermOrdinals;
+    private boolean matchesAtLeastOneTerm = false;
+    private boolean cache = true;
+    private boolean cacheSeparately = false;
+    private int cost;
+
+
+    public TopLevelDocValuesTermsQuery(String field, BytesRef... terms) {
+      super(field, terms);
+      this.fieldName = field;
+    }
+
+    public Weight createWeight(IndexSearcher searcher, final ScoreMode scoreMode, float boost) throws IOException {
+      values = DocValues.getSortedSet(((SolrIndexSearcher)searcher).getSlowAtomicReader(), fieldName);
 
 Review comment:
   Another naming proposal is ```global_ordinals```.
    

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363745034
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -102,6 +125,9 @@ public QParser createParser(String qstr, SolrParams localParams, SolrParams para
       public Query parse() throws SyntaxError {
         String fname = localParams.get(QueryParsing.F);
         FieldType ft = req.getSchema().getFieldTypeNoEx(fname);
+        if (ft == null) {
+          throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Field name [" + fname + "] does not exist");
 
 Review comment:
   Instead the line above "NoEx" could be removed and then we get an exception

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363744702
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -79,20 +88,34 @@ Query makeFilter(String fname, BytesRef[] byteRefs) {
     },
     automaton {
       @Override
-      Query makeFilter(String fname, BytesRef[] byteRefs) {
+      Query makeFilter(String fname, BytesRef[] byteRefs, SolrParams localParams) {
         ArrayUtil.timSort(byteRefs); // same sort algo as TermInSetQuery's choice
         Automaton union = Automata.makeStringUnion(Arrays.asList(byteRefs)); // input must be sorted
         return new AutomatonQuery(new Term(fname), union);//constant scores
       }
     },
     docValuesTermsFilter {//on 4x this is FieldCacheTermsFilter but we use the 5x name any way
       @Override
-      Query makeFilter(String fname, BytesRef[] byteRefs) {
-        return new DocValuesTermsQuery(fname, byteRefs);//constant scores
+      Query makeFilter(String fname, BytesRef[] byteRefs, SolrParams localParams) {
+        final String type = localParams.get(SUBMETHOD);
 
 Review comment:
   Instead of yet another local-param, maybe "method" could be docValuesTermsFilterTopLevel and docValuesTermsFilterTopLevel.  The existing docValuesTermsFilter could dispatch to either of those.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363758827
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -138,8 +164,128 @@ public Query parse() throws SyntaxError {
           bytesRefs[i] = term.toBytesRef();
         }
 
-        return method.makeFilter(fname, bytesRefs);
+        return method.makeFilter(fname, bytesRefs, localParams);
       }
     };
   }
+
+  private static abstract class TopLevelTwoPhaseIterator extends TwoPhaseIterator {
+    private final int docBase;
+    private final int nextDocBase;
+    public TopLevelTwoPhaseIterator(DocIdSetIterator approximation, int docBase, int nextDocBase) {
+      super(new PerSegmentViewDocIdSetIterator(approximation, docBase, nextDocBase));
+
+      this.docBase = docBase;
+      this.nextDocBase = nextDocBase;
+    }
+  }
+
+  private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery implements ExtendedQuery{
+    private final String fieldName;
+    private SortedSetDocValues values;
+    private LongBitSet queryTermOrdinals;
+    private boolean matchesAtLeastOneTerm = false;
+    private boolean cache = true;
+    private boolean cacheSeparately = false;
+    private int cost;
+
+
+    public TopLevelDocValuesTermsQuery(String field, BytesRef... terms) {
+      super(field, terms);
+      this.fieldName = field;
+    }
+
+    public Weight createWeight(IndexSearcher searcher, final ScoreMode scoreMode, float boost) throws IOException {
+      values = DocValues.getSortedSet(((SolrIndexSearcher)searcher).getSlowAtomicReader(), fieldName);
+      queryTermOrdinals = new LongBitSet(values.getValueCount());
+      PrefixCodedTerms.TermIterator iterator = getTerms().iterator();
+
+      long lastOrdFound = 0;
+      for(BytesRef term = iterator.next(); term != null; term = iterator.next()) {
+        long ord = lookupTerm(values, term, lastOrdFound);
+        if (ord >= 0L) {
+          matchesAtLeastOneTerm = true;
+          queryTermOrdinals.set(ord);
+          lastOrdFound = ord;
+        }
+      }
+
+      return new ConstantScoreWeight(this, boost) {
+        public Scorer scorer(LeafReaderContext context) throws IOException {
 
 Review comment:
   I see that you are using the top level sortedSetDocValues as a DocIdSetIterator approximation.  I can see this works but I think we needn't get that complex, and we miss out on other optimizations by doing so.  I'll post PR comment on how to replace it in a way that also removes TopLevelTwoPhaseIterator.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on issue #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on issue #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#issuecomment-573107306
 
 
   Ready for review again when you get a chance.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364193401
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/PerSegmentViewDocIdSetIterator.java
 ##########
 @@ -0,0 +1,80 @@
+/*
+ * 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 java.io.IOException;
+
+import org.apache.lucene.search.DocIdSetIterator;
+
+/**
+ * A {@link org.apache.lucene.search.DocIdSetIterator} which allows single-segment access to a wrapped top-level (multi-segment) DISI.
+ *
+ * Useful as a shim between a top-level data structure, and logic that attempts to operate on it using segment-level docIds.
+ * The same wrapped DISI can be used serially by multiple PerSegmentViewDocIdSetIterators
+ */
+public class PerSegmentViewDocIdSetIterator extends DocIdSetIterator {
 
 Review comment:
   Removed entirely per your future feedback

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364301259

Review comment:
"doesn't have the same advantages" without also listing some positives here makes me think this method is not useful when really it's excellent. Lots of users would say 300 queries is a "large number", and this method is appropriate. Also this method is very NRT friendly.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363750077
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -138,8 +164,128 @@ public Query parse() throws SyntaxError {
           bytesRefs[i] = term.toBytesRef();
         }
 
-        return method.makeFilter(fname, bytesRefs);
+        return method.makeFilter(fname, bytesRefs, localParams);
       }
     };
   }
+
+  private static abstract class TopLevelTwoPhaseIterator extends TwoPhaseIterator {
+    private final int docBase;
+    private final int nextDocBase;
+    public TopLevelTwoPhaseIterator(DocIdSetIterator approximation, int docBase, int nextDocBase) {
+      super(new PerSegmentViewDocIdSetIterator(approximation, docBase, nextDocBase));
+
+      this.docBase = docBase;
+      this.nextDocBase = nextDocBase;
+    }
+  }
+
+  private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery implements ExtendedQuery{
 
 Review comment:
   I wouldn't bother implementing ExtendedQuery.  If we want to tweak defaults, we should probably do so to affect all docValuesTermsQuery (both sub-methods) and that can be done by replacing QParser.localParams wrapped with one setting defaults we choose in our QParser.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365341958
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -88,7 +91,20 @@ Query makeFilter(String fname, BytesRef[] byteRefs) {
     docValuesTermsFilter {//on 4x this is FieldCacheTermsFilter but we use the 5x name any way
       @Override
       Query makeFilter(String fname, BytesRef[] byteRefs) {
-        return new DocValuesTermsQuery(fname, byteRefs);//constant scores
+        // TODO Further tune this heuristic number
+        return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
+      }
+    },
+    docValuesTermsFilterTopLevel {
+      @Override
+      Query makeFilter(String fname, BytesRef[] byteRefs) {
+        return new TopLevelDocValuesTermsQuery(fname, byteRefs);
 
 Review comment:
   Sure, setting`cache=true` is trivial.  It's _knowing_ that you need to be explicit for this one query in particular that's the tricky part.
   
   But I'm just hand-wringing; I've already made the change.  It's documented in the ref-guide, so that's as good as we're going to get.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on issue #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on issue #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#issuecomment-572012777
 
 
   David's Scorer implementation is waay simpler than mine.  I ran a performance test last night and it performs just as well.  (see below).  Not sure what I was thinking with the overly complex PerSegmentView stuff.  Thanks David!  Hoping to get address review comments addressed and pushed up shortly.
   
   ![perf-with-davids-improvement](https://user-images.githubusercontent.com/9030708/71975425-ec4b9380-31e1-11ea-999e-a8caaaa43afa.png)
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364215294
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -138,8 +164,128 @@ public Query parse() throws SyntaxError {
           bytesRefs[i] = term.toBytesRef();
         }
 
-        return method.makeFilter(fname, bytesRefs);
+        return method.makeFilter(fname, bytesRefs, localParams);
       }
     };
   }
+
+  private static abstract class TopLevelTwoPhaseIterator extends TwoPhaseIterator {
+    private final int docBase;
+    private final int nextDocBase;
+    public TopLevelTwoPhaseIterator(DocIdSetIterator approximation, int docBase, int nextDocBase) {
+      super(new PerSegmentViewDocIdSetIterator(approximation, docBase, nextDocBase));
+
+      this.docBase = docBase;
+      this.nextDocBase = nextDocBase;
+    }
+  }
+
+  private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery implements ExtendedQuery{
 
 Review comment:
   Will that work?  My understanding was that SolrIndexSearcher uses cost/cache iff it notices its working with an ExtendedQuery that has `getCost`/`getCache` methods it can use. (i.e. it uses those specific methods, it doesn't root around in SolrParams objects looking for cost/cache param values.)

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365344397
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -92,22 +92,28 @@ Query makeFilter(String fname, BytesRef[] byteRefs) {
       @Override
       Query makeFilter(String fname, BytesRef[] byteRefs) {
         // TODO Further tune this heuristic number
-        return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
+        return disableCacheByDefault((byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs));
 
 Review comment:
   right here simply call docValuesTermsFilterTopLevel.makeFilter or docValuesTermsFilterPerSegment.makeFilter.  Yes, enum methods may refer to other enums in the same enum :-)

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363759271
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -138,8 +164,128 @@ public Query parse() throws SyntaxError {
           bytesRefs[i] = term.toBytesRef();
         }
 
-        return method.makeFilter(fname, bytesRefs);
+        return method.makeFilter(fname, bytesRefs, localParams);
       }
     };
   }
+
+  private static abstract class TopLevelTwoPhaseIterator extends TwoPhaseIterator {
+    private final int docBase;
+    private final int nextDocBase;
+    public TopLevelTwoPhaseIterator(DocIdSetIterator approximation, int docBase, int nextDocBase) {
+      super(new PerSegmentViewDocIdSetIterator(approximation, docBase, nextDocBase));
+
+      this.docBase = docBase;
+      this.nextDocBase = nextDocBase;
+    }
+  }
+
+  private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery implements ExtendedQuery{
+    private final String fieldName;
+    private SortedSetDocValues values;
+    private LongBitSet queryTermOrdinals;
+    private boolean matchesAtLeastOneTerm = false;
+    private boolean cache = true;
+    private boolean cacheSeparately = false;
+    private int cost;
+
+
+    public TopLevelDocValuesTermsQuery(String field, BytesRef... terms) {
+      super(field, terms);
+      this.fieldName = field;
+    }
+
+    public Weight createWeight(IndexSearcher searcher, final ScoreMode scoreMode, float boost) throws IOException {
+      values = DocValues.getSortedSet(((SolrIndexSearcher)searcher).getSlowAtomicReader(), fieldName);
 
 Review comment:
   I suggest renaming to "topDocValues".   The "top"-ness vs "segment" needs to be made more clear in all variables names here because we dance between them.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365065931
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -142,4 +158,100 @@ public Query parse() throws SyntaxError {
       }
     };
   }
+
+  private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery {
+    private final String fieldName;
+    private SortedSetDocValues topLevelDocValues;
+    private LongBitSet topLevelTermOrdinals;
+    private boolean matchesAtLeastOneTerm = false;
+
+
+    public TopLevelDocValuesTermsQuery(String field, BytesRef... terms) {
+      super(field, terms);
+      this.fieldName = field;
+    }
+
+    public Weight createWeight(IndexSearcher searcher, final ScoreMode scoreMode, float boost) throws IOException {
+      if (! (searcher instanceof SolrIndexSearcher)) {
+        log.debug("Falling back to DocValuesTermsQuery because searcher [{}] is not the required SolrIndexSearcher", searcher);
+        return super.createWeight(searcher, scoreMode, boost);
+      }
+
+      topLevelDocValues = DocValues.getSortedSet(((SolrIndexSearcher)searcher).getSlowAtomicReader(), fieldName);
+      topLevelTermOrdinals = new LongBitSet(topLevelDocValues.getValueCount());
+      PrefixCodedTerms.TermIterator iterator = getTerms().iterator();
+
+      long lastTermOrdFound = 0;
+      for(BytesRef term = iterator.next(); term != null; term = iterator.next()) {
+        long currentTermOrd = lookupTerm(topLevelDocValues, term, lastTermOrdFound);
+        if (currentTermOrd >= 0L) {
+          matchesAtLeastOneTerm = true;
+          topLevelTermOrdinals.set(currentTermOrd);
+          lastTermOrdFound = currentTermOrd;
+        }
+      }
+
+      return new ConstantScoreWeight(this, boost) {
+        public Scorer scorer(LeafReaderContext context) throws IOException {
+          if (! matchesAtLeastOneTerm) {
+            return null;
+          }
+          
+          SortedSetDocValues segmentDocValues = context.reader().getSortedSetDocValues(fieldName);
+          if (segmentDocValues == null) {
+            return null;
+          }
+
+          final int docBase = context.docBase;
+          return new ConstantScoreScorer(this, this.score(), scoreMode, new TwoPhaseIterator(segmentDocValues) {
+            public boolean matches() throws IOException {
+              topLevelDocValues.advanceExact(docBase + approximation.docID());
+              for(long ord = topLevelDocValues.nextOrd(); ord != -1L; ord = topLevelDocValues.nextOrd()) {
+                if (topLevelTermOrdinals.get(ord)) {
+                  return true;
+                }
+              }
+
+              return false;
+            }
+
+            public float matchCost() {
+              return 3.0F;
 
 Review comment:
   Compared to other TPIs, I think 3f is too low.  For example DVTQ (in the superclass) is 3f but we also must call advanceExact on the top level DV.  I suggest 10f.  It's a bike-shed number but I just want more than 3 and much less than 100 which is what some of the no-idea but probably expensive TPIs have.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365356666

Review comment:
I thought about this too when I saw the diff. I remember it coming up as a discussion point when the ref-guide was initially being hashed out too.

I won't change my docs here, as it's probably worth keeping the file as a whole consistent in the strategy it uses. But maybe it's worth revisiting at some point.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365364487
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -92,22 +92,28 @@ Query makeFilter(String fname, BytesRef[] byteRefs) {
       @Override
       Query makeFilter(String fname, BytesRef[] byteRefs) {
         // TODO Further tune this heuristic number
-        return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
+        return disableCacheByDefault((byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs));
 
 Review comment:
   LOL there is something slightly unsettling about it I admit.  Though it's not confusing or anything.  Any way I value brevity over a lot and I'll take it!

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364293164
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -138,8 +164,128 @@ public Query parse() throws SyntaxError {
           bytesRefs[i] = term.toBytesRef();
         }
 
-        return method.makeFilter(fname, bytesRefs);
+        return method.makeFilter(fname, bytesRefs, localParams);
       }
     };
   }
+
+  private static abstract class TopLevelTwoPhaseIterator extends TwoPhaseIterator {
+    private final int docBase;
+    private final int nextDocBase;
+    public TopLevelTwoPhaseIterator(DocIdSetIterator approximation, int docBase, int nextDocBase) {
+      super(new PerSegmentViewDocIdSetIterator(approximation, docBase, nextDocBase));
+
+      this.docBase = docBase;
+      this.nextDocBase = nextDocBase;
+    }
+  }
+
+  private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery implements ExtendedQuery{
 
 Review comment:
   It will because QParser.getQuery will wrap the query produced by parse() if it's not already an ExtendedQuery.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on issue #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on issue #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#issuecomment-572846598
 
 
   @dsmiley ready for re-review when you get another chance

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365358715
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -92,22 +92,28 @@ Query makeFilter(String fname, BytesRef[] byteRefs) {
       @Override
       Query makeFilter(String fname, BytesRef[] byteRefs) {
         // TODO Further tune this heuristic number
-        return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
+        return disableCacheByDefault((byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs));
 
 Review comment:
   Hah.  I don't know whether to think that's awesome or revolting.  Done.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija merged pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija merged pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151
 
 
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364393673

Review comment:
Hmm, I see your point. I don't want to take the advice out entirely because the first-query QTime is pretty startling and I don't want that to scare people away. Maybe I'll leave the warning in but reword it to be a bit more conditional.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r363753194
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/PerSegmentViewDocIdSetIterator.java
 ##########
 @@ -0,0 +1,80 @@
+/*
+ * 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 java.io.IOException;
+
+import org.apache.lucene.search.DocIdSetIterator;
+
+/**
+ * A {@link org.apache.lucene.search.DocIdSetIterator} which allows single-segment access to a wrapped top-level (multi-segment) DISI.
+ *
+ * Useful as a shim between a top-level data structure, and logic that attempts to operate on it using segment-level docIds.
+ * The same wrapped DISI can be used serially by multiple PerSegmentViewDocIdSetIterators
+ */
+public class PerSegmentViewDocIdSetIterator extends DocIdSetIterator {
 
 Review comment:
   Can you make this an inner class of topLevelDocValuesTermsQuery or anonymous?  If there is a future need to share it _then_ I think we should pull it out.  I doubt it's as generally useful to be a standalone public class.
   
   After further review, I don't think we need the functionality here _at all_... see my future feedback

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365341958
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -88,7 +91,20 @@ Query makeFilter(String fname, BytesRef[] byteRefs) {
     docValuesTermsFilter {//on 4x this is FieldCacheTermsFilter but we use the 5x name any way
       @Override
       Query makeFilter(String fname, BytesRef[] byteRefs) {
-        return new DocValuesTermsQuery(fname, byteRefs);//constant scores
+        // TODO Further tune this heuristic number
+        return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
+      }
+    },
+    docValuesTermsFilterTopLevel {
+      @Override
+      Query makeFilter(String fname, BytesRef[] byteRefs) {
+        return new TopLevelDocValuesTermsQuery(fname, byteRefs);
 
 Review comment:
   Sure, setting`cache=true` is trivial.  It's _knowing_ that you need to be explicit for this one query in particular that's the tricky part.
   
   But sure, I'm just hand-wringing; I've already made the change.  It's documented in the ref-guide, so that's as good as we're going to get.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365336343
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -88,7 +91,20 @@ Query makeFilter(String fname, BytesRef[] byteRefs) {
     docValuesTermsFilter {//on 4x this is FieldCacheTermsFilter but we use the 5x name any way
       @Override
       Query makeFilter(String fname, BytesRef[] byteRefs) {
-        return new DocValuesTermsQuery(fname, byteRefs);//constant scores
+        // TODO Further tune this heuristic number
+        return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
+      }
+    },
+    docValuesTermsFilterTopLevel {
+      @Override
+      Query makeFilter(String fname, BytesRef[] byteRefs) {
+        return new TopLevelDocValuesTermsQuery(fname, byteRefs);
 
 Review comment:
   This is just about a default.  So-called "Hands-on users" (experts) will be able to be explicit with cache=true.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

dsmiley commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r365345185

Review comment:
Just a side-comment about our ref docs: In the ref docs I write or update, I convert them to one sentence per line. This makes the diffs easy to read! The pain of no newlines is very apparent here. Change or not as you wish. CC @ctargett

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene-solr] gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation

Posted by GitBox <gi...@apache.org>.

gerlowskija commented on a change in pull request #1151: SOLR-13890: Add "top-level" DVTQ implementation
URL: https://github.com/apache/lucene-solr/pull/1151#discussion_r364197715
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
 ##########
 @@ -138,8 +164,128 @@ public Query parse() throws SyntaxError {
           bytesRefs[i] = term.toBytesRef();
         }
 
-        return method.makeFilter(fname, bytesRefs);
+        return method.makeFilter(fname, bytesRefs, localParams);
       }
     };
   }
+
+  private static abstract class TopLevelTwoPhaseIterator extends TwoPhaseIterator {
+    private final int docBase;
+    private final int nextDocBase;
+    public TopLevelTwoPhaseIterator(DocIdSetIterator approximation, int docBase, int nextDocBase) {
+      super(new PerSegmentViewDocIdSetIterator(approximation, docBase, nextDocBase));
+
+      this.docBase = docBase;
+      this.nextDocBase = nextDocBase;
+    }
+  }
+
+  private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery implements ExtendedQuery{
+    private final String fieldName;
+    private SortedSetDocValues values;
+    private LongBitSet queryTermOrdinals;
+    private boolean matchesAtLeastOneTerm = false;
+    private boolean cache = true;
+    private boolean cacheSeparately = false;
+    private int cost;
+
+
+    public TopLevelDocValuesTermsQuery(String field, BytesRef... terms) {
+      super(field, terms);
+      this.fieldName = field;
+    }
+
+    public Weight createWeight(IndexSearcher searcher, final ScoreMode scoreMode, float boost) throws IOException {
+      values = DocValues.getSortedSet(((SolrIndexSearcher)searcher).getSlowAtomicReader(), fieldName);
 
 Review comment:
   I went with `topLevelDocValues`, as `topDocValues` might sounds like a PQueue or a list to those unfamiliar with the code.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org