cpoerschke commented on a change in pull request #151: URL: https://github.com/apache/solr/pull/151#discussion_r640946517
########## File path: solr/core/src/java/org/apache/solr/handler/component/ShardFieldSortedHitQueueWithSameShardCompareSkip.java ########## @@ -0,0 +1,48 @@ +/* + * 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; + +/** + * Used by distributed search to merge results. + * If two documents are on the same shard, their orderInShard is used for sorting instead of comparing their sort values. + * This shortcut does only work if we are not reRanking the results. + */ +public class ShardFieldSortedHitQueueWithSameShardCompareSkip extends ShardFieldSortedHitQueue { Review comment: Have you considered not having a `ShardFieldSortedHitQueueWithSameShardCompareSkip` class but for the existing `ShardFieldSortedHitQueue` class to support variant behaviour e.g. something like this ``` public class ShardFieldSortedHitQueue ... { + private final boolean orderInShard; public ShardFieldSortedHitQueue(SortField[] fields, int size, IndexSearcher searcher) { this(fields, size, searcher, true /* useOrderInShard */); } public ShardFieldSortedHitQueue(SortField[] fields, int size, IndexSearcher searcher, boolean useOrderInShard) { ... this.useOrderInShard = useOrderInShard; } ... - if (docA.shard == docB.shard) { + if (docA.shard == docB.shard && this.useOrderInShard) { ... } ``` noting that the 3-arg constructor signature maintains existing i.e. backwards compatible behaviour for the class even if perhaps `QueryComponent` with the changes always uses the new 4-arg constructor. ########## File path: solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java ########## @@ -865,7 +866,19 @@ protected void mergeIds(ResponseBuilder rb, ShardRequest sreq) { // Merge the docs via a priority queue so we don't have to sort *all* of the // documents... we only need to order the top (rows+start) - final ShardFieldSortedHitQueue queue = new ShardFieldSortedHitQueue(sortFields, ss.getOffset() + ss.getCount(), rb.req.getSearcher()); + ShardFieldSortedHitQueue queue = new ShardFieldSortedHitQueueWithSameShardCompareSkip(sortFields, ss.getOffset() + ss.getCount(), rb.req.getSearcher()); + + ShardFieldSortedHitQueue reRankQueue = null; + int reRankDocsSize = 0; + if(rb.getRankQuery() != null){ + final RankQuery rankQuery = rb.getRankQuery(); + if(rankQuery instanceof AbstractReRankQuery){ + reRankDocsSize = ((AbstractReRankQuery) rankQuery).getReRankDocs(); + reRankQueue = new ShardFieldSortedHitQueueWithSameShardCompareSkip(new SortField[]{SortField.FIELD_SCORE}, Math.min(reRankDocsSize, ss.getCount()), rb.req.getSearcher()); + + queue = new ShardFieldSortedHitQueue(sortFields, ss.getOffset() + ss.getCount(), rb.req.getSearcher()); + } + } Review comment: The existing `mergeIds` method is already quite long and complex and addition of `reRankQuery` and `reRankDocSize` for intricate use together with the existing `queue` increases complexity further. I wonder though if with some strategic signature choices it might be possible to somehow abstract the re-rank related logic into a class so that from the perspective of the `mergeIds` method nothing really changes? E.g. imagine that line 869 here did not do `new ShardFieldSortedHitQueue...` directly but instead called a small wrapper method in QueryComponent: ``` private static ShardFieldSortedHitQueue newShardFieldSortedHitQueue(SortField[] sortFields, int count, int offset, IndexSearcher searcher, RankQuery rankQuery) { if (rankQuery instanceof AbstractReRankQuery) { return new FooBarShardFieldSortedHitQueue(sortFields, count, offset, searcher, ((AbstractReRankQuery) rankQuery).getReRankDocs()); } else { return new ShardFieldSortedHitQueue(sortFields, count + offset, searcher); } } ``` and well `FooBarShardFieldSortedHitQueue` is obviously not a good class name but imagine it looked something like this ``` class FooBarShardFieldSortedHitQueue extends ShardFieldSortedHitQueue { final private int reRankDocsSize; final private ShardFieldSortedHitQueue foobarQueue; @Override public void insertWithOverflowAndI(T element, int i) { if (i < reRankDocsSize) { T droppedElement = foobarQueue.insertWithOverflow(element); if (dropped != null ) { super.insertWithOverflow(dropped); } } else { super.insertWithOverflow(element); } } @Override public T pop1() { T element = super.pop(); if (element == null) { element = foobar.pop(); } return element; } } ``` with supporting `ShardFieldSortedHitQueue` tweaks as follows ``` public class ShardFieldSortedHitQueue extends PriorityQueue<ShardDoc> { + public void insertWithOverflowAndI(T element, int i) { + super.insertWithOverflow(element); + } + @Override // workaround for super class' pop (understandably) being final + public T pop1() { + return super.pop(); + } } ``` and small matching `mergeId` adjustments to call `insertWithOverflowAndI` instead of `insertWithOverflow` and `pop1` instead of `pop`. So same logic, only structured differently in terms of code with `FooBarShardFieldSortedHitQueue` potentially also more easily testable (though I haven't though much about that as yet to be honest). -- 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: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
