Remove ToParentBlockJoinCollector [LUCENE-6959]

[asfimport]

This collector uses the getWeight() and getChildren() methods from the passed in Scorer, which are not always available (eg. disjunctions expose fake scorers) hence the need for a dedicated IndexSearcher (ToParentBlockJoinIndexSearcher). Given that this is the only collector in this case, I would like to remove it.

---

Migrated from LUCENE-6959 by Adrien Grand (@jpountz), resolved Jan 29 2017
Attachments: LUCENE_6959.patch (versions: 5), LUCENE-6959.patch
Linked issues:

• Remove Scorer.getChildren? [LUCENE-6229] #7291
• Do not expose full-fledged scorers in LeafCollector.setScorer [LUCENE-6228] #7290
• To-parent block joins should implement two-phase iteration [LUCENE-7654] #8705
• SOLR-8643

[asfimport]

Mikhail Khludnev (@mkhludnev) (migrated from JIRA)

Here is the recently arrived BlockJoinFacetCollector also calls getChildren(). It seems it works under SolrIndexSearcher, and it's not easy to use a dedicated searcher in Solr.

[asfimport]

Mikhail Khludnev (@mkhludnev) (migrated from JIRA)

Note. SOLR-8643 removed BlockJoinFacetCollector which calls scorer getChildren()

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

This collector is also blocking improvements to block-join queries since it prevents them from using two-phase iteration: #8705.

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

Here is a patch that removes it. The class was exprimental, so I think it is fine to remove in 6.x, but if there are concerns, I am ok with waiting for 7.0.

[asfimport]

Martijn van Groningen (@martijnvg) (migrated from JIRA)

+1 To remove this collector in the master and 6x branches.

As a follow up issue we can add back to ability to include child docs, but in a different way than is done today. A subsequent search after the main search, that selects child docs for specific parents. For example like is done here: https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/search/fetch/subphase/InnerHitsContext.java#L160

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I agree it's messy that one must use a special IndexSearcher to use this collector, and that it's overly invasive that it needs access to child scorers, and we should find a better way.

This collector enables you to get the matching child hits when running a joined search against child hits up to parent documents. This is very important for people "joining up" with document blocks, e.g. I use this in http://jirasearch.mikemccandless.com, where child documents are each jira comment, and parent documents are the whole jira issue.

I'd like to understand a bit better how exactly we can re-implement this functionality once we remove the collector. That ES query class seems to be created for each parent doc that made the top N hits, right? And then you search with that query, which is very quick since it will hone in on just the children of that one parent, and build up your child hits that way ... I think this is indeed a better approach!

It's sort of weird to remove this class without first making the cleaner implementation available, but since neither Solr nor ES use it, I think it's OK (and it is experimental). Can we at least open an issue to add the cleaner option? I can try to tackle it, certainly when I next upgrade jirasearch ;) Thanks, +1 to remove in 6.x and 7.0.

[asfimport]

Martijn van Groningen (@martijnvg) (migrated from JIRA)

I've modified Adrien's patch and ported the ES query mentioned in my previous comment to Lucene join module.

I'd like to understand a bit better how exactly we can re-implement this functionality once we remove the collector. That ES query class seems to be created for each parent doc that made the top N hits, right?

Yes, that is what it is doing.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Oooh, thank you @martijnvg! I'll review the patch...

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Thanks @martijnvg.

I think it's dangerous that we hold onto a LeafReader in the new ParentChildrenBlockJoinQuery?

Can we maybe change the new query to instead hold the parent's docID in the top-level reader's space, and then in the scorer method, check the incoming reader context to see if this is the segment that holds the parent? This would also simplify usage, so users wouldn't have to create their own weights? Then I think you don't need the LeafReader reference.

Also, the TestBlockJoin tests got a little over decimated I think :) Can we restore at least some of the places that were verifying children? Or maybe we could make a simple sugar API that returns TopGroups again, and then we wouldn't need to change the tests (except to switch to this sugar API)?

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

I think I'm the one responsible for the decimated tests. Agreed it would be nice to restore them using an API that is able to compute top groups.

[asfimport]

Martijn van Groningen (@martijnvg) (migrated from JIRA)

Can we maybe change the new query to instead hold the parent's docID in the top-level reader's space, and then in the scorer method, check the incoming reader context to see if this is the segment that holds the parent? This would also simplify usage, so users wouldn't have to create their own weights? Then I think you don't need the LeafReader reference.

+1 That is much better.

[asfimport]

Martijn van Groningen (@martijnvg) (migrated from JIRA)

I've changed the ParentChildrenBlockJoinQuery query, so that it no longer requires to accept a LeafReader and it figures out by itself which leaf reader the parent doc is in.

[asfimport]

Martijn van Groningen (@martijnvg) (migrated from JIRA)

I wonder if we should continue using TopGroups in the TestBlockJoin test case? If we stop using it then we can remove the the dependency on the grouping module the join module has.

The TopGroups has logic for merging other groups, but in case for block join there should be no need for that as the parent and its children are always in the same Lucene index. That makes sense, because in grouping arbitrary document can belong to a group.

Mike, would just using a ScoreDoc with TopDocs be sufficient to represent a jira issue with comments in jirasearch?

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Mike, would just using a ScoreDoc with TopDocs be sufficient to represent a jira issue with comments in jirasearch?

I think that's fine? I agree it would be nice to remove the dependency.

I'll have a look at the patch! Thanks for iterating.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

The new ParentChildrenBlockJoinQuery looks great! Much easier to use.

Do we really need the origChildQuery anymore?