HSEARCH-2254 Support (single-valued) sorts on fields within nested fields

[fax4ever]

https://hibernate.atlassian.net/browse/HSEARCH-2254

[fax4ever]

Still in preview: I handled only Field sort.
I think I have to handle at least the Distance one too.

I would prefer have a review on what I've done so far, before continue. Since the issue is not trivial at all.

To be honest, it was very hard for me
and I'm pretty sure we could improve several choices I made here.
For instance we will avoid to run a top doc collector again just to load the nested contributions.

Looking at Elasticsearch's code it seems that they load the nested document using
not ToParentBlockJoinQuery nor ToChildBlockJoinQuery but ParentChildrenBlockJoinQuery.
With the latter we need to pass the parent id. So we would have needed to run one for each parent document.
For that reason here I opted for the ToChildBlockJoinQuery`, that's same as we used for supporting projection on nested fields.

This issue is more difficult than the one on projection. Here we cannot simply merge all contributions. We need to switch document id ( parent to nested ) for each nested field ( so the switching and the convertion map are scoped on the single field ), paying attention do not alter what we already do on flattened or root object fields: see CompositeSortIT.

[yrodiere]

Well, that must have required some deep thinking. The solution looks very good overall, so thanks for all this nice work!

As you suspected we will need several improvements, mainly to improve performance. I added a few comments below on areas that can be improved.

There is the matter of rebasing on my "aggregations" branch, however. There will be a few conflicts due to the refactorings I had to do to introduce aggregations. I see you split your commits very finely, which is a very good idea and should facilitate rebasing.

My suggestion would be to proceed like this:

1. Squash the commits related to the Lucene implementation of sorts, or at least squash the commits that are erased by a later commit. I think there are several of these, and it would be wasting your time to rebase them.
2. Rebase on my branch
3. And only then, try to address my comments.

If necessary, I can take care of step 2; it will probably be easier for me since you haven't reviewed my PR yet.

[yrodiere]

You should probably move this above requireFieldDocRescoring, otherwise scores might be wrong in some cases.

Also, I think extractTopDocs doesn't need to be called when you call this method? Something like this would work:

if ( nestedPathsInSort != null && !nestedPathsInSort.isEmpty() ) {
	extractTopDocsUsingTheirNested( indexSearcher, luceneQuery, offset, limit ); 
}
else {
	extractTopDocs( offset, limit );
}

if ( requireFieldDocRescoring ) {
	handleRescoring( indexSearcher, luceneQuery );
}

[fax4ever]

I moved extractTopDocsUsingTheirNested above.
But without other changes we need to call extractTopDocs before we call extractTopDocsUsingTheirNested, since we need the ScoreDoc[] scoreDocs to fetch the nestedDocumentMap.

[yrodiere]

Ok... now that I think about it, I think there's a flaw in this approach.
What about the following scenario:

• The query matches documents A, B, C
• The limit is 2
• The first search returns top documents [A, B]
• The second search, taking into account nested documents, would have returned [C, B] (A excluded because it's third), but since C was not in the top docs of the first search, it ends up returning [B, A].

I suspect this is a very real possibility...

The only option to avoid this problem would be to revert the logic:

• Currently you perform the search() call once, and then if you discover that there are nested sorts, you try to "fix" the first results by running a second search().
• The alternative would be to detect before any search() that there are nested sorts. In that case you would run a preliminary search() to collect nested documents (and only nested documents) without a limit. Then you would run the actual search() with a limit.

I'm aware this would be require an awful lot of memory for big indexes, but at least it would work (?). We can create a ticket to try and optimize this, maybe.

[yrodiere]

Actually, I've had a look at the Elasticsearch code, and it seems they retrieve nested documents on the fly when it comes to sorts. See my other comment. I think if you manage to implement a solution that does the same, the flaw I just mentioned will just disappear. Could you have a look, please?

[fax4ever]

Sure

[fax4ever]

Thanks @yrodiere. Rebased the changes.
Now I can address your comments.

[fax4ever]

Thanks @yrodiere. I think I've address all the comments except the one about to optimize nested queries when we have both nested sorts and nested projections.

I'm not sure we can optimize something, since projections and sorts can work on different nested document paths. Let's talk on Zulip.

[lgtm-com]

This pull request introduces 1 alert when merging 2e68834 into 1e3ae6a - view on LGTM.com

new alerts:

• 1 for Useless null check

[yrodiere]

Thanks, most of my concerns have been addressed.

I answered your comments concerning the remaining problems. Sure, let's talk on Zulip.

[fax4ever]

@yrodiere thanks!
I think that now we have an on-the-fly nested field sorting.
Have I missed some comments?

[yrodiere]

Here are some suggestions.

WARNING: I force-pushed to your branch to change a test! See my comments below, but don't forget to pull before you start working again.

[fax4ever]

@yrodiere thanks.
I think I fulfilled the comments here.
If there were other changes to do, I would happy to handle them.

[yrodiere]

Here are a few (final?) comments. I think we can merge this PR once they are resolved; I created HSEARCH-3694 to address distance sorts later (I'm sure it will be just as complex...).

[fax4ever]

Addressed all comments, with the exception of this one: #2068 (comment).

We could solve it, but we definitely need the final lucene query at LuceneCollectors build time (not just at invocation time). Do you think that is possible?
Or should we create TopFieldCollector at LuceneCollectors invocation time?
The latter sounds a bit strange...
Thanks!

[fax4ever]

Running a full build: job/PR-2068/19

[fax4ever]

I need to add a commit to support the old nested sorting syntax of Elasticsearch 5 .

[fax4ever]

Here we have a new build for all supported ES dialects: job/PR-2068/22

[fax4ever]

I added a test on a deep > 1 nested field. Full build: job/PR-2068/24

[fax4ever]

@yrodiere as usual you were right. Taking the flag from the ElasticsearchJsonSyntaxHelper is definitely much easier! You can find the yet another full build here.

[yrodiere]

Merged! Thanks for all the work... and for putting up with me ;)