Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 3.1
    • Fix Version/s: None
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Make BytesRef comparable, document that byte[] should not be null, remove explicit null check + allocation.

      1. LUCENE-2438.patch
        8 kB
        Uwe Schindler
      2. LUCENE-2438.patch
        4 kB
        Yonik Seeley
      3. LUCENE-2438.patch
        3 kB
        Yonik Seeley

        Activity

        Hide
        Yonik Seeley added a comment -

        Here's an updated version. Uwe is now also working on this, so I won't make further changes.

        Show
        Yonik Seeley added a comment - Here's an updated version. Uwe is now also working on this, so I won't make further changes.
        Hide
        Uwe Schindler added a comment -

        Here my version with other CTA and NTS changes.

        Show
        Uwe Schindler added a comment - Here my version with other CTA and NTS changes.
        Hide
        Yonik Seeley added a comment -

        Looks good.
        This is trunk, and currently lucene.internal, so I'll commit shortly.

        Show
        Yonik Seeley added a comment - Looks good. This is trunk, and currently lucene.internal, so I'll commit shortly.
        Hide
        Yonik Seeley added a comment - - edited

        Looks like BytesRef.equals() is incorrect... I'll fix.

        edit: no it's not... I keep seeing length as bytes.length

        Show
        Yonik Seeley added a comment - - edited Looks like BytesRef.equals() is incorrect... I'll fix. edit: no it's not... I keep seeing length as bytes.length
        Hide
        Uwe Schindler added a comment -

        We should keep the issue open after the commit, as there is more to do. I only changed the code parts that were done by me, maybe Mike has other ones with null checks.

        Show
        Uwe Schindler added a comment - We should keep the issue open after the commit, as there is more to do. I only changed the code parts that were done by me, maybe Mike has other ones with null checks.
        Hide
        Yonik Seeley added a comment -

        Another issue is that it looks like some code assumes that offset==0 (and this is reinforced by the naming of BytesRef.length, which we should consider changing to BytesRef.len)

        Most of these seem sort-of OK... it's after a UTF16TOUTF8, which always makes the offset zero. But if we depend on this, it should be documented.

        For example:
        org.apache.lucene.document.CompressionTools#compressString
        org.apache.lucene.index.codecs.pulsing.PulsingPostingsWriterImpl#finishTerm
        org.apache.lucene.store.DataOutput#writeString

        Show
        Yonik Seeley added a comment - Another issue is that it looks like some code assumes that offset==0 (and this is reinforced by the naming of BytesRef.length, which we should consider changing to BytesRef.len) Most of these seem sort-of OK... it's after a UTF16TOUTF8, which always makes the offset zero. But if we depend on this, it should be documented. For example: org.apache.lucene.document.CompressionTools#compressString org.apache.lucene.index.codecs.pulsing.PulsingPostingsWriterImpl#finishTerm org.apache.lucene.store.DataOutput#writeString
        Hide
        Yonik Seeley added a comment -

        I've added javadoc that defines result.offset to be 0 after the UTF16toUTF8 methods.
        I also clarified that the byte[] one passes in is directly referenced instead of copied... which implies that BytesRef(byte[]) will cause offset to be 0.

        Show
        Yonik Seeley added a comment - I've added javadoc that defines result.offset to be 0 after the UTF16toUTF8 methods. I also clarified that the byte[] one passes in is directly referenced instead of copied... which implies that BytesRef(byte[]) will cause offset to be 0.
        Hide
        Uwe Schindler added a comment -

        I fixed NumericUtils to do the same in rev 940545.

        Show
        Uwe Schindler added a comment - I fixed NumericUtils to do the same in rev 940545.

          People

          • Assignee:
            Unassigned
            Reporter:
            Yonik Seeley
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development