Dan Hecht has posted comments on this change. Change subject: IMPALA-3764,3914: fuzz test HDFS scanners and fix parquet bugs found ......................................................................
Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/3448/9/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 649: FILE_CHECK_GE(col_reader->def_level(), Doesn't have to be done now, but FILE_CHECK places would be nice if they can be exercised using the fuzz tester and then fixed http://gerrit.cloudera.org:8080/#/c/3448/9/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: Line 497: DCHECK(false) << ss.str(); this seems weird. makes it unclear if it's an invariant or not. I think we should just delete this line. http://gerrit.cloudera.org:8080/#/c/3448/9/be/src/runtime/scoped-buffer.h File be/src/runtime/scoped-buffer.h: Line 13: // limitations under the License. mind putting in the ASF license text instead? http://gerrit.cloudera.org:8080/#/c/3448/9/be/src/util/dict-encoding.h File be/src/util/dict-encoding.h: Line 171: if (UNLIKELY(buffer_len == 0)) return Status("Dictionary cannot be 0 bytes"); who can call this with buffer_len == 0? why isn't != 0 an invariant while < 0 is? http://gerrit.cloudera.org:8080/#/c/3448/9/tests/query_test/test_scanners_fuzz.py File tests/query_test/test_scanners_fuzz.py: Line 1: # Copyright (c) 2016 Cloudera, Inc. All rights reserved. please put ASF license here rather than copyright. -- To view, visit http://gerrit.cloudera.org:8080/3448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I50cf43195a7c582caa02c85ae400ea2256fa3a3b Gerrit-PatchSet: 9 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
