impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1740: Add support for skip.header.line.count.
Date Tue, 12 Apr 2016 15:09:10 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Skye Wanderman-Milne <skye@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message