Lucene - Core
  1. Lucene - Core
  2. LUCENE-5187

Make SlowCompositeReaderWrapper constructor private

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.5, Trunk
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      I found a couple of places in the code base that duplicate the logic of SlowCompositeReaderWrapper.wrap. I think SlowCompositeReaderWrapper.wrap (vs. new SlowCompositeReaderWrapper) is what users need so we should probably make SlowCompositeReaderWrapper constructor private to enforce usage of wrap.

      1. LUCENE-5187.patch
        16 kB
        Adrien Grand

        Activity

        Hide
        Adrien Grand added a comment -

        Here is a patch.

        Show
        Adrien Grand added a comment - Here is a patch.
        Hide
        Robert Muir added a comment -

        +1

        In the case of grouping tests, is it really safe to change this way?

               // NOTE: intentional but temporary field cache insanity!
        -      final FieldCache.Ints docIdToFieldId = FieldCache.DEFAULT.getInts(new SlowCompositeReaderWrapper(r), "id", false);
        +      final FieldCache.Ints docIdToFieldId = FieldCache.DEFAULT.getInts(SlowCompositeReaderWrapper.wrap(r), "id", false);
        

        if needed, Maybe instead, we should force compositeness by e.g. inserting an empty reader if necessary or whatever. but i'm not even sure what this test is doing.

        Show
        Robert Muir added a comment - +1 In the case of grouping tests, is it really safe to change this way? // NOTE: intentional but temporary field cache insanity! - final FieldCache.Ints docIdToFieldId = FieldCache.DEFAULT.getInts(new SlowCompositeReaderWrapper(r), "id", false); + final FieldCache.Ints docIdToFieldId = FieldCache.DEFAULT.getInts(SlowCompositeReaderWrapper.wrap(r), "id", false); if needed, Maybe instead, we should force compositeness by e.g. inserting an empty reader if necessary or whatever. but i'm not even sure what this test is doing.
        Hide
        Adrien Grand added a comment -

        Hmm, my understanding was that this test just needs a top-level fieldcache instance for testing purpose but I may be wrong...

        Show
        Adrien Grand added a comment - Hmm, my understanding was that this test just needs a top-level fieldcache instance for testing purpose but I may be wrong...
        Hide
        Robert Muir added a comment -

        My concern was the combination of the "intentional" comment and the fact it used the ctor (which forces wrapping always).

        I guess we can just see if it fails, and deal with it if so (by forcing compositeness).

        So I would just commit the patch for now!

        Show
        Robert Muir added a comment - My concern was the combination of the "intentional" comment and the fact it used the ctor (which forces wrapping always). I guess we can just see if it fails, and deal with it if so (by forcing compositeness). So I would just commit the patch for now!
        Hide
        Uwe Schindler added a comment -

        Stroooong ++++++1

        I wanted to do that long time, but some tests were made me afraid.

        Show
        Uwe Schindler added a comment - Stroooong ++++++1 I wanted to do that long time, but some tests were made me afraid.
        Hide
        Michael McCandless added a comment -

        +1 to commit, those grouping tests are fine if the wrapping doesn't happen (because the reader already happened to be atomic).

        Show
        Michael McCandless added a comment - +1 to commit, those grouping tests are fine if the wrapping doesn't happen (because the reader already happened to be atomic).
        Hide
        ASF subversion and git services added a comment -

        Commit 1517447 from Adrien Grand in branch 'dev/trunk'
        [ https://svn.apache.org/r1517447 ]

        LUCENE-5187: Make SlowCompositeReaderWrapper's constructor private.

        Show
        ASF subversion and git services added a comment - Commit 1517447 from Adrien Grand in branch 'dev/trunk' [ https://svn.apache.org/r1517447 ] LUCENE-5187 : Make SlowCompositeReaderWrapper's constructor private.
        Hide
        ASF subversion and git services added a comment -

        Commit 1517449 from Adrien Grand in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1517449 ]

        LUCENE-5187: Make SlowCompositeReaderWrapper's constructor private.

        Show
        ASF subversion and git services added a comment - Commit 1517449 from Adrien Grand in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1517449 ] LUCENE-5187 : Make SlowCompositeReaderWrapper's constructor private.
        Hide
        Adrien Grand added a comment -

        Thanks all for the review!

        Show
        Adrien Grand added a comment - Thanks all for the review!
        Hide
        Adrien Grand added a comment -

        4.5 release -> bulk close

        Show
        Adrien Grand added a comment - 4.5 release -> bulk close

          People

          • Assignee:
            Adrien Grand
            Reporter:
            Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development