Marcel Kornacker has posted comments on this change. Change subject: IMPALA-1740: Add support for skip.header.line.count. ......................................................................
Patch Set 18: (8 comments) http://gerrit.cloudera.org:8080/#/c/2110/18/fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetTblProperties.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetTblProperties.java: Line 43: private final HashMap<String, String> tblProperties_; can this be null? if so, it might be more convenient to always have to set (so you only have to check for empty, not for null on top of that) Line 129: if (tblProperties != null && tblProperties.containsKey( does it make sense to call this function w/ tblProperties == null? Line 130: HdfsTable.TBL_PROP_SKIP_HEADER_LINE_COUNT)) { awkward formatting; change it so the containsKey call isn't broken up or broken up in a way that it's clear that the two consecutive lines belong together Line 131: if (table != null && !(table instanceof HdfsTable)) { does it make sense to call this function w/ table == null? http://gerrit.cloudera.org:8080/#/c/2110/18/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: Line 1230: "must be >= 0)", key, string_value); let's be even clearer: "must be integer >= 0" Line 1236: if (skipHeaderLineCount < 0) { single line http://gerrit.cloudera.org:8080/#/c/2110/18/testdata/workloads/functional-query/queries/QueryTest/alter-table.test File testdata/workloads/functional-query/queries/QueryTest/alter-table.test: Line 855: Could not parse table property skip.header.line.count: this doesn't look right, given that you unified the error message http://gerrit.cloudera.org:8080/#/c/2110/18/testdata/workloads/functional-query/queries/QueryTest/hdfs-text-scan-with-header.test File testdata/workloads/functional-query/queries/QueryTest/hdfs-text-scan-with-header.test: Line 118: alter table mixed set tblproperties("skip.header.line.count"="2"); what if you did this in reverse: create parquet table, then add text file partitions. can you still set the linecount property? i think we need to allow that, otherwise it's inconsistent. in the end, the table format doesn't really matter, since you can change the format on a partition basis. sorry to go back on this, but i think we need to relax this so you can specify the table property regardless of the format, and then ignore the property for non-text partitions (but return an error for text partitions if the property value is bad). -- 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: 18 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
