Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7206

nest child query explain into ToParentBlockJoinQuery.BlockJoinScorer.explain(int)

    Details

    • Lucene Fields:
      New

      Description

      Now to parent query match is explained with {{Score based on child doc range from .. to .. }} that's quite useless.
      It's proposed to nest child query match explanation from the first matching child document into parent explain.

      WDYT?

      1. LUCENE-7206.diff
        2 kB
        Ilya Kasnacheev
      2. LUCENE-7206-one-child-with-tests.patch
        7 kB
        Ilya Kasnacheev
      3. LUCENE-7206-test.patch
        3 kB
        Jeff Evans

        Activity

        Hide
        martijn.v.groningen Martijn van Groningen added a comment -

        +1 I think including the first child explain makes sense. Perhaps instead of 'child docid x to y' matches, we can just include the number of child docs matched (total_matches)?

        Show
        martijn.v.groningen Martijn van Groningen added a comment - +1 I think including the first child explain makes sense. Perhaps instead of 'child docid x to y' matches, we can just include the number of child docs matched (total_matches)?
        Hide
        ilyak Ilya Kasnacheev added a comment -

        See the attached diff.

        Now, it explains every matched child, perhaps it should be limited to one with best score.

        Show
        ilyak Ilya Kasnacheev added a comment - See the attached diff. Now, it explains every matched child, perhaps it should be limited to one with best score.
        Hide
        martijn.v.groningen Martijn van Groningen added a comment -

        Now, it explains every matched child, perhaps it should be limited to one with best score.

        Yes, I think we should limit it to the best matching child document, otherwise the explain (which without this change can already be large) may explode with the amount of child document explanations.]

        Would be great to also add a test, which checks the child doc explanations are actually there.

        Show
        martijn.v.groningen Martijn van Groningen added a comment - Now, it explains every matched child, perhaps it should be limited to one with best score. Yes, I think we should limit it to the best matching child document, otherwise the explain (which without this change can already be large) may explode with the amount of child document explanations.] Would be great to also add a test, which checks the child doc explanations are actually there.
        Hide
        jeff.w.evans Jeff Evans added a comment -

        It seems as though org.apache.lucene.search.CheckHits#verifyExplanation was already verifying that the scores were tying out, so I just added a simple assertion that makes sure that explanations are present at all for the child doc description line.

        Show
        jeff.w.evans Jeff Evans added a comment - It seems as though org.apache.lucene.search.CheckHits#verifyExplanation was already verifying that the scores were tying out, so I just added a simple assertion that makes sure that explanations are present at all for the child doc description line.
        Hide
        ilyak Ilya Kasnacheev added a comment -

        NB: It should perhaps be

        childWeight.explain(context, childDoc - context.docBase);

        in the original patch in order for this to work over multiple segments.

        Show
        ilyak Ilya Kasnacheev added a comment - NB: It should perhaps be childWeight.explain(context, childDoc - context.docBase); in the original patch in order for this to work over multiple segments.
        Hide
        ilyak Ilya Kasnacheev added a comment -

        I've prepared a revised patch. Only one child explanation is provided along with number of matched children. Test is also improved slightly.

        Show
        ilyak Ilya Kasnacheev added a comment - I've prepared a revised patch. Only one child explanation is provided along with number of matched children. Test is also improved slightly.
        Hide
        martijn.v.groningen Martijn van Groningen added a comment -

        Ilya: Thanks! This looks good. I'll push this shortly.

        Show
        martijn.v.groningen Martijn van Groningen added a comment - Ilya: Thanks! This looks good. I'll push this shortly.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit af98f35d21c0ab3e03bfe45a2ce980d7c574a8a0 in lucene-solr's branch refs/heads/branch_6x from Martijn van Groningen
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=af98f35 ]

        LUCENE-7206: Improve the ToParentBlockJoinQuery's explain by including the explain of the best matching child doc

        Show
        jira-bot ASF subversion and git services added a comment - Commit af98f35d21c0ab3e03bfe45a2ce980d7c574a8a0 in lucene-solr's branch refs/heads/branch_6x from Martijn van Groningen [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=af98f35 ] LUCENE-7206 : Improve the ToParentBlockJoinQuery's explain by including the explain of the best matching child doc
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit d29ab1e81279c87c57b5c83a635275ad00b9c896 in lucene-solr's branch refs/heads/master from Martijn van Groningen
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d29ab1e ]

        LUCENE-7206: Improve the ToParentBlockJoinQuery's explain by including the explain of the best matching child doc

        Show
        jira-bot ASF subversion and git services added a comment - Commit d29ab1e81279c87c57b5c83a635275ad00b9c896 in lucene-solr's branch refs/heads/master from Martijn van Groningen [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d29ab1e ] LUCENE-7206 : Improve the ToParentBlockJoinQuery's explain by including the explain of the best matching child doc
        Hide
        mkhludnev Mikhail Khludnev added a comment -

        Just a note, current approach might be too slow, it explains all children and then pickups one of these explanation. For short blocks it's ok, but for longer ones the two phase algorithm makes sense: find appropriate child (min/max or so), and explain it only after that.
        I prefer to keep it as a reminder, let's raise an issue if someone bother about it too.

        Show
        mkhludnev Mikhail Khludnev added a comment - Just a note, current approach might be too slow, it explains all children and then pickups one of these explanation. For short blocks it's ok, but for longer ones the two phase algorithm makes sense: find appropriate child (min/max or so), and explain it only after that. I prefer to keep it as a reminder, let's raise an issue if someone bother about it too.

          People

          • Assignee:
            Unassigned
            Reporter:
            mkhludnev Mikhail Khludnev
          • Votes:
            2 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development