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

Only assign ScoreDoc#shardIndex if it was already assigned to non default (-1) value

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 7.0, 6.5
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      When you use TopDocs.merge today it always overrides the ScoreDoc#shardIndex value. The assumption that is made here is that all shard results are merges at once which is not necessarily the case. If for instance incremental merge phases are applied the shard index doesn't correspond to the index in the outer TopDocs array. To make this a backwards compatible but yet non-controversial change we could change the internals of TopDocs#merge to only assign this value unless it's not been assigned before to a non-default (-1) value to allow multiple or sparse top docs merging.

      1. LUCENE-7707.patch
        15 kB
        Michael McCandless
      2. LUCENE-7707.patch
        13 kB
        Michael McCandless
      3. LUCENE-7707.patch
        13 kB
        Simon Willnauer
      4. LUCENE-7707.patch
        13 kB
        Simon Willnauer
      5. LUCENE-7707.patch
        10 kB
        Simon Willnauer
      6. LUCENE-7707.patch
        10 kB
        Simon Willnauer
      7. LUCENE-7707.patch
        8 kB
        Simon Willnauer
      8. LUCENE-7707.patch
        4 kB
        Simon Willnauer

        Activity

        Hide
        simonw Simon Willnauer added a comment -

        here is a patch

        Show
        simonw Simon Willnauer added a comment - here is a patch
        Hide
        simonw Simon Willnauer added a comment -

        here is another iteration that makes sorting stable and shares some code for tiebreaking between sorting and merge by score code

        Show
        simonw Simon Willnauer added a comment - here is another iteration that makes sorting stable and shares some code for tiebreaking between sorting and merge by score code
        Hide
        jim.ferenczi Jim Ferenczi added a comment -

        +1, this will make the merge more flexible.
        If we really want to be sure that it does not break the BWC maybe it can be an option of the merge function ? A simple boolean overrideShardIndex with a default value of false ?

        Show
        jim.ferenczi Jim Ferenczi added a comment - +1, this will make the merge more flexible. If we really want to be sure that it does not break the BWC maybe it can be an option of the merge function ? A simple boolean overrideShardIndex with a default value of false ?
        Hide
        simonw Simon Willnauer added a comment -

        I personally think making this solely dependent on a boolean would be best IMO. It would be an additional overload of the methods that explicitly turns on that shardIndex is set on the ScoreDoc and we don't need to do as much conditionals in the tie-breaking. I personally would like that but Michael McCandless had some issues with this?

        Show
        simonw Simon Willnauer added a comment - I personally think making this solely dependent on a boolean would be best IMO. It would be an additional overload of the methods that explicitly turns on that shardIndex is set on the ScoreDoc and we don't need to do as much conditionals in the tie-breaking. I personally would like that but Michael McCandless had some issues with this?
        Hide
        mikemccand Michael McCandless added a comment -

        +1 to the patch.

        I personally would like that but Michael McCandless had some issues with this?

        Yeah, I'd prefer not to add a boolean argument: that's allowing a temporary back compat issue to have a permanent impact on our APIs. Our APIs should be designed for future usage. Plus I think it's very unlikely someone today is pre-setting the shardIndex (off of it's default -1 value) and then relying on TopDocs.merge to overwrite it. I think the patch is sufficient back compat behavior w/o a compromised API change.

        Show
        mikemccand Michael McCandless added a comment - +1 to the patch. I personally would like that but Michael McCandless had some issues with this? Yeah, I'd prefer not to add a boolean argument: that's allowing a temporary back compat issue to have a permanent impact on our APIs. Our APIs should be designed for future usage. Plus I think it's very unlikely someone today is pre-setting the shardIndex (off of it's default -1 value) and then relying on TopDocs.merge to overwrite it. I think the patch is sufficient back compat behavior w/o a compromised API change.
        Hide
        jpountz Adrien Grand added a comment -

        I don't like the fact that if you mix top docs that have the shard index set and other instances that have it undefined, then we could end up assigning a shard id that is already in use. Is there a way we can avoid that?

        Show
        jpountz Adrien Grand added a comment - I don't like the fact that if you mix top docs that have the shard index set and other instances that have it undefined, then we could end up assigning a shard id that is already in use. Is there a way we can avoid that?
        Hide
        jim.ferenczi Jim Ferenczi added a comment -

        Plus I think it's very unlikely someone today is pre-setting the shardIndex (off of it's default -1 value) and then relying on TopDocs.merge

        Good point. +1 to the patch too, there's nothing to break here

        Show
        jim.ferenczi Jim Ferenczi added a comment - Plus I think it's very unlikely someone today is pre-setting the shardIndex (off of it's default -1 value) and then relying on TopDocs.merge Good point. +1 to the patch too, there's nothing to break here
        Hide
        mikemccand Michael McCandless added a comment -

        Maybe we could require that either all incoming shardIndex are undefined, or all are set, but you are not allowed to mix?

        Show
        mikemccand Michael McCandless added a comment - Maybe we could require that either all incoming shardIndex are undefined, or all are set, but you are not allowed to mix?
        Hide
        simonw Simon Willnauer added a comment -

        Maybe we could require that either all incoming shardIndex are undefined, or all are set, but you are not allowed to mix?

        I think this is what we should ultimately do. I don't see a different way than peaking at the at the TopDocs so see if it's preset and then executed based on that. I can certainly add assertions...

        Show
        simonw Simon Willnauer added a comment - Maybe we could require that either all incoming shardIndex are undefined, or all are set, but you are not allowed to mix? I think this is what we should ultimately do. I don't see a different way than peaking at the at the TopDocs so see if it's preset and then executed based on that. I can certainly add assertions...
        Hide
        simonw Simon Willnauer added a comment -

        here is a new patch adding more safety to it and making the decision up-front if we assign shardIndex or not

        Show
        simonw Simon Willnauer added a comment - here is a new patch adding more safety to it and making the decision up-front if we assign shardIndex or not
        Hide
        simonw Simon Willnauer added a comment -

        s/where/were

        Show
        simonw Simon Willnauer added a comment - s/where/were
        Hide
        mikemccand Michael McCandless added a comment -

        +1, looks awesome!

        Maybe update the javadocs to explain that we will either fill in the shardIndex or will not but never both.

        Show
        mikemccand Michael McCandless added a comment - +1, looks awesome! Maybe update the javadocs to explain that we will either fill in the shardIndex or will not but never both.
        Hide
        simonw Simon Willnauer added a comment -

        updated javadocs

        Show
        simonw Simon Willnauer added a comment - updated javadocs
        Hide
        mikemccand Michael McCandless added a comment -

        +1, thanks Simon Willnauer.

        Show
        mikemccand Michael McCandless added a comment - +1, thanks Simon Willnauer .
        Hide
        simonw Simon Willnauer added a comment -

        fix typo s/loosing/losing

        Show
        simonw Simon Willnauer added a comment - fix typo s/loosing/losing
        Hide
        simonw Simon Willnauer added a comment -

        Adrien Grand are your concerns addressed with the latest patch?

        Show
        simonw Simon Willnauer added a comment - Adrien Grand are your concerns addressed with the latest patch?
        Hide
        jpountz Adrien Grand added a comment -

        Yes, thanks for adding this validation. +1 to the patch

        Show
        jpountz Adrien Grand added a comment - Yes, thanks for adding this validation. +1 to the patch
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 5eeb8136f34fc7b3e2157982e5fa42da7f115d76 in lucene-solr's branch refs/heads/master from Simon Willnauer
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5eeb813 ]

        LUCENE-7707: Use predefined shard index when mergeing top docs if present.

        This allows to use TopDoc#merge to merge shard responses incrementally
        instead of once all shard responses are present.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 5eeb8136f34fc7b3e2157982e5fa42da7f115d76 in lucene-solr's branch refs/heads/master from Simon Willnauer [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5eeb813 ] LUCENE-7707 : Use predefined shard index when mergeing top docs if present. This allows to use TopDoc#merge to merge shard responses incrementally instead of once all shard responses are present.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7f904a2bccad38929ced27b5ccd81fbc6df5d8ff in lucene-solr's branch refs/heads/branch_6x from Simon Willnauer
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7f904a2 ]

        LUCENE-7707: Use predefined shard index when mergeing top docs if present.

        This allows to use TopDoc#merge to merge shard responses incrementally
        instead of once all shard responses are present.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7f904a2bccad38929ced27b5ccd81fbc6df5d8ff in lucene-solr's branch refs/heads/branch_6x from Simon Willnauer [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7f904a2 ] LUCENE-7707 : Use predefined shard index when mergeing top docs if present. This allows to use TopDoc#merge to merge shard responses incrementally instead of once all shard responses are present.
        Hide
        simonw Simon Willnauer added a comment - - edited

        thanks everybody! it's good to be back here

        Show
        simonw Simon Willnauer added a comment - - edited thanks everybody! it's good to be back here
        Hide
        mikemccand Michael McCandless added a comment -

        Alas, I think we do need to add an explicit boolean to the public API ... this test failure repros for me:

         [junit4] Suite: org.apache.lucene.search.TestShardSearching
           [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestShardSearching -Dtests.method=testSimple -Dtests.seed=2D10A476239970A9 -Dtests.slow=true -Dtests.locale=en-NZ -Dtests.timezone=Navajo -Dtests.asserts=true -Dtests.file.encoding=UTF8
           [junit4] FAILURE 0.64s J2 | TestShardSearching.testSimple <<<
           [junit4]    > Throwable #1: java.lang.AssertionError
           [junit4]    > 	at __randomizedtesting.SeedInfo.seed([2D10A476239970A9:15A38088046AA478]:0)
           [junit4]    > 	at org.apache.lucene.search.TopDocs.tieBreakLessThan(TopDocs.java:104)
           [junit4]    > 	at org.apache.lucene.search.TopDocs$MergeSortQueue.lessThan(TopDocs.java:196)
           [junit4]    > 	at org.apache.lucene.search.TopDocs$MergeSortQueue.lessThan(TopDocs.java:138)
           [junit4]    > 	at org.apache.lucene.util.PriorityQueue.upHeap(PriorityQueue.java:263)
           [junit4]    > 	at org.apache.lucene.util.PriorityQueue.add(PriorityQueue.java:140)
           [junit4]    > 	at org.apache.lucene.search.TopDocs.mergeAux(TopDocs.java:283)
           [junit4]    > 	at org.apache.lucene.search.TopDocs.merge(TopDocs.java:248)
           [junit4]    > 	at org.apache.lucene.search.TopDocs.merge(TopDocs.java:232)
           [junit4]    > 	at org.apache.lucene.search.ShardSearchingTestBase$NodeState$ShardIndexSearcher.search(ShardSearchingTestBase.java:440)
           [junit4]    > 	at org.apache.lucene.search.TestShardSearching.assertSame(TestShardSearching.java:313)
           [junit4]    > 	at org.apache.lucene.search.TestShardSearching.testSimple(TestShardSearching.java:236)
           [junit4]    > 	at java.lang.Thread.run(Thread.java:745)
        

        It happens because IndexSearcher will sometimes (when using an executor) set the shardIndex of the TopDocs it returns to the caller, but it should not (so that the caller can then later do their own merging). Likewise, grouping, drill sideways ...

        I'll work on a patch to make it explicit...

        Show
        mikemccand Michael McCandless added a comment - Alas, I think we do need to add an explicit boolean to the public API ... this test failure repros for me: [junit4] Suite: org.apache.lucene.search.TestShardSearching [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestShardSearching -Dtests.method=testSimple -Dtests.seed=2D10A476239970A9 -Dtests.slow=true -Dtests.locale=en-NZ -Dtests.timezone=Navajo -Dtests.asserts=true -Dtests.file.encoding=UTF8 [junit4] FAILURE 0.64s J2 | TestShardSearching.testSimple <<< [junit4] > Throwable #1: java.lang.AssertionError [junit4] > at __randomizedtesting.SeedInfo.seed([2D10A476239970A9:15A38088046AA478]:0) [junit4] > at org.apache.lucene.search.TopDocs.tieBreakLessThan(TopDocs.java:104) [junit4] > at org.apache.lucene.search.TopDocs$MergeSortQueue.lessThan(TopDocs.java:196) [junit4] > at org.apache.lucene.search.TopDocs$MergeSortQueue.lessThan(TopDocs.java:138) [junit4] > at org.apache.lucene.util.PriorityQueue.upHeap(PriorityQueue.java:263) [junit4] > at org.apache.lucene.util.PriorityQueue.add(PriorityQueue.java:140) [junit4] > at org.apache.lucene.search.TopDocs.mergeAux(TopDocs.java:283) [junit4] > at org.apache.lucene.search.TopDocs.merge(TopDocs.java:248) [junit4] > at org.apache.lucene.search.TopDocs.merge(TopDocs.java:232) [junit4] > at org.apache.lucene.search.ShardSearchingTestBase$NodeState$ShardIndexSearcher.search(ShardSearchingTestBase.java:440) [junit4] > at org.apache.lucene.search.TestShardSearching.assertSame(TestShardSearching.java:313) [junit4] > at org.apache.lucene.search.TestShardSearching.testSimple(TestShardSearching.java:236) [junit4] > at java.lang.Thread.run(Thread.java:745) It happens because IndexSearcher will sometimes (when using an executor) set the shardIndex of the TopDocs it returns to the caller, but it should not (so that the caller can then later do their own merging). Likewise, grouping, drill sideways ... I'll work on a patch to make it explicit...
        Hide
        mikemccand Michael McCandless added a comment -

        OK, here's a patch to make the boolean setShardIndex explicit; I just feed that down to mergeAux and replace the auto-detect logic it previously had.

        There is no back-compat break: I default the original APIs to passing setShardIndex=true. I think it's ready.

        Show
        mikemccand Michael McCandless added a comment - OK, here's a patch to make the boolean setShardIndex explicit; I just feed that down to mergeAux and replace the auto-detect logic it previously had. There is no back-compat break: I default the original APIs to passing setShardIndex=true . I think it's ready.
        Hide
        jpountz Adrien Grand added a comment -

        Let's make sure that the shard index is not -1 if setShardIndex is false? Otherwise + 1.

        Show
        jpountz Adrien Grand added a comment - Let's make sure that the shard index is not -1 if setShardIndex is false? Otherwise + 1.
        Hide
        mikemccand Michael McCandless added a comment -

        Thanks Adrien Grand; I added checking for that abuse, and the original test case for this...

        Show
        mikemccand Michael McCandless added a comment - Thanks Adrien Grand ; I added checking for that abuse, and the original test case for this...
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit d00c5cae2b80941bbe71c091d42659e0c504b5ec in lucene-solr's branch refs/heads/branch_6x from Mike McCandless
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d00c5ca ]

        LUCENE-7707: add explicit boolean to TopDocs.merge to govern whether incoming or implicit shard index should be used

        Show
        jira-bot ASF subversion and git services added a comment - Commit d00c5cae2b80941bbe71c091d42659e0c504b5ec in lucene-solr's branch refs/heads/branch_6x from Mike McCandless [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d00c5ca ] LUCENE-7707 : add explicit boolean to TopDocs.merge to govern whether incoming or implicit shard index should be used
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 2e56c0e50564c8feeeb2831dd36cff1e9b23a00f in lucene-solr's branch refs/heads/master from Mike McCandless
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2e56c0e ]

        LUCENE-7707: add explicit boolean to TopDocs.merge to govern whether incoming or implicit shard index should be used

        Show
        jira-bot ASF subversion and git services added a comment - Commit 2e56c0e50564c8feeeb2831dd36cff1e9b23a00f in lucene-solr's branch refs/heads/master from Mike McCandless [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2e56c0e ] LUCENE-7707 : add explicit boolean to TopDocs.merge to govern whether incoming or implicit shard index should be used
        Hide
        mikemccand Michael McCandless added a comment -

        OK I committed that last patch.

        Show
        mikemccand Michael McCandless added a comment - OK I committed that last patch.

          People

          • Assignee:
            simonw Simon Willnauer
            Reporter:
            simonw Simon Willnauer
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development