Lars Volker has posted comments on this change. Change subject: IMPALA-1740: Add support for skip.header.line.count. ......................................................................
Patch Set 18: (8 comments) Thanks for the review, please see PS19. 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 No, not in this class. The null check came from CreateTableStmt. I moved it there now. Line 129: if (tblProperties != null && tblProperties.containsKey( > does it make sense to call this function w/ tblProperties == null? CreateTableStmt would do that. I moved the check there. Line 130: HdfsTable.TBL_PROP_SKIP_HEADER_LINE_COUNT)) { > awkward formatting; change it so the containsKey call isn't broken up or br Done Line 131: if (table != null && !(table instanceof HdfsTable)) { > does it make sense to call this function w/ table == null? I needed a way to verify that table is an HdfsTable, but for CreateTableStmt we don't have a table when calling this function. I replaced it with a boolean flag, but I'm still not entirely satisfied with the solution. Do you have a better idea? 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" Done Line 1236: if (skipHeaderLineCount < 0) { > single line Done 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 Done 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 p No, currently it strictly requires textfile table format. I removed the restriction, the rest of the code did not need to be changed. -- 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
