Lucene - Core
  1. Lucene - Core
  2. LUCENE-2986

divorce defaultsimilarityprovider from defaultsimilarity

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      In LUCENE-2236 as a start, we made DefaultSimilarity which implements the factory interface (SimilarityProvider), and also extends Similarity.

      Its factory interface just returns itself always by default.

      Doron mentioned it would be cleaner to split the two, and I thought it would be good to revisit it later.

      Today as I was looking at SOLR-2338, it became pretty clear that we should do this, it makes things a lot cleaner. I think currently its confusing to users to see the two apis mixed if they are trying to subclass.

        Issue Links

          Activity

          Robert Muir created issue -
          Hide
          Robert Muir added a comment -

          Attached is a patch: adds DefaultSimilarityProvider, which has our default implementations of the non-field-specific methods (coord/queryNorm/etc), and always returns DefaultSimilarity.

          Show
          Robert Muir added a comment - Attached is a patch: adds DefaultSimilarityProvider, which has our default implementations of the non-field-specific methods (coord/queryNorm/etc), and always returns DefaultSimilarity.
          Robert Muir made changes -
          Field Original Value New Value
          Attachment LUCENE-2986.patch [ 12474413 ]
          Robert Muir made changes -
          Link This issue blocks SOLR-2338 [ SOLR-2338 ]
          Hide
          Doron Cohen added a comment -

          +1 for this change (I did not remember discussing this, but other than remembering I am consistent )

          Patch looks very clean.

          Minor technical comments - concerning just some tests:

          • some of the DSP implementations are still named xyzSimilarity - I think it would be more clear to name them xyzSimilarityProvider:
            • o.a.l.search.payloads.TestPayloadNearQuery.BoostingSimilarity
            • o.a.l.search.payloads.TestPayloadTermQuery.BoostingSimilarity
            • o.a.solr.schema.MockConfigurableSimilarity
            • o.a.l.index.TestIndexWriterConfig.MySimilarity
            • o.a.l.index.TestIndexReaderCloneNorms.SimilarityOne
            • o.a.l.index.TestNorms.SimilarityOne
            • o.a.l.index.TestOmitTf.SimpleSimilarity
            • o.a.l.search.TestSimilarity.SimpleSimilarity
          • for few of the above it is not only the name - they are still doing both roles:
            extends DefaultSimilarity implements SimilarityProvider

            :

            • o.a.l.search.payloads.TestPayloadNearQuery.BoostingSimilarity
            • o.a.l.search.payloads.TestPayloadTermQuery.BoostingSimilarity
            • o.a.l.index.TestOmitTf.SimpleSimilarity
            • o.a.l.search.TestSimilarity.SimpleSimilarity

          Other than that I think it is good to go in.

          Also, tests from trunk/lucene and trunk/solr passed.
          (I am seeing problems in running all trunk tests, at least on Windows, but I'll send a separate mail to the list on that)

          Show
          Doron Cohen added a comment - +1 for this change (I did not remember discussing this, but other than remembering I am consistent ) Patch looks very clean. Minor technical comments - concerning just some tests: some of the DSP implementations are still named xyzSimilarity - I think it would be more clear to name them xyzSimilarityProvider: o.a.l.search.payloads.TestPayloadNearQuery.BoostingSimilarity o.a.l.search.payloads.TestPayloadTermQuery.BoostingSimilarity o.a.solr.schema.MockConfigurableSimilarity o.a.l.index.TestIndexWriterConfig.MySimilarity o.a.l.index.TestIndexReaderCloneNorms.SimilarityOne o.a.l.index.TestNorms.SimilarityOne o.a.l.index.TestOmitTf.SimpleSimilarity o.a.l.search.TestSimilarity.SimpleSimilarity for few of the above it is not only the name - they are still doing both roles: extends DefaultSimilarity implements SimilarityProvider : o.a.l.search.payloads.TestPayloadNearQuery.BoostingSimilarity o.a.l.search.payloads.TestPayloadTermQuery.BoostingSimilarity o.a.l.index.TestOmitTf.SimpleSimilarity o.a.l.search.TestSimilarity.SimpleSimilarity Other than that I think it is good to go in. Also, tests from trunk/lucene and trunk/solr passed. (I am seeing problems in running all trunk tests, at least on Windows, but I'll send a separate mail to the list on that)
          Hide
          Robert Muir added a comment -

          Thanks, I agree the tests look messy/lazy... I'll fix these (well I think I would like to delay the solr one until SOLR-2338, i fixed the MockConfigurableSimilarity in that patch already)

          Show
          Robert Muir added a comment - Thanks, I agree the tests look messy/lazy... I'll fix these (well I think I would like to delay the solr one until SOLR-2338 , i fixed the MockConfigurableSimilarity in that patch already)
          Hide
          Robert Muir added a comment -

          Committed revision 1085374.

          I cleaned up all the tests as Doron suggested, except for the solr MockConfigurableSimilarity as I discussed (since solr scoring is not yet per-field, but i have a patch for this that fixes the naming issues there too).

          Show
          Robert Muir added a comment - Committed revision 1085374. I cleaned up all the tests as Doron suggested, except for the solr MockConfigurableSimilarity as I discussed (since solr scoring is not yet per-field, but i have a patch for this that fixes the naming issues there too).
          Robert Muir made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Gavin made changes -
          Link This issue blocks SOLR-2338 [ SOLR-2338 ]
          Gavin made changes -
          Link This issue is depended upon by SOLR-2338 [ SOLR-2338 ]
          Uwe Schindler made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          1d 21h 12m 1 Robert Muir 25/Mar/11 14:01
          Resolved Resolved Closed Closed
          776d 20h 41m 1 Uwe Schindler 10/May/13 11:42

            People

            • Assignee:
              Robert Muir
              Reporter:
              Robert Muir
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development