Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201602260210.u1Q2Ahqt008691@ip-10-146-233-104.ec2.internal>
Date: Fri, 26 Feb 2016 02:10:43 +0000
From: "Dan Hecht (Code Review)" <gerrit@cloudera.org>
To: Juan Yu <jyu@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Skye Wanderman-Milne <skye@cloudera.com>
Reply-To: dhecht@cloudera.com
Subject: 
 =?UTF-8?Q?[Impala-CR](cdh5-2.5.0=5F5.7.0)_IMPALA-1886/IMPALA-2154:_Add_support_for_multi-stream_bz2/gzip_compressed_files.=0A?=
In-Reply-To: 
 <gerrit.1455761144000.Icbe617d03a69953f0bf3aa0f7c30d34bc612f9f8@gerrit.cloudera.org>
References: 
 <gerrit.1455761144000.Icbe617d03a69953f0bf3aa0f7c30d34bc612f9f8@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.10-rc0

Dan Hecht has posted comments on this change.

Change subject: IMPALA-1886/IMPALA-2154: Add support for multi-stream bz2/gzip compressed files.
......................................................................


Patch Set 14:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/2219/14/be/src/exec/hdfs-text-scanner.cc
File be/src/exec/hdfs-text-scanner.cc:

Line 484:   }
L469-484 should be restructured as:

if (stream_->eosr()) {
   if (stream_end) {
      *eosr = true;
   } else {
       return Status(truncated);
   }
} else if (*decompressed_len == 0) {
   return Status(NO_PRORESS);
}


http://gerrit.cloudera.org:8080/#/c/2219/14/be/src/util/codec.h
File be/src/util/codec.h:

Line 130:   ///   stream_end: if decompressor consumed all input and reached end of compressed stream.
I think we should remove the "if decompressor consumed all input" condition, and this should just signify that the end of the 'output' buffer corresponds to the end of a compression stream (and then see my comments in the code).  The caller should (and already does check) that there is no more input via the stream_->eosr() check.


http://gerrit.cloudera.org:8080/#/c/2219/14/be/src/util/decompress.cc
File be/src/util/decompress.cc:

Line 89:   *stream_end = false;
move this to L91 inside the loop.  See comment at line 113 for why.


Line 113:       if (stream_.avail_in == 0) *stream_end = true;
As mentioned elsewhere, let's simplify this and just uncondtionally set *stream_end = true here.  The avail_in check isn't interesting, since only the caller knows whether there really is more input or not.


Line 342:   *stream_end = false;
same comments as above


Line 352:       if (stream_.avail_in == 0) *stream_end = true;
and here


http://gerrit.cloudera.org:8080/#/c/2219/14/common/thrift/generate_error_codes.py
File common/thrift/generate_error_codes.py:

Line 226: decompressed
compressed


http://gerrit.cloudera.org:8080/#/c/2219/14/testdata/workloads/functional-query/queries/DataErrorsTest/hdfs-scan-node-errors.test
File testdata/workloads/functional-query/queries/DataErrorsTest/hdfs-scan-node-errors.test:

Line 194: decompressed
this should say "compressed" file.  From the users perspective, it's the compressed file that was malformed.


-- 
To view, visit http://gerrit.cloudera.org:8080/2219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icbe617d03a69953f0bf3aa0f7c30d34bc612f9f8
Gerrit-PatchSet: 14
Gerrit-Project: Impala
Gerrit-Branch: cdh5-2.5.0_5.7.0
Gerrit-Owner: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-Reviewer: Skye Wanderman-Milne <skye@cloudera.com>
Gerrit-HasComments: Yes