cpoerschke commented on a change in pull request #151: URL: https://github.com/apache/solr/pull/151#discussion_r656418654
########## File path: solr/core/src/java/org/apache/solr/handler/component/SortedHitQueueManager.java ########## @@ -0,0 +1,51 @@ +/* + * 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; + +public abstract class SortedHitQueueManager { + + abstract void addDocument(ShardDoc shardDoc); + + abstract ShardDoc popDocument(); + + abstract int size(); + + /** + * Return the implementation of SortedHitQueueManager that should be used for the given sort and ranking. + * We use multiple queues if reRanking is enabled and we do not sort by score. + * In all other cases only one queue is used. + * + * @param sortFields the fields by which the results should be sorted + * @param ss SortSpec of the responseBuilder Review comment: ```suggestion * @param ss SortSpec ``` or perhaps we can remove the `ss` parameter then and do `rb.getSort()` instead? ########## File path: solr/core/src/java/org/apache/solr/handler/component/ReRankSortedHitQueueManager.java ########## @@ -0,0 +1,88 @@ +/* + * 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.IndexSearcher; +import org.apache.lucene.search.SortField; + +/** + * This class is used to manage the multiple SortedHitQueues that we need during mergeIds( ). + * Multiple queues are needed when reRanking is used. + * + * The top reRankDocsSize documents are added to the reRankQueue, all other documents are + * collected in the queue. + */ +public class ReRankSortedHitQueueManager extends SortedHitQueueManager { + + private final ShardFieldSortedHitQueue queue; + private final ShardFieldSortedHitQueue reRankQueue; + private final int reRankDocsSize; + + public ReRankSortedHitQueueManager(SortField[] sortFields, int count, int offset, IndexSearcher searcher, int reRankDocsSize) { + this.reRankDocsSize = reRankDocsSize; + int absoluteReRankDocs = Math.min(reRankDocsSize, count); + reRankQueue = new ShardFieldSortedHitQueue(new SortField[]{SortField.FIELD_SCORE}, + absoluteReRankDocs, searcher); + queue = new ShardFieldSortedHitQueue(sortFields, offset + count - absoluteReRankDocs, + searcher, false /* useSameShardShortcut */); + } + + /** + * Check whether the sortFields contain score. + * If that's true, we do not support this query. (see comments below) + * @deprecated will be removed as soon as https://github.com/apache/solr/pull/171 is done + */ + @Deprecated // will be removed when all sort fields become supported + public static boolean supports(SortField[] sortFields) { + for (SortField sortField : sortFields) { + // do not check equality with SortField.FIELD_SCORE because that would only cover score desc, not score asc + if (sortField.getType().equals(SortField.Type.SCORE) && sortField.getField() == null) { + // using two queues makes reRanking on Solr Cloud possible (@see https://github.com/apache/solr/pull/151 ) + // however, the fix does not work if the non-reRanked docs should be sorted by score ( @see https://github.com/apache/solr/pull/151#discussion_r640664451 ) + // to maintain the existing behavior for these cases, we disable the broken use of two queues and use the (also broken) status quo instead + return false; + } + } + return true; + } + + public void addDocument(ShardDoc shardDoc) { + if(shardDoc.orderInShard < reRankDocsSize) { + ShardDoc droppedShardDoc = reRankQueue.insertWithOverflow(shardDoc); + // Only works if the original request does not sort by score + // @AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-15437";) Review comment: `@AwaitsFix` is usually used for tests I think and using it in non-test code (albeit in a comment) could be confusing. ```suggestion // see https://issues.apache.org/jira/browse/SOLR-15437 ``` ########## File path: solr/core/src/java/org/apache/solr/handler/component/ReRankSortedHitQueueManager.java ########## @@ -0,0 +1,88 @@ +/* + * 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.IndexSearcher; +import org.apache.lucene.search.SortField; + +/** + * This class is used to manage the multiple SortedHitQueues that we need during mergeIds( ). + * Multiple queues are needed when reRanking is used. + * + * The top reRankDocsSize documents are added to the reRankQueue, all other documents are + * collected in the queue. + */ +public class ReRankSortedHitQueueManager extends SortedHitQueueManager { + + private final ShardFieldSortedHitQueue queue; + private final ShardFieldSortedHitQueue reRankQueue; + private final int reRankDocsSize; + + public ReRankSortedHitQueueManager(SortField[] sortFields, int count, int offset, IndexSearcher searcher, int reRankDocsSize) { + this.reRankDocsSize = reRankDocsSize; + int absoluteReRankDocs = Math.min(reRankDocsSize, count); + reRankQueue = new ShardFieldSortedHitQueue(new SortField[]{SortField.FIELD_SCORE}, + absoluteReRankDocs, searcher); + queue = new ShardFieldSortedHitQueue(sortFields, offset + count - absoluteReRankDocs, + searcher, false /* useSameShardShortcut */); + } + + /** + * Check whether the sortFields contain score. + * If that's true, we do not support this query. (see comments below) + * @deprecated will be removed as soon as https://github.com/apache/solr/pull/171 is done + */ + @Deprecated // will be removed when all sort fields become supported + public static boolean supports(SortField[] sortFields) { + for (SortField sortField : sortFields) { + // do not check equality with SortField.FIELD_SCORE because that would only cover score desc, not score asc + if (sortField.getType().equals(SortField.Type.SCORE) && sortField.getField() == null) { + // using two queues makes reRanking on Solr Cloud possible (@see https://github.com/apache/solr/pull/151 ) + // however, the fix does not work if the non-reRanked docs should be sorted by score ( @see https://github.com/apache/solr/pull/151#discussion_r640664451 ) + // to maintain the existing behavior for these cases, we disable the broken use of two queues and use the (also broken) status quo instead Review comment: minor ```suggestion // to maintain the existing behavior for these cases, we do not support the broken use of two queues and continue to use the (also broken) status quo instead ``` ########## File path: solr/core/src/java/org/apache/solr/handler/component/ReRankSortedHitQueueManager.java ########## @@ -0,0 +1,88 @@ +/* + * 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.IndexSearcher; +import org.apache.lucene.search.SortField; + +/** + * This class is used to manage the multiple SortedHitQueues that we need during mergeIds( ). Review comment: minor: ```suggestion * This class is used to manage the multiple SortedHitQueues that we need during {@link QueryComponent#mergeIds(ResponseBuilder, ShardRequest)}. ``` ########## 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: > But I do not think that the QueryComponent should know when to create which SortedHitQueueManager. So I moved the `newSortedHitQueueManager()` into the manager itself and changed the interface to an abstract class. ([bbf887f](https://github.com/apache/solr/commit/bbf887f3bc4f24c1b5ff4ff213fb658f05c7d25f)) > > What do you think about that change? Makes sense. And thanks for adding wonderful javadocs for the new classes 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. 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
