Skye Wanderman-Milne has posted comments on this change. Change subject: IMPALA-3441, IMPALA-3659: check for malformed Avro data ......................................................................
Patch Set 11: (12 comments) http://gerrit.cloudera.org:8080/#/c/3072/11/be/src/exec/hdfs-avro-scanner-test.cc File be/src/exec/hdfs-avro-scanner-test.cc: Line 304: // TestReadAvroString(data, 10, sv, 1); > i assume you'll reenable this? oops yes http://gerrit.cloudera.org:8080/#/c/3072/11/be/src/exec/hdfs-avro-scanner.cc File be/src/exec/hdfs-avro-scanner.cc: PS11, Line 189: num_entries > this should be validated (like line 123). Done Line 483: DCHECK_GE(num_records, 0); > should return status Done Line 485: DCHECK_GE(compressed_size, 0); > same Done http://gerrit.cloudera.org:8080/#/c/3072/11/be/src/exec/hdfs-avro-scanner.h File be/src/exec/hdfs-avro-scanner.h: PS11, Line 283: Report > Consider renaming these: SetStatusCorruptData(), SetStatusInvalidValue() to Done http://gerrit.cloudera.org:8080/#/c/3072/11/be/src/exec/read-write-util.cc File be/src/exec/read-write-util.cc: PS11, Line 25: FindZLongLength > how about calling it FindZIntegerLength() since it's really a utility for R Done PS11, Line 36: ReadZLong > ReadZInteger() Done PS11, Line 38: ReadZLongSlow > ReadZIntegerSlow Done Line 61: ZResult ReadWriteUtil::ReadZInteger(uint8_t** buf, uint8_t* buf_end) { > maybe add: Done Line 87: } > does ReadZInteger get inlined? might be safer to put these in the header so They have to go here due to the template specialization, see http://stackoverflow.com/questions/21112148/specialization-of-member-function-template-after-instantiation-error-and-order/21112309#21112309. I'm hoping that the templated function definition gets inlined into these stubs. http://gerrit.cloudera.org:8080/#/c/3072/11/be/src/exec/read-write-util.h File be/src/exec/read-write-util.h: Line 109: /// is incremented past the encoded long. > should document buf_end Done Line 247: // } > remove commented out code Done -- To view, visit http://gerrit.cloudera.org:8080/3072 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I801a11c496a128e02c564c2a9c44baa5a97be132 Gerrit-PatchSet: 11 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
