cpoerschke commented on a change in pull request #151: URL: https://github.com/apache/solr/pull/151#discussion_r654956812
########## File path: solr/core/src/java/org/apache/solr/handler/component/SortedHitQueueManager.java ########## @@ -0,0 +1,83 @@ +/* + * 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.handler.component; + +import org.apache.lucene.search.SortField; +import org.apache.solr.search.AbstractReRankQuery; +import org.apache.solr.search.RankQuery; +import org.apache.solr.search.SortSpec; + +/** + * This class is used to manage the possible multiple SortedHitQueues that we need during mergeIds( ). + * Multiple queues are needed, if reRanking is used. + * + * If reRanking is disabled, only the queue is used. + * If reRanking is enabled, the top reRankDocsSize documents are added to the reRankQueue, all other documents are + * collected in the queue. + */ +public class SortedHitQueueManager { + private final ShardFieldSortedHitQueue queue; + private final ShardFieldSortedHitQueue reRankQueue; + private final int reRankDocsSize; + + public SortedHitQueueManager(SortField[] sortFields, SortSpec ss, ResponseBuilder rb) { + final RankQuery rankQuery = rb.getRankQuery(); + + if(rb.getRankQuery() != null && rankQuery instanceof AbstractReRankQuery){ Review comment: Hi @tomglk, Thanks for updating the branch so that the existing behaviour is maintained when the sort includes a score! In terms of communicating the scope of the fix, I can see a few complimentary possibilities: * solr/CHANGES.txt entry for the JIRA issue: if this is very specific e.g. "custom sort without a score" rather than only "custom sort" wording then that could convey the scope and non-scope of the fix. * Solr Ref Guide: https://solr.apache.org/guide/8_9/query-re-ranking.html appears to not yet mention distributed queries, so there's an opportunity there to document limitations or caveats e.g. https://solr.apache.org/guide/8_9/result-grouping.html#distributed-result-grouping-caveats does something similar. * code javadocs: i've pushed an exploratory commit to the branch -- _feel free to revert or to adapt e.g. the (Single/Dual) SortedHitQueueManager name choice is probably too generic_ -- and in it the javadocs for the `DualSortedHitQueueManager.supports` method could provide a natural place to communicate what is and what is not supported and the usage of the method when deciding the DualSortedHitQueueManager vs. SingleSortedHitQueueManager instantiation helps communicate structually when existing behaviour is maintained since SingleSortedHitQueueManager is a simple wrapper around ShardFieldSortedHitQueue. > ... I think this was the only ToDo left. ... The test coverage in `solr/core` remains on the ToDo list I think, though it's easy to miss since it's rather hidden amongst all the discussion detail that led us to this point. -- 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...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
