Lucene - Core
  1. Lucene - Core
  2. LUCENE-3687

Allow similarity to encode norms other than a single byte

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 4.0-ALPHA
    • Component/s: core/index, core/search
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      LUCENE-3628 cut over norms to docvalues. This removes the long standing limitation that norms are a single byte. Yet, we still need to expose this functionality to Similarity to write / encode norms in a different format.

      1. LUCENE-3687.patch
        85 kB
        Simon Willnauer
      2. LUCENE-3687.patch
        93 kB
        Simon Willnauer
      3. LUCENE-3687.patch
        104 kB
        Simon Willnauer
      4. LUCENE-3687.patch
        149 kB
        Simon Willnauer
      5. LUCENE-3687.patch
        159 kB
        Simon Willnauer

        Issue Links

          Activity

          Simon Willnauer created issue -
          Hide
          Simon Willnauer added a comment -

          here is a first patch allowing similarity be more flexible when encoding norms. This patch allows similarity to write all numeric values plus fixed bytes. I tried to keep the actual DocValues type out of similarity to reduce complexity. From my perspective this is a fair game here not exposing the entire power of DocValues to similarity to reduce complexity in the API.

          I cleaned up the Lucene4FieldInfoFormat and added a preflex and 3x version. The 3x version is read-only and only deals with what was supported in 3x. 4x FieldInfo format has no backwards logic in there anymore too.

          The preflex version is slightly different. Even that this is a 3x mock codec for RW support I need to write fieldnumbers since the actual FI ord might be different from the field number assigned by the trunk IW. so I have a special format for this.

          there is certainly room for improvement but its a start. comments are welcome

          Show
          Simon Willnauer added a comment - here is a first patch allowing similarity be more flexible when encoding norms. This patch allows similarity to write all numeric values plus fixed bytes. I tried to keep the actual DocValues type out of similarity to reduce complexity. From my perspective this is a fair game here not exposing the entire power of DocValues to similarity to reduce complexity in the API. I cleaned up the Lucene4FieldInfoFormat and added a preflex and 3x version. The 3x version is read-only and only deals with what was supported in 3x. 4x FieldInfo format has no backwards logic in there anymore too. The preflex version is slightly different. Even that this is a 3x mock codec for RW support I need to write fieldnumbers since the actual FI ord might be different from the field number assigned by the trunk IW. so I have a special format for this. there is certainly room for improvement but its a start. comments are welcome
          Simon Willnauer made changes -
          Field Original Value New Value
          Attachment LUCENE-3687.patch [ 12510176 ]
          Hide
          Simon Willnauer added a comment -

          what a typo... s/omit/emit in the patch! anyway I think we should name the methos setXXX instead.

          Show
          Simon Willnauer added a comment - what a typo... s/omit/emit in the patch! anyway I think we should name the methos setXXX instead.
          Hide
          Robert Muir added a comment -

          I havent done a full review but with the typo fix I am very happy with the sim api.

          I also understand the preflex problem and I totally agree with the solution here.

          Show
          Robert Muir added a comment - I havent done a full review but with the typo fix I am very happy with the sim api. I also understand the preflex problem and I totally agree with the solution here.
          Hide
          Simon Willnauer added a comment -

          new patch with setXXX methods on Norm class. I also fixed some JavaDoc issues since computeNorm signature has changed. All tests pass but still need to add JavaDoc to the norms class.

          patch created with svn diff --show-copies-as-adds for readability.

          Show
          Simon Willnauer added a comment - new patch with setXXX methods on Norm class. I also fixed some JavaDoc issues since computeNorm signature has changed. All tests pass but still need to add JavaDoc to the norms class. patch created with svn diff --show-copies-as-adds for readability.
          Simon Willnauer made changes -
          Attachment LUCENE-3687.patch [ 12510361 ]
          Hide
          Simon Willnauer added a comment -

          new patch. added some cleanups + javadoc.
          I now throw a hard IArgExc if sim emits different types for the same field and added tests to check if that is happening.

          I also remove setByte in favor of setInt(byte)

          I think its ready!

          Show
          Simon Willnauer added a comment - new patch. added some cleanups + javadoc. I now throw a hard IArgExc if sim emits different types for the same field and added tests to check if that is happening. I also remove setByte in favor of setInt(byte) I think its ready!
          Simon Willnauer made changes -
          Attachment LUCENE-3687.patch [ 12510477 ]
          Hide
          Robert Muir added a comment -

          I also remove setByte in favor of setInt(byte)
          {quote]

          Why? this is really confusing! I think setByte should take byte, setInt int, setLong, long etc.

          Show
          Robert Muir added a comment - I also remove setByte in favor of setInt(byte) {quote] Why? this is really confusing! I think setByte should take byte, setInt int, setLong, long etc.
          Hide
          Robert Muir added a comment -

          I just saw DocValuesField has this same problem. Ill open a separate issue for that.

          But this is a real serious trap. Overloading setint as setInt(short) setInt(int) setInt(byte) setInt(long),
          besides being confusing (and I see no advantage to doing this), has the nice property
          of easily quadrupling peoples norms with no type safety unless they are like, peeking at
          their generated bytecode to verify java didn't promote them up to setInt(int)

          by making it setByte(byte) instead, if they don't pass an actual byte they will get
          a compile error.

          Show
          Robert Muir added a comment - I just saw DocValuesField has this same problem. Ill open a separate issue for that. But this is a real serious trap. Overloading setint as setInt(short) setInt(int) setInt(byte) setInt(long), besides being confusing (and I see no advantage to doing this), has the nice property of easily quadrupling peoples norms with no type safety unless they are like, peeking at their generated bytecode to verify java didn't promote them up to setInt(int) by making it setByte(byte) instead, if they don't pass an actual byte they will get a compile error.
          Hide
          Uwe Schindler added a comment - - edited

          Why? this is really confusing! I think setByte should take byte, setInt int, setLong, long etc.

          I agree. That's also a problem in other DocValues use cases like sorting. The SortField name uses INT but sorts by long and so on. There is already an issue open: LUCENE-3192

          Show
          Uwe Schindler added a comment - - edited Why? this is really confusing! I think setByte should take byte, setInt int, setLong, long etc. I agree. That's also a problem in other DocValues use cases like sorting. The SortField name uses INT but sorts by long and so on. There is already an issue open: LUCENE-3192
          Hide
          Simon Willnauer added a comment -

          I agree, I will change this accordingly for all methods in Norm. Any other comments?

          Show
          Simon Willnauer added a comment - I agree, I will change this accordingly for all methods in Norm. Any other comments?
          Uwe Schindler made changes -
          Link This issue requires LUCENE-3694 [ LUCENE-3694 ]
          Hide
          Robert Muir added a comment -

          I think otherwise the patch is great.

          we really needed to split out fieldinfos implementations for 3.x and 4.x, for example
          this will be useful to support offsets in the postings lists for lucene4xcodec (LUCENE-3684)

          a few minor/trivial comments:

          in PreFlexRW and SimpleText's impl:
          throw new IllegalArgumentException("Codec only supports single byte norm values. Type give: " + type);
          I think this should be UOE?

          shouldn't we combine Norm and DocValueNorm into one final class (Norm) and pull it out of Sim?
          I don't think it needs to be inside Sim anymore, as I don't see any usefulness to making your own
          subclass (how will this affect scoring versus just using the DocValueNorm impl)

          Is there any other use case to Norm being abstract? iff there is, shouldnt normsconsumer[perfield] pull the impl from the codec?

          Show
          Robert Muir added a comment - I think otherwise the patch is great. we really needed to split out fieldinfos implementations for 3.x and 4.x, for example this will be useful to support offsets in the postings lists for lucene4xcodec ( LUCENE-3684 ) a few minor/trivial comments: in PreFlexRW and SimpleText's impl: throw new IllegalArgumentException("Codec only supports single byte norm values. Type give: " + type); I think this should be UOE? shouldn't we combine Norm and DocValueNorm into one final class (Norm) and pull it out of Sim? I don't think it needs to be inside Sim anymore, as I don't see any usefulness to making your own subclass (how will this affect scoring versus just using the DocValueNorm impl) Is there any other use case to Norm being abstract? iff there is, shouldnt normsconsumer [perfield] pull the impl from the codec?
          Hide
          Robert Muir added a comment -

          Two more questions:

          In the patch omitNorms is still a separate boolean from normValueType == null.
          If this is the case, aren't we now able to represent (in 4.x codec) that norms are not omitted for the field,
          they just don't exist at all (e.g. all documents without norms are deleted).
          Couldn't we take advantage of this to provide a true fix for LUCENE-3678, removing the bogus fileExists() stuff in NormsFormat's files()?

          What happens if a sim doesnt ever set anything in computeNorm at all for a field?
          Previously this was 'required' because the method required that you return a byte.
          I think, if we do the above and are able to separately represent 'doesnt hvae any norms for this segment'
          from 'omitNorms for this field always', that we would just have normValueType as null, and it would all just work?

          Show
          Robert Muir added a comment - Two more questions: In the patch omitNorms is still a separate boolean from normValueType == null. If this is the case, aren't we now able to represent (in 4.x codec) that norms are not omitted for the field, they just don't exist at all (e.g. all documents without norms are deleted). Couldn't we take advantage of this to provide a true fix for LUCENE-3678 , removing the bogus fileExists() stuff in NormsFormat's files()? What happens if a sim doesnt ever set anything in computeNorm at all for a field? Previously this was 'required' because the method required that you return a byte. I think, if we do the above and are able to separately represent 'doesnt hvae any norms for this segment' from 'omitNorms for this field always', that we would just have normValueType as null, and it would all just work?
          Hide
          Simon Willnauer added a comment -

          in PreFlexRW and SimpleText's impl:

          I agree this should be UOE

          shouldn't we combine Norm and DocValueNorm into one final class (Norm) and pull it out of Sim?

          I kind of like the separation here. DocValues is really the Codec side of things and it would be nice if we would not mix that stuff in the similarity API.

          If this is the case, aren't we now able to represent (in 4.x codec) that norms are not omitted for the field, they just don't exist at all (e.g. all documents without norms are deleted).

          yeah we can do that I will look into it but I am not sure if we should rather let that patch bake in for a bit and then do that change in a second issue. Would make debugging simpler if we run into problems.

          from 'omitNorms for this field always', that we would just have normValueType as null, and it would all just work?

          I agree, maybe its better to get this right in this patch already I can still move the stupid file checks removed in a second issue. But I should really handle null types ie. Sims that don't set a value, currently I have tons of asserts that enforce a value.

          Show
          Simon Willnauer added a comment - in PreFlexRW and SimpleText's impl: I agree this should be UOE shouldn't we combine Norm and DocValueNorm into one final class (Norm) and pull it out of Sim? I kind of like the separation here. DocValues is really the Codec side of things and it would be nice if we would not mix that stuff in the similarity API. If this is the case, aren't we now able to represent (in 4.x codec) that norms are not omitted for the field, they just don't exist at all (e.g. all documents without norms are deleted). yeah we can do that I will look into it but I am not sure if we should rather let that patch bake in for a bit and then do that change in a second issue. Would make debugging simpler if we run into problems. from 'omitNorms for this field always', that we would just have normValueType as null, and it would all just work? I agree, maybe its better to get this right in this patch already I can still move the stupid file checks removed in a second issue. But I should really handle null types ie. Sims that don't set a value, currently I have tons of asserts that enforce a value.
          Hide
          Robert Muir added a comment -

          I kind of like the separation here. DocValues is really the Codec side of things and it would be nice if we would not mix that stuff in the similarity API.

          But what is the use case of having a separate Abstract class (Norm) from an implementation class (DocValueNorm) that the codec doesn't even provide (its instantiated as DocValues by normsconsumer[perfield] directly.

          If we are going to have separate classes, then norm should be abstract and the codec should provide the implementation. But we need a real use case as to why a codec would need to customize the implementation of Norm to justify this.

          Show
          Robert Muir added a comment - I kind of like the separation here. DocValues is really the Codec side of things and it would be nice if we would not mix that stuff in the similarity API. But what is the use case of having a separate Abstract class (Norm) from an implementation class (DocValueNorm) that the codec doesn't even provide (its instantiated as DocValues by normsconsumer [perfield] directly. If we are going to have separate classes, then norm should be abstract and the codec should provide the implementation. But we need a real use case as to why a codec would need to customize the implementation of Norm to justify this.
          Hide
          Simon Willnauer added a comment -

          If we are going to have separate classes, then norm should be abstract and the codec should provide the implementation. But we need a real use case as to why a codec would need to customize the implementation of Norm to justify this.

          maybe we should. if a user wants to do crazy stuff with norms ie. use packed ints for norms or var length bytes they should be able the do this ie return a different type then we do right now?

          Show
          Simon Willnauer added a comment - If we are going to have separate classes, then norm should be abstract and the codec should provide the implementation. But we need a real use case as to why a codec would need to customize the implementation of Norm to justify this. maybe we should. if a user wants to do crazy stuff with norms ie. use packed ints for norms or var length bytes they should be able the do this ie return a different type then we do right now?
          Hide
          Robert Muir added a comment -

          maybe we should. if a user wants to do crazy stuff with norms ie. use packed ints for norms or var length bytes they should be able the do this ie return a different type then we do right now?

          What crazy stuff would they be doing? Keep in mind that the only thing you can put in a norm is stuff from FieldInvertState.

          Show
          Robert Muir added a comment - maybe we should. if a user wants to do crazy stuff with norms ie. use packed ints for norms or var length bytes they should be able the do this ie return a different type then we do right now? What crazy stuff would they be doing? Keep in mind that the only thing you can put in a norm is stuff from FieldInvertState.
          Hide
          Robert Muir added a comment -

          yeah we can do that I will look into it but I am not sure if we should rather let that patch bake in for a bit and then do that change in a second issue. Would make debugging simpler if we run into problems.

          I agree, that case is crazy today and it shouldn't block nor confuse the issue. I just wanted us to have a plan for the file format. Otherwise there is no point in writing OMIT_NORMS bit in the fieldinfoswriter because it could be represented by normValueType of 0.

          I agree, maybe its better to get this right in this patch already I can still move the stupid file checks removed in a second issue. But I should really handle null types ie. Sims that don't set a value, currently I have tons of asserts that enforce a value.

          This isn't a huge deal though, its mostly just curiousity. Previously you always had to return something, we didnt even have the option for a sim (like basic tf * idf) to not encode any length normalization information. The way you had to do that before was to return a bogus byte in computeNorm and ensure you always did omitNorms for the field.

          If its tricky or messy, in my opinion we could even just add an assertion for now and document "you must set something" in Similarity, because its a lower level API than it was in previous release (most people would generally extend higher level stuff like BM25Similarity, TFIDFSimilarity, or even DefaultSimilarity that do not expose this stuff).

          Show
          Robert Muir added a comment - yeah we can do that I will look into it but I am not sure if we should rather let that patch bake in for a bit and then do that change in a second issue. Would make debugging simpler if we run into problems. I agree, that case is crazy today and it shouldn't block nor confuse the issue. I just wanted us to have a plan for the file format. Otherwise there is no point in writing OMIT_NORMS bit in the fieldinfoswriter because it could be represented by normValueType of 0. I agree, maybe its better to get this right in this patch already I can still move the stupid file checks removed in a second issue. But I should really handle null types ie. Sims that don't set a value, currently I have tons of asserts that enforce a value. This isn't a huge deal though, its mostly just curiousity. Previously you always had to return something, we didnt even have the option for a sim (like basic tf * idf) to not encode any length normalization information. The way you had to do that before was to return a bogus byte in computeNorm and ensure you always did omitNorms for the field. If its tricky or messy, in my opinion we could even just add an assertion for now and document "you must set something" in Similarity, because its a lower level API than it was in previous release (most people would generally extend higher level stuff like BM25Similarity, TFIDFSimilarity, or even DefaultSimilarity that do not expose this stuff).
          Hide
          Simon Willnauer added a comment -

          new patch

          • renamed setXXX to setDouble, setFloat etc.
          • moved Norm our of similarity and merged it with DocValueNorm
          • use UOE in simpletext and 3x norms
          • utilize normValueType to actually not write norms if only bogus norms would be written. Lucene4 can now check if norms are present without looking at the directory.

          this patch actually doesn't even write norms if not absolutely needed. Do we have any requirement that this needs to happen even if there is not a single doc with norms in that field? I removed all the crazy 0 value writing in NormsConsumer and still all tests pass. If there is a case where these bogus norms are required we need to have a test for it.

          Show
          Simon Willnauer added a comment - new patch renamed setXXX to setDouble, setFloat etc. moved Norm our of similarity and merged it with DocValueNorm use UOE in simpletext and 3x norms utilize normValueType to actually not write norms if only bogus norms would be written. Lucene4 can now check if norms are present without looking at the directory. this patch actually doesn't even write norms if not absolutely needed. Do we have any requirement that this needs to happen even if there is not a single doc with norms in that field? I removed all the crazy 0 value writing in NormsConsumer and still all tests pass. If there is a case where these bogus norms are required we need to have a test for it.
          Simon Willnauer made changes -
          Attachment LUCENE-3687.patch [ 12510622 ]
          Hide
          Michael McCandless added a comment -

          This looks great! I love how you pass Norm to the sim and it sets
          what it wants...

          Hmm... Norm impls DocValue which I just removed... maybe we should add
          it back?

          Very nice to have a private 3x FieldInfos reader/writer!

          What happens if an app (incorrectly) tries to change up the normType
          suddenly...? If you change in the middle of an IW session... it's
          quietly ignored and you get 0/empty bytes indexed instead?
          (FieldInfo.setNormValueType silently does nothing if it's already
          set). Can we throw an exception if it's already set to a conflicting
          value?

          If you change on a new IW session... they will be merged according to
          the normal type promotion rules of doc values right?

          Maybe instead of set and reset methods on FieldInfo just have a single
          set method with force boolean?

          This fails for me:

              [junit] Testsuite: org.apache.solr.search.function.TestFunctionQuery
              [junit] Tests run: 1, Failures: 0, Errors: 1, Time elapsed: 0.939 sec
              [junit] 
              [junit] ------------- Standard Error -----------------
              [junit] NOTE: reproduce with: ant test -Dtestcase=TestFunctionQuery -Dtestmethod=testGeneral -Dtests.seed=-406004cdb5fb0321:-4f9a2e7a9fa49aa4:7de466246285c0e9 -Dargs="-Dfile.encoding=UTF-8"
              [junit] NOTE: test params are: codec=Lucene40: {}, sim=RandomSimilarityProvider(queryNorm=true,coord=true): {}, locale=ro, timezone=Europe/Malta
              [junit] NOTE: all tests run in this JVM:
              [junit] [TestFunctionQuery]
              [junit] NOTE: Linux 2.6.33.6-147.fc13.x86_64 amd64/Sun Microsystems Inc. 1.6.0_21 (64-bit)/cpus=24,threads=1,free=171062240,total=239534080
              [junit] ------------- ---------------- ---------------
              [junit] Testcase: testGeneral(org.apache.solr.search.function.TestFunctionQuery):	Caused an ERROR
              [junit] null
              [junit] java.lang.NullPointerException
              [junit] 	at org.apache.solr.search.function.TestFunctionQuery.testGeneral(TestFunctionQuery.java:352)
              [junit] 	at org.apache.lucene.util.LuceneTestCase$3$1.evaluate(LuceneTestCase.java:528)
              [junit] 	at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:165)
              [junit] 	at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:57)
          
          Show
          Michael McCandless added a comment - This looks great! I love how you pass Norm to the sim and it sets what it wants... Hmm... Norm impls DocValue which I just removed... maybe we should add it back? Very nice to have a private 3x FieldInfos reader/writer! What happens if an app (incorrectly) tries to change up the normType suddenly...? If you change in the middle of an IW session... it's quietly ignored and you get 0/empty bytes indexed instead? (FieldInfo.setNormValueType silently does nothing if it's already set). Can we throw an exception if it's already set to a conflicting value? If you change on a new IW session... they will be merged according to the normal type promotion rules of doc values right? Maybe instead of set and reset methods on FieldInfo just have a single set method with force boolean? This fails for me: [junit] Testsuite: org.apache.solr.search.function.TestFunctionQuery [junit] Tests run: 1, Failures: 0, Errors: 1, Time elapsed: 0.939 sec [junit] [junit] ------------- Standard Error ----------------- [junit] NOTE: reproduce with: ant test -Dtestcase=TestFunctionQuery -Dtestmethod=testGeneral -Dtests.seed=-406004cdb5fb0321:-4f9a2e7a9fa49aa4:7de466246285c0e9 -Dargs="-Dfile.encoding=UTF-8" [junit] NOTE: test params are: codec=Lucene40: {}, sim=RandomSimilarityProvider(queryNorm=true,coord=true): {}, locale=ro, timezone=Europe/Malta [junit] NOTE: all tests run in this JVM: [junit] [TestFunctionQuery] [junit] NOTE: Linux 2.6.33.6-147.fc13.x86_64 amd64/Sun Microsystems Inc. 1.6.0_21 (64-bit)/cpus=24,threads=1,free=171062240,total=239534080 [junit] ------------- ---------------- --------------- [junit] Testcase: testGeneral(org.apache.solr.search.function.TestFunctionQuery): Caused an ERROR [junit] null [junit] java.lang.NullPointerException [junit] at org.apache.solr.search.function.TestFunctionQuery.testGeneral(TestFunctionQuery.java:352) [junit] at org.apache.lucene.util.LuceneTestCase$3$1.evaluate(LuceneTestCase.java:528) [junit] at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:165) [junit] at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:57)
          Hide
          Simon Willnauer added a comment -

          here is a new patch with changes.txt and more tests for cases where norms are not written but also not omitted.

          all tests pass, I think this is ready though!

          Hmm... Norm impls DocValue which I just removed... maybe we should add it back?

          I used DocValuesField as a delegate in this patch. I don't think we need this interface at this point.


          What happens if an app (incorrectly) tries to change up the normType suddenly...? If you change in the middle of an IW session... it's quietly ignored and you get 0/empty bytes indexed instead?

          there is a test for this in TestCustomNorms (testExceptionOnRandomType) it throws a IAE in NormsConsumerPerField.

          (FieldInfo.setNormValueType silently does nothing if it's already
          set). Can we throw an exception if it's already set to a conflicting
          value?

          I added some logic in there to throw an exc if it is not forced and the value != null.

          If you change on a new IW session... they will be merged according to
          the normal type promotion rules of doc values right?

          right!

          Maybe instead of set and reset methods on FieldInfo just have a single
          set method with force boolean?

          done! I also added some sugar to FieldInfo to check if the norms are present.

          This fails for me:

          fixed my fault

          Show
          Simon Willnauer added a comment - here is a new patch with changes.txt and more tests for cases where norms are not written but also not omitted. all tests pass, I think this is ready though! Hmm... Norm impls DocValue which I just removed... maybe we should add it back? I used DocValuesField as a delegate in this patch. I don't think we need this interface at this point. What happens if an app (incorrectly) tries to change up the normType suddenly...? If you change in the middle of an IW session... it's quietly ignored and you get 0/empty bytes indexed instead? there is a test for this in TestCustomNorms (testExceptionOnRandomType) it throws a IAE in NormsConsumerPerField. (FieldInfo.setNormValueType silently does nothing if it's already set). Can we throw an exception if it's already set to a conflicting value? I added some logic in there to throw an exc if it is not forced and the value != null. If you change on a new IW session... they will be merged according to the normal type promotion rules of doc values right? right! Maybe instead of set and reset methods on FieldInfo just have a single set method with force boolean? done! I also added some sugar to FieldInfo to check if the norms are present. This fails for me: fixed my fault
          Simon Willnauer made changes -
          Attachment LUCENE-3687.patch [ 12510691 ]
          Hide
          Simon Willnauer added a comment -

          I plan to commit this soon if nobody objects.

          Show
          Simon Willnauer added a comment - I plan to commit this soon if nobody objects.
          Hide
          Michael McCandless added a comment -

          Patch looks good! Small typo in FieldInfo.setNormValueType "Norn type..." --> "Norm type...".

          I don't like the inconsistency of Field.setValue(T value) but Norm.setT(T value)... we should somehow reconcile how we name the native type setters on these "value holder" classes... but we can sort this later.

          You can remove the unused BytesRef spare in NormsConsumerPerField...

          Show
          Michael McCandless added a comment - Patch looks good! Small typo in FieldInfo.setNormValueType "Norn type..." --> "Norm type...". I don't like the inconsistency of Field.setValue(T value) but Norm.setT(T value)... we should somehow reconcile how we name the native type setters on these "value holder" classes... but we can sort this later. You can remove the unused BytesRef spare in NormsConsumerPerField...
          Hide
          Simon Willnauer added a comment -

          Patch looks good! Small typo in FieldInfo.setNormValueType "Norn type..." --> "Norm type...".

          I will fix that.

          I don't like the inconsistency of Field.setValue(T value) but Norm.setT(T value)... we should somehow reconcile how we name the native type setters on these "value holder" classes... but we can sort this later.

          I will open a followup issue

          You can remove the unused BytesRef spare in NormsConsumerPerField...

          done!

          Show
          Simon Willnauer added a comment - Patch looks good! Small typo in FieldInfo.setNormValueType "Norn type..." --> "Norm type...". I will fix that. I don't like the inconsistency of Field.setValue(T value) but Norm.setT(T value)... we should somehow reconcile how we name the native type setters on these "value holder" classes... but we can sort this later. I will open a followup issue You can remove the unused BytesRef spare in NormsConsumerPerField... done!
          Simon Willnauer made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Assignee Simon Willnauer [ simonw ]
          Resolution Fixed [ 1 ]
          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
          5d 3h 28m 1 Simon Willnauer 16/Jan/12 14:57
          Resolved Resolved Closed Closed
          479d 19h 46m 1 Uwe Schindler 10/May/13 11:44

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development