Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8936 )
Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners ...................................................................... Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/hdfs-rcfile-scanner.cc File be/src/exec/hdfs-rcfile-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/hdfs-rcfile-scanner.cc@119 PS5, Line 119: ss << __FILE__ << ":" << __LINE__<< " Invalid RCFILE_VERSION_HEADER: '" I meant the filename of the file being decoded, i.e. stream_->filename(). http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/scanner-context.inline.h File be/src/exec/scanner-context.inline.h: http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/scanner-context.inline.h@96 PS5, Line 96: if (bytes_left < 0) { Is this possible? The intent of the above code is that it shouldn't go negative if 'length' is non-negative. Callers shouldn't be passing in a negative length, if it's from the caller passing in a negative length, we should add a DCHECK_GE(length, 0) at the start of the function and fix the caller. http://gerrit.cloudera.org:8080/#/c/8936/5/be/src/exec/scanner-context.inline.h@97 PS5, Line 97: SCANNER_INCOMPLETE_READ This will crash since the template for the error message in common/thrift/generate_error_codes.py requires a couple of parameters. It's unfortunate we don't have compile-time checks for this. http://gerrit.cloudera.org:8080/#/c/8936/4/be/src/exec/scanner-context.inline.h File be/src/exec/scanner-context.inline.h: http://gerrit.cloudera.org:8080/#/c/8936/4/be/src/exec/scanner-context.inline.h@97 PS4, Line 97: *status = Status(TErrorCode::SCANNER_INCOMPLETE_READ); The SCANNER_INCOMPLETE_READ template in common/thrift/generate_error_codes.py requires two arguments - it will crash if it doesn't get the expected number. -- To view, visit http://gerrit.cloudera.org:8080/8936 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a Gerrit-Change-Number: 8936 Gerrit-PatchSet: 5 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-Comment-Date: Wed, 31 Jan 2018 01:25:13 +0000 Gerrit-HasComments: Yes
