Marcel Kornacker has posted comments on this change. Change subject: IMPALA-1740: Add support for skip.header.line.count. ......................................................................
Patch Set 16: (6 comments) http://gerrit.cloudera.org:8080/#/c/2110/16/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: Line 635: // When skipping multiple header lines we only try to skip them in the first scan precede w/ blank line http://gerrit.cloudera.org:8080/#/c/2110/16/be/src/runtime/descriptors.h File be/src/runtime/descriptors.h: Line 306: PartitionIdToDescriptorMap partition_descriptors_; why did you move this? http://gerrit.cloudera.org:8080/#/c/2110/16/common/thrift/Descriptors.thrift File common/thrift/Descriptors.thrift: Line 75: /// applies to hdfs text files. this should only be in THdfsTable http://gerrit.cloudera.org:8080/#/c/2110/16/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: Line 1842: skipHeaderLineCount = Integer.parseInt(msTable_.getParameters().get(key)); parseInt returns a signed value; check for < 0 Line 1844: throw new NumberFormatException(String.format( what happens when you hit that? this is an uncaught exception. also: test case missing. http://gerrit.cloudera.org:8080/#/c/2110/16/fe/src/main/java/com/cloudera/impala/catalog/Table.java File fe/src/main/java/com/cloudera/impala/catalog/Table.java: Line 479: public int getSkipHeaderLineCount() { return 0; } this doesn't make sense for anything other than an hdfs table. move it there. -- To view, visit http://gerrit.cloudera.org:8080/2110 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I595f01a165d41499ca1956fe748ba3840a6eb543 Gerrit-PatchSet: 16 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-HasComments: Yes
