Don't run sort optimization on size=0 #57044
Conversation
Sort optimization creates TopFieldCollector that errors when size=0. This ensures that sort optimization is not run when size=0. Closes elastic#56923
mayya-sharipova
added
the
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
jtibshirani
left a comment
jtibshirani
left a comment
There was a problem hiding this comment.
Thanks @mayya-sharipova, this looks good to me. I left a question on the testing strategy but it's not critical.
| // 5. Test that sort optimization is NOT run with size 0 | ||
| { | ||
| sortAndFormats = new SortAndFormats(longSort, new DocValueFormat[]{DocValueFormat.RAW}); | ||
| searchContext = spy(new TestSearchContext(null, indexShard, newContextSearcher(reader))); |
There was a problem hiding this comment.
I'm wondering why we use a spy here, if we don't verify the calls after?
There was a problem hiding this comment.
@jtibshirani Thanks for the review. I am using spy here to wrap an instance of searchContext for it to return a fake mapperService on the following line of 711. Since we don't have a real node here, I had to create a fake MapperService on line 646.
Sort optimization creates TopFieldCollector that errors when size=0. This ensures that sort optimization is not run when size=0. Closes #56923
Sort optimization creates TopFieldCollector that errors when size=0. This ensures that sort optimization is not run when size=0. Closes #56923
Sort optimization creates TopFieldCollector that errors when size=0. This ensures that sort optimization is not run when size=0. Closes #56923
| CheckedConsumer<List<LeafReaderContext>, IOException> leafSorter = l -> {}; | ||
| // try to rewrite numeric or date sort to the optimized distanceFeatureQuery | ||
| if ((searchContext.sort() != null) && SYS_PROP_REWRITE_SORT) { | ||
| if ((searchContext.sort() != null) && (searchContext.size() > 0) && SYS_PROP_REWRITE_SORT) { |
There was a problem hiding this comment.
What if 'size' is 0 but 'from' is not, can this case uses sort optimization? if it can, changing this place to "from + size" may be much better.
Feel free about this, just an advice :P
There was a problem hiding this comment.
@yyff thanks for pointing this out! It seems unusual to perform a search with from: n, size: 0, but I do agree it's more consistent to check 'num hits' (which is from + size) instead of just size. @mayya-sharipova what are your thoughts on adjusting this?
There was a problem hiding this comment.
@yyff @jtibshirani Thanks for commenting. I have addressed this in #57250
5 participants
Sort optimization creates TopFieldCollector that errors
when size=0. This ensures that sort optimization is not
run when size=0.
Closes #56923