impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Amos Bird (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1654: General partition exprs in DDL operations.
Date Wed, 21 Sep 2016 02:00:53 GMT
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

Mime
View raw message