Lucene - Core
  1. Lucene - Core
  2. LUCENE-2343

Add support for benchmarking Collectors

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: modules/benchmark
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      As the title says.

      1. LUCENE-2343.patch
        31 kB
        Grant Ingersoll
      2. LUCENE-2343.patch
        30 kB
        Grant Ingersoll

        Activity

        Hide
        Grant Ingersoll added a comment -

        Should be in the contrib/benchmark/CHANGES

        Show
        Grant Ingersoll added a comment - Should be in the contrib/benchmark/CHANGES
        Hide
        Shai Erera added a comment -

        I've just realized you haven't added a CHANGES entry (and I missed that in my previous review, sorry).

        Show
        Shai Erera added a comment - I've just realized you haven't added a CHANGES entry (and I missed that in my previous review, sorry).
        Hide
        Grant Ingersoll added a comment -

        Committed revision 927178.

        Show
        Grant Ingersoll added a comment - Committed revision 927178.
        Hide
        Shai Erera added a comment -

        Looks good !

        Show
        Shai Erera added a comment - Looks good !
        Hide
        Grant Ingersoll added a comment -

        I think this addresses all of Shai's concerns.

        Show
        Grant Ingersoll added a comment - I think this addresses all of Shai's concerns.
        Hide
        Grant Ingersoll added a comment -

        I wasn't talking about the name of the parameter but about the comment in the javadoc. TopDocsCollector is a typo - should have been TopScoreDocCollector. If you also want to change the name of the parameter in the .alg file that's ok as well, though I'm fine w/ topDocOrdered/Unordered.

        Yeah, I think I will change the name, too, but point taken. Will fix the docs.

        Show
        Grant Ingersoll added a comment - I wasn't talking about the name of the parameter but about the comment in the javadoc. TopDocsCollector is a typo - should have been TopScoreDocCollector. If you also want to change the name of the parameter in the .alg file that's ok as well, though I'm fine w/ topDocOrdered/Unordered. Yeah, I think I will change the name, too, but point taken. Will fix the docs.
        Hide
        Shai Erera added a comment -

        I wasn't talking about the name of the parameter but about the comment in the javadoc. TopDocsCollector is a typo - should have been TopScoreDocCollector. If you also want to change the name of the parameter in the .alg file that's ok as well, though I'm fine w/ topDocOrdered/Unordered.

        Show
        Shai Erera added a comment - I wasn't talking about the name of the parameter but about the comment in the javadoc. TopDocsCollector is a typo - should have been TopScoreDocCollector. If you also want to change the name of the parameter in the .alg file that's ok as well, though I'm fine w/ topDocOrdered/Unordered.
        Hide
        Grant Ingersoll added a comment -

        In the patch you write: "topDocOrdered - Creates a TopDocCollector that requires in order docs" - did you mean TopScoreDocCollector? Because TopDocCollector is abstract ...

        Yeah, I can make that topScoreDoc...

        Show
        Grant Ingersoll added a comment - In the patch you write: "topDocOrdered - Creates a TopDocCollector that requires in order docs" - did you mean TopScoreDocCollector? Because TopDocCollector is abstract ... Yeah, I can make that topScoreDoc...
        Hide
        Shai Erera added a comment -

        ok I won't argue about == true/false. It's a style thing and I'm not too fanatic about it .

        Show
        Shai Erera added a comment - ok I won't argue about == true/false. It's a style thing and I'm not too fanatic about it .
        Hide
        Grant Ingersoll added a comment -

        can be written as Class.forName(clnName).asSubclass(Collector.class).newInstance();

        OK

        Also, and it's a style issue, can you remove the '== true/false' from ifs?

        No. . Feel free to leave them out, but I prefer it to be explicit. See my rant at: http://www.lucidimagination.com/search/document/476a10c1c687971d/randomseedgenerator#684ed7253617ea71

        I'd change if (clnName.equals("") == false) to if (clnName.length() > 0).

        Sure.

        Why does benchmark/build.xml now relies on the compiled classes/test (of core)?

        Hmm, guess I didn't see that English got moved from test to core.

        Show
        Grant Ingersoll added a comment - can be written as Class.forName(clnName).asSubclass(Collector.class).newInstance(); OK Also, and it's a style issue, can you remove the '== true/false' from ifs? No. . Feel free to leave them out, but I prefer it to be explicit. See my rant at: http://www.lucidimagination.com/search/document/476a10c1c687971d/randomseedgenerator#684ed7253617ea71 I'd change if (clnName.equals("") == false) to if (clnName.length() > 0). Sure. Why does benchmark/build.xml now relies on the compiled classes/test (of core)? Hmm, guess I didn't see that English got moved from test to core.
        Hide
        Shai Erera added a comment -

        In the patch you write: "topDocOrdered - Creates a TopDocCollector that requires in order docs" - did you mean TopScoreDocCollector? Because TopDocCollector is abstract ...

        I think the following:

        +      Class<? extends Collector> clazz = (Class<? extends Collector>) Class.forName(clnName);
        +      collector = clazz.newInstance();
        

        can be written as Class.forName(clnName).asSubclass(Collector.class).newInstance();

        Also, and it's a style issue, can you remove the '== true/false' from ifs?

        I'd change if (clnName.equals("") == false) to if (clnName.length() > 0).

        Why does benchmark/build.xml now relies on the compiled classes/test (of core)?

        Show
        Shai Erera added a comment - In the patch you write: "topDocOrdered - Creates a TopDocCollector that requires in order docs" - did you mean TopScoreDocCollector? Because TopDocCollector is abstract ... I think the following: + Class <? extends Collector> clazz = ( Class <? extends Collector>) Class .forName(clnName); + collector = clazz.newInstance(); can be written as Class.forName(clnName).asSubclass(Collector.class).newInstance(); Also, and it's a style issue, can you remove the '== true/false' from ifs? I'd change if (clnName.equals("") == false) to if (clnName.length() > 0) . Why does benchmark/build.xml now relies on the compiled classes/test (of core)?
        Hide
        Grant Ingersoll added a comment -

        I intend to commit today or tomorrow.

        Show
        Grant Ingersoll added a comment - I intend to commit today or tomorrow.
        Hide
        Grant Ingersoll added a comment -

        Add support for benchmarking collectors

        Show
        Grant Ingersoll added a comment - Add support for benchmarking collectors

          People

          • Assignee:
            Grant Ingersoll
            Reporter:
            Grant Ingersoll
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development