Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201609210200.u8L20rpT009533@ip-10-146-233-104.ec2.internal>
Date: Wed, 21 Sep 2016 02:00:53 +0000
From: "Amos Bird (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Jim Apple <jbapple@cloudera.com>,
        Bharath Vissapragada <bharathv@cloudera.com>,
        Alex Behm <alex.behm@cloudera.com>
Reply-To: amosbird@gmail.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-1654=3A_General_partition_exprs_in_DDL_operations=2E=0A?=
In-Reply-To: <gerrit.1470906636000.I2c9162fcf9d227b8daf4c2e761d57bab4e26408f@gerrit.cloudera.org>
References: <gerrit.1470906636000.I2c9162fcf9d227b8daf4c2e761d57bab4e26408f@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Wed, 21 Sep 2016 02:00:59 -0000

Amos Bird has posted comments on this change.

Change subject: IMPALA-1654: General partition exprs in DDL operations.
......................................................................


Patch Set 16:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/3942/16/fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java:

PS16, Line 42: NUM_PARTITION_LOG_LIMIT
> nit: Could you put a comment on this that its related to consistent logging
Done


PS16, Line 83: List<HdfsPartition> sortedPartitions = Lists.newArrayList(partitions);
             :           Collections.sort(sortedPartitions);
             :           List<String> sortedPartitionNames = Lists.newArrayList();
             :           int num = 0;
             :           for (HdfsPartition partition : sortedPartitions) {
             :             sortedPartitionNames.add(partition.getPartitionName());
             :             ++num;
             :             if (num == NUM_PARTITION_LOG_LIMIT) break;
             :           }
> nit: you can make this more readable with Lists.transform(sortedPartitions.
Done


http://gerrit.cloudera.org:8080/#/c/3942/16/fe/src/main/java/com/cloudera/impala/analysis/DropStatsStmt.java
File fe/src/main/java/com/cloudera/impala/analysis/DropStatsStmt.java:

Line 35:   PartitionSet partitionSet_ = null;
> private final
Done


Line 90:       TableRef tableRef =
> move this up and replace the analyzer.getTargetDbName() and analyzer.getTab
Done


http://gerrit.cloudera.org:8080/#/c/3942/16/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java
File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java:

Line 116: import java.io.IOException;
> Please fix duplicate imports and the order of imports as well. Should be gr
Done


Line 1021:       if (hdfsPartition != null) { droppedPartitions.add(hdfsPartition); }
> Redundant braces for a single line "if" stmt.
Done


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

PS16, Line 1751: numUpdatedPartitions
> please add a description for this.
Done


PS16, Line 1782:  try(
> nit: double spacing.
Done


http://gerrit.cloudera.org:8080/#/c/3942/16/testdata/workloads/functional-query/queries/QueryTest/compute-stats.test
File testdata/workloads/functional-query/queries/QueryTest/compute-stats.test:

Line 1: ====
> Should we add more tests here with PartitionSet?
I cannot get e2e tests running correctly in my local env. It's quite hard for me to blind code some tests here :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c9162fcf9d227b8daf4c2e761d57bab4e26408f
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Amos Bird <amosbird@gmail.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Amos Bird <amosbird@gmail.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-HasComments: Yes