Commons Compress
  1. Commons Compress
  2. COMPRESS-69

Inconsistent handling of BZ2-Header after redesign

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0
    • Component/s: None
    • Labels:
      None

      Description

      I tried the current trunk of compress and tried the BZIP2-Streams. There are the following inconsitenceis now:
      a) BZIP InputStream checks that the "BZ" header is available in the File (using this magicChar method)
      b) BZIP OutputStream does not add the "BZ" header (it is commented out, saying that the caller does this)
      c) The Javadocs of the input stream say, you should skip the header, which is wrong.
      d) the tests do not find these errors, as the test only compressed a file, but does not try to decompress it or check its contents

      Before the redesign commit, the handling of headers was consistent (both streams used it). The BZ Headers should be added to the Streams, as this would be consistent with GZIP (where the headers are automatically added, too).

      1. COMPRESS-69.patch
        1 kB
        Uwe Schindler

        Activity

        Uwe Schindler created issue -
        grobmeier committed 764423 (1 file)
        Reviews: none

        improved testcase for COMPRESS-69: currently commented out since it doesn't work as expected

        Hide
        Christian Grobmeier added a comment -

        I improved the testcase where the described behaviour can be seen. I also think BZ should be added at the OutputStream.
        Thanks for reporting!

        Show
        Christian Grobmeier added a comment - I improved the testcase where the described behaviour can be seen. I also think BZ should be added at the OutputStream. Thanks for reporting!
        Christian Grobmeier made changes -
        Field Original Value New Value
        Fix Version/s 1.0 [ 12313768 ]
        Affects Version/s 1.0 [ 12313768 ]
        Hide
        Christian Grobmeier added a comment -

        Fix is like following:

        comment in:
        Line713: this.out.write('B');
        Line724: this.out.write('Z');

        and update documentation. Just waiting for responses on the mailinglist, why this change has happened.

        Show
        Christian Grobmeier added a comment - Fix is like following: comment in: Line713: this.out.write('B'); Line724: this.out.write('Z'); and update documentation. Just waiting for responses on the mailinglist, why this change has happened.
        Hide
        Uwe Schindler added a comment -

        I would use the new method bsPutUByte(), see attached patch. The documentation is not updated. The test passes.

        Show
        Uwe Schindler added a comment - I would use the new method bsPutUByte(), see attached patch. The documentation is not updated. The test passes.
        Uwe Schindler made changes -
        Attachment COMPRESS-69.patch [ 12405324 ]
        grobmeier committed 764502 (1 file)
        Reviews: none

        COMPRESS-69: Docs state BZ should be skipped by caller, but the Stream checks for it. Removed wrong docs.

        grobmeier committed 764503 (1 file)
        Reviews: none

        COMPRESS-69: Applied patch from Uwe Schindler. Writing magic BZ is now done by the stream again.
        COMPRESS-69: Updated docs

        Hide
        Christian Grobmeier added a comment -

        Patch applied, thanks Uwe. Docs also updated.
        New revision: r764503

        Show
        Christian Grobmeier added a comment - Patch applied, thanks Uwe. Docs also updated. New revision: r764503
        Christian Grobmeier made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Uwe Schindler added a comment -

        you forgot to enable the test again...

        Show
        Uwe Schindler added a comment - you forgot to enable the test again...
        Hide
        Uwe Schindler added a comment -

        also the top level class comments contain still "without headers". We are discussing this in LUCENE-1591 where we want to use commons-compress for the handling of the very large wikipedia bz2 dumps.

        Show
        Uwe Schindler added a comment - also the top level class comments contain still "without headers". We are discussing this in LUCENE-1591 where we want to use commons-compress for the handling of the very large wikipedia bz2 dumps.
        Hide
        Uwe Schindler added a comment -

        I reopen the issue to be sure, that the last two comments are seen.

        Show
        Uwe Schindler added a comment - I reopen the issue to be sure, that the last two comments are seen.
        Uwe Schindler made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        grobmeier committed 764536 (1 file)
        grobmeier committed 764537 (1 file)
        grobmeier committed 764538 (1 file)
        Hide
        Christian Grobmeier added a comment -

        Thanks again Uwe.
        And thanks for bringing commons-compress into discussion. It's great to see that people start trying and using the lib.

        Show
        Christian Grobmeier added a comment - Thanks again Uwe. And thanks for bringing commons-compress into discussion. It's great to see that people start trying and using the lib.
        Christian Grobmeier made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Uwe Schindler added a comment -

        Thanks, too. Apache TIKA is also waiting for version 1.0 to be released (TIKA-204)! For Lucene, the snapshot is OK at the moment, as it is only for a module that is measuring indexing performance with very big source files that should decompressed on the fly.

        Show
        Uwe Schindler added a comment - Thanks, too. Apache TIKA is also waiting for version 1.0 to be released ( TIKA-204 )! For Lucene, the snapshot is OK at the moment, as it is only for a module that is measuring indexing performance with very big source files that should decompressed on the fly.

          People

          • Assignee:
            Unassigned
            Reporter:
            Uwe Schindler
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development