Lucene - Core
  1. Lucene - Core
  2. LUCENE-3589

BytesRef copy short missed the length setting

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 4.0-ALPHA
    • Component/s: core/other
    • Labels:
    • Environment:

      linux 64bit jdk 6

    • Lucene Fields:
      New, Patch Available

      Description

      when storing a short type integer to BytesRef, BytesRef missed the length setting. then it will cause the storage size is ZERO if no continuous options on this BytesRef

        Activity

        Hide
        Robert Muir added a comment -

        Peter: thank you very much for opening this issue.

        Your patch is correct - however I noticed this method is totally dead code in lucene.
        Its not used by any code or tests.

        I'm going to remove any dead code in BytesRef... this class is spiraling out of control.

        Show
        Robert Muir added a comment - Peter: thank you very much for opening this issue. Your patch is correct - however I noticed this method is totally dead code in lucene. Its not used by any code or tests. I'm going to remove any dead code in BytesRef... this class is spiraling out of control.
        Hide
        Dawid Weiss added a comment -

        I think I'm using it in that fst suggester patch, actually... Not that it cannot be hand-coded if necessary.

        Show
        Dawid Weiss added a comment - I think I'm using it in that fst suggester patch, actually... Not that it cannot be hand-coded if necessary.
        Hide
        peter chang added a comment -

        in fact, i use BytesRef.copy(short) in my code to store an id something like this, and since in lucene 4.0 return BytesRef when get binary value of a field. i think this piece of code can be kept.

        Show
        peter chang added a comment - in fact, i use BytesRef.copy(short) in my code to store an id something like this, and since in lucene 4.0 return BytesRef when get binary value of a field. i think this piece of code can be kept.
        Hide
        Robert Muir added a comment -

        We really cannot just let BytesRef pile up into some mega API.
        When we add things to the public API, we have to eventually support this stuff in future releases.

        If we are hell-bent to keep the api, we must fix the bogosities about it, otherwise its unmaintainable, and should be removed:

        • the fact it is copy(short) is wrong, if i delete the copy(short) and copy(int) methods everything compiles as normal, leading me to believe they are unused. this is because anything using thse gets promoted to copy(long). So the methods must be renamed to copyShort, copyInt, etc to prevent these types of mistakes.
        • why do we reset the offset to 0?

        In general i'm just wondering why this is needed in bytesref itself... do people know about ByteArrayDataInput and ByteArrayDataOutput?

        Separately, I'm still going to remove all unused bytesref methods at the current moment... i'll leave this one be though.

        Show
        Robert Muir added a comment - We really cannot just let BytesRef pile up into some mega API. When we add things to the public API, we have to eventually support this stuff in future releases. If we are hell-bent to keep the api, we must fix the bogosities about it, otherwise its unmaintainable, and should be removed: the fact it is copy(short) is wrong, if i delete the copy(short) and copy(int) methods everything compiles as normal, leading me to believe they are unused. this is because anything using thse gets promoted to copy(long). So the methods must be renamed to copyShort, copyInt, etc to prevent these types of mistakes. why do we reset the offset to 0? In general i'm just wondering why this is needed in bytesref itself... do people know about ByteArrayDataInput and ByteArrayDataOutput? Separately, I'm still going to remove all unused bytesref methods at the current moment... i'll leave this one be though.
        Hide
        Dawid Weiss added a comment -

        I agree with you in that to know what these methods actually do one has to look into the source code – this is what I actually did. And I agree this isn't good from the API perspective.

        As for ByteArrayDataInput/Output – these are fine, we all know they're; I used BytesRef#copy(short) to avoid creating an extra object.

        Show
        Dawid Weiss added a comment - I agree with you in that to know what these methods actually do one has to look into the source code – this is what I actually did. And I agree this isn't good from the API perspective. As for ByteArrayDataInput/Output – these are fine, we all know they're; I used BytesRef#copy(short) to avoid creating an extra object.
        Hide
        Robert Muir added a comment -

        As for ByteArrayDataInput/Output – these are fine, we all know they're; I used BytesRef#copy(short) to avoid creating an extra object.

        If you are worried about the object overhead of ByteArrayDataInput/Output in some loop or something, just reuse the same one with reset()....

        Show
        Robert Muir added a comment - As for ByteArrayDataInput/Output – these are fine, we all know they're; I used BytesRef#copy(short) to avoid creating an extra object. If you are worried about the object overhead of ByteArrayDataInput/Output in some loop or something, just reuse the same one with reset()....
        Hide
        Dawid Weiss added a comment -

        I'd probably just push the two bytes myself to the byte array, to be honest

        Show
        Dawid Weiss added a comment - I'd probably just push the two bytes myself to the byte array, to be honest
        Hide
        Robert Muir added a comment -

        Thanks Peter: I committed your fix

        Show
        Robert Muir added a comment - Thanks Peter: I committed your fix

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development