Tim Armstrong has posted comments on this change. Change subject: IMPALA-3441: check for malformed Avro data ......................................................................
Patch Set 6: (6 comments) Code is looking good, just need to figure out perf before +1 I think http://gerrit.cloudera.org:8080/#/c/3072/5/be/src/exec/hdfs-avro-scanner-ir.cc File be/src/exec/hdfs-avro-scanner-ir.cc: PS5, Line 46: > It doesn't seem to have an effect, probably because everything's inlined wi Ahh, yeah I didn't think about that - inlining may actually discard the __restrict__ qualifiers. Or maybe it just didn't have a measurable effect. http://gerrit.cloudera.org:8080/#/c/3072/6/be/src/exec/hdfs-avro-scanner-ir.cc File be/src/exec/hdfs-avro-scanner-ir.cc: Line 176: if (*field_len < 0) { UNLIKELY Line 180: if (*data_len < *field_len) { UNLIKELY http://gerrit.cloudera.org:8080/#/c/3072/5/be/src/exec/hdfs-avro-scanner-test.cc File be/src/exec/hdfs-avro-scanner-test.cc: Line 190: data[0] = 0; > It DCHECKs right now, so it would take some changes to even run this case. There's an ASSERT_DEBUG_DEATH macro that does some magic to assert that it hits a DCHECK. Might be useful to add the tests wrapped in that. Feel free to ignore. http://gerrit.cloudera.org:8080/#/c/3072/6/be/src/exec/hdfs-avro-scanner-test.cc File be/src/exec/hdfs-avro-scanner-test.cc: Line 96: // Test type promotion to long, float, and double Nice http://gerrit.cloudera.org:8080/#/c/3072/6/testdata/workloads/functional-query/queries/DataErrorsTest/avro-errors.test File testdata/workloads/functional-query/queries/DataErrorsTest/avro-errors.test: Line 7: # TODO(skye) Are you intending to fix this in this patch? -- 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: 6 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
