impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1740: Add support for skip.header.line.count.
Date Tue, 12 Apr 2016 02:39:21 GMT
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 <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