Lucene - Core
  1. Lucene - Core
  2. LUCENE-4701

Use MockAnalyzers in lucene/facet tests

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.0
    • Fix Version/s: 4.2, 5.0
    • Component/s: modules/facet
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      It'd be nicer to use MockAnalyzer in facet tests instead of specific analyzers from analyzers-common module.

      1. LUCENE-4701.patch
        5 kB
        Shai Erera
      2. LUCENE-4701.patch
        3 kB
        Tommaso Teofili

        Activity

        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Tommaso Teofili
        http://svn.apache.org/viewvc?view=revision&revision=1436355

        LUCENE-4701 - merged back to branch_4x

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Tommaso Teofili http://svn.apache.org/viewvc?view=revision&revision=1436355 LUCENE-4701 - merged back to branch_4x
        Hide
        Tommaso Teofili added a comment -

        yep, I just committed for both trunk and branch_4x.

        Show
        Tommaso Teofili added a comment - yep, I just committed for both trunk and branch_4x.
        Hide
        Shai Erera added a comment -

        Are you going to fix it for trunk too?

        Show
        Shai Erera added a comment - Are you going to fix it for trunk too?
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] Tommaso Teofili
        http://svn.apache.org/viewvc?view=revision&revision=1436346

        LUCENE-4701 - applied Shai's patch for using MockAnalyzer in tests and keeping analyzers-common dep only in examples classpath

        Show
        Commit Tag Bot added a comment - [trunk commit] Tommaso Teofili http://svn.apache.org/viewvc?view=revision&revision=1436346 LUCENE-4701 - applied Shai's patch for using MockAnalyzer in tests and keeping analyzers-common dep only in examples classpath
        Hide
        Shai Erera added a comment -

        yes go ahead. when we separate examples (requires changing some big tests that should stay under /facet), we'll know if the dissection is successful .

        Show
        Shai Erera added a comment - yes go ahead. when we separate examples (requires changing some big tests that should stay under /facet), we'll know if the dissection is successful .
        Hide
        Tommaso Teofili added a comment -

        ok cool, so I think we can go on committing your patch.

        Show
        Tommaso Teofili added a comment - ok cool, so I think we can go on committing your patch.
        Hide
        Shai Erera added a comment -

        Shai, can you think of any problems with regard to the null Analnyzer initialization in the DTW IndexWriterConfig?

        Now not, I think because of the changes to FieldType in Lucene 4.0. I.e., DirTaxoWriter adds indexed but not tokenized fields. Previously, you had to set an Analyzer, even if you added NOT_ANALYZED_NO_NORMS, or otherwise you'd hit NPE.

        If the tests pass, it means it's ok, because DTW is not extendable in a way that it allows you to control how documents are added. But if in the future we'll need to, we'll add this dependency back.

        Show
        Shai Erera added a comment - Shai, can you think of any problems with regard to the null Analnyzer initialization in the DTW IndexWriterConfig? Now not, I think because of the changes to FieldType in Lucene 4.0. I.e., DirTaxoWriter adds indexed but not tokenized fields. Previously, you had to set an Analyzer, even if you added NOT_ANALYZED_NO_NORMS, or otherwise you'd hit NPE. If the tests pass, it means it's ok, because DTW is not extendable in a way that it allows you to control how documents are added. But if in the future we'll need to, we'll add this dependency back.
        Hide
        Tommaso Teofili added a comment -

        I'm not against having analyzers-common as a dependency if that's needed (even in the main facet module) so I'm fine if that needs to be re-added in the future.
        Shai, can you think of any problems with regard to the null Analnyzer initialization in the DTW IndexWriterConfig?

        Show
        Tommaso Teofili added a comment - I'm not against having analyzers-common as a dependency if that's needed (even in the main facet module) so I'm fine if that needs to be re-added in the future. Shai, can you think of any problems with regard to the null Analnyzer initialization in the DTW IndexWriterConfig ?
        Hide
        Shai Erera added a comment -

        Removes KeywordAnalyzer from DirTaxoWriter.

        Also, I restored the dependency on analyzers in build.xml under examples.classpath, since they do depend on it. Once LUCENE-3998 is done, examples will be removed entirely under demo/ and this dependency will go away w/ them.

        A general statement about this dependency, I don't think it's a bad one, not for tests anyway. And maybe DirTaxoWriter will need it in the future too. I'm fine w/ removing it for now, but note that it may be restored in the future, if a real need arises.

        Show
        Shai Erera added a comment - Removes KeywordAnalyzer from DirTaxoWriter. Also, I restored the dependency on analyzers in build.xml under examples.classpath, since they do depend on it. Once LUCENE-3998 is done, examples will be removed entirely under demo/ and this dependency will go away w/ them. A general statement about this dependency, I don't think it's a bad one, not for tests anyway. And maybe DirTaxoWriter will need it in the future too. I'm fine w/ removing it for now, but note that it may be restored in the future, if a real need arises.
        Hide
        Shai Erera added a comment -

        needed by DirectoryTaxonomyWriter which internally uses a KeywordAnalyzer

        hmmm, right. I set the Analyzer used by DirTaxoWriter to null, and tests pass. However, as long as examples are still under facet/, we need that dependency in build.xml. It will be removed as part of LUCENE-3998.

        I will post a patch w/ my update shortly.

        Show
        Shai Erera added a comment - needed by DirectoryTaxonomyWriter which internally uses a KeywordAnalyzer hmmm, right. I set the Analyzer used by DirTaxoWriter to null, and tests pass. However, as long as examples are still under facet/, we need that dependency in build.xml. It will be removed as part of LUCENE-3998 . I will post a patch w/ my update shortly.
        Hide
        Shai Erera added a comment -

        I ran facet tests (which cover examples too) and they pass. Seems like not all analyzer dependencies were removed from build.xml. Working on that now.

        Show
        Shai Erera added a comment - I ran facet tests (which cover examples too) and they pass. Seems like not all analyzer dependencies were removed from build.xml. Working on that now.
        Hide
        Tommaso Teofili added a comment - - edited

        at the moment the analyzers-common dependency cannot be completely removed since it's needed by DirectoryTaxonomyWriter which internally uses a KeywordAnalyzer in its IndexWriterConfig so we can get rid of that only if that's made configurable (so we pass an Analyzer in the DTW constructor).

        Show
        Tommaso Teofili added a comment - - edited at the moment the analyzers-common dependency cannot be completely removed since it's needed by DirectoryTaxonomyWriter which internally uses a KeywordAnalyzer in its IndexWriterConfig so we can get rid of that only if that's made configurable (so we pass an Analyzer in the DTW constructor).
        Hide
        Shai Erera added a comment -

        ... facet/examples (which should move to demo/ imo)

        that's the part that's still needed to be done, under LUCENE-3998. I haven't forgotten about it .

        Show
        Shai Erera added a comment - ... facet/examples (which should move to demo/ imo) that's the part that's still needed to be done, under LUCENE-3998 . I haven't forgotten about it .
        Hide
        Robert Muir added a comment -

        does this really work... What about the analyzer usage in facet/examples (which should move to demo/ imo) ?

        Show
        Robert Muir added a comment - does this really work... What about the analyzer usage in facet/examples (which should move to demo/ imo) ?
        Hide
        Shai Erera added a comment -

        Great Tommaso. I was going to handle that under LUCENE-3998, so could you just drop a comment there about the analyzers dependency removal, after you commit this change?

        Show
        Shai Erera added a comment - Great Tommaso. I was going to handle that under LUCENE-3998 , so could you just drop a comment there about the analyzers dependency removal, after you commit this change?
        Hide
        Tommaso Teofili added a comment -

        and here's the related patch.

        Show
        Tommaso Teofili added a comment - and here's the related patch.

          People

          • Assignee:
            Tommaso Teofili
            Reporter:
            Tommaso Teofili
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development