impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Attila Jeges (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1670: Support multiple partitions in ALTER TABLE ADD PARTITION
Date Sat, 03 Sep 2016 02:16:32 GMT
Attila Jeges has posted comments on this change.

Change subject: IMPALA-1670: Support multiple partitions in ALTER TABLE ADD PARTITION
......................................................................


Patch Set 5:

(21 comments)

Thanks for commenting o this change.

IMPALA-1654 addresses a different improvement, merging the two changes together shuld not
be an issue.

http://gerrit.cloudera.org:8080/#/c/4144/4/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:

PS4, Line 227: If true, no error is raised if a partition with the same spec already exists.
> What happens when multiple partitions are specified, if_not_exists is true 
Done


PS4, Line 231: add_partitions
> nit: maybe call it simply partitions or partitions_to_add?
Done


http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

PS4, Line 377: add_partition;
             : nonterminal ArrayList<AddPartition> add_partition_list;
> 'AddPartition', 'add_partition' and 'add_partition_list' sound a lot like f
Done


PS4, Line 879: ArrayList<AddPartition> list = new ArrayList<AddPartition>();
             :     list.add(item);
> I believe you can replace these two lines with one:
Done


http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/main/java/com/cloudera/impala/analysis/AddPartition.java
File fe/src/main/java/com/cloudera/impala/analysis/AddPartition.java:

PS4, Line 41: private Table table_;
            :   private HdfsTable hdfsTable_;
> Why do you need both?
Removed hdfsTable_


PS4, Line 105: a
> "an"
Done


http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/main/java/com/cloudera/impala/analysis/AlterTableAddPartitionStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/AlterTableAddPartitionStmt.java:

PS4, Line 40: ImmutableList
> Why an ImmutableList? Do you expose this outside the class?
I guess it was exposed in an earlier draft, but it is not needed anymore. Removing ImmutableList.


PS4, Line 43: ifNotExists
> As I mentioned in an earlier comment, plz describe the semantics of using t
Done


PS4, Line 46: partitions.size() > 0
> !partitions.isEmpty()
Done


PS4, Line 47: mmutableList.copyOf(partitions);
> I don't believe you need a copy.
Done


PS4, Line 49:  
> formatting nit: no space between variable name and ':'. Also, don't this bl
Done


PS4, Line 60: if (ifNotExists_) {
            :       sb.append(" IF NOT EXISTS");
            :     }
> single line, no braces
Done


PS4, Line 63: for (AddPartition p : partitions_) {
            :       sb.append(" " + p.toSql());
            :     }
> here as well (single line, no braces) and everywhere this applies :)
Done


PS4, Line 97: .toLowerCase
> Are you sure the partition key values are not already in lower case?
I've looked it up, they are converted to lower case in PartitionKeyValue class. Removing toLowerCase()
call from here.


http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java
File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java:

PS4, Line 358: Table refreshedTable = null;
> Why is this here? Can you move it back to L375?
refreshedTable variable is also used in L400 in the DROP_PARTITION case block.  Since refreshedTable
is used in different case blocks,  I thought it would be cleaner to define it in front of
the switch statement.

I can move it back to L375 of course, if you think that it does not hurt readability.


PS4, Line 373: If the partitions already exist in Hive and "IfNotExists" is true
> As I commented earlier, let's try to clarify a bit the behavior in this cas
Done


PS4, Line 1641: Table result = null;
> I don't think you need this. You can remove it and have L1707 simply do "re
Done


http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java:

Line 204: 
> Can you also add the following cases:
Done


http://gerrit.cloudera.org:8080/#/c/4144/4/testdata/workloads/functional-query/queries/QueryTest/alter-table.test
File testdata/workloads/functional-query/queries/QueryTest/alter-table.test:

PS4, Line 1004: pzrtitions
> typo
Fixed


PS4, Line 1015: test-warehouse/i1670_alter
> Why using the entire path here?
Done


PS4, Line 1071: ---- QUERY
              : alter table i1670_alter add if not exists
              : partition (i=0) location '/i0C'
              : partition (i=1) location '/i1C'
              : partition (i=2) location '/i2C';
              : show partitions i1670_alter;
              : ---- RESULTS
              : '0',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i0C
              : '1',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i1A
              : '2',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i2B
              : '3',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i3A
              : '4',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i4B
              : 'Total',-1,0,'0B','0B','','','',''
              : ---- TYPES
              : STRING, BIGINT, BIGINT, STRING, STRING, STRING, STRING, STRING, STRING
              : ====
              : ---- QUERY
              : alter table i1670_alter add if not exists
              : partition (i=1) location '/i1D'
              : partition (i=2) location '/i2D'
              : partition (i=3) location '/i3D';
              : show partitions i1670_alter;
              : ---- RESULTS
              : '0',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i0C
              : '1',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i1A
              : '2',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i2B
              : '3',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i3A
              : '4',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i4B
              : 'Total',-1,0,'0B','0B','','','',''
              : ---- TYPES
              : STRING, BIGINT, BIGINT, STRING, STRING, STRING, STRING, STRING, STRING
> I don't think these cases add anything in terms of testing coverage compare
Ok, removing it.


-- 
To view, visit http://gerrit.cloudera.org:8080/4144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iddbc951f2931f488f7048c9780260f6b49100750
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message