impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1670: Support multiple partitions in ALTER TABLE ADD PARTITION
Date Mon, 12 Sep 2016 18:52:38 GMT
Bharath Vissapragada has posted comments on this change.

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


Patch Set 7:

(2 comments)

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

PS7, Line 55:   public void setTableName(TableName tableName) {
            :     partitionSpec_.setTableName(tableName);
            :   }
            :   public void setPartitionShouldNotExist() {
            :     partitionSpec_.setPartitionShouldNotExist();
Can't we just make PartitionParams subclass PartitionSpec and avoid duplicate code? It cleans
up the class structure and also the analyze() as well.


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

Line 375:           // already exist in Hive, then throw ImpalaRuntimeException.
> You may want to comment on the consistency guarantees (if any) here. For ex
Skimming through the code, looks like there can be partial updates and inconsistencies possible,
please correct me if I'm wrong. If for some reason an add_partition() on a specific fails,
the query fails but the HMS has already updated a few partitions for all the successful add_partition()
calls till then. Given we don't propagate DDL updates to the coordinator for failed queries,
Impalads and Hive can become inconsistent. +1 to Dimitris' suggestion to use the add_partitions()
call.


-- 
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: 7
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-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message