Bharath Vissapragada has posted comments on this change.

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


Patch Set 16:

(9 comments)

Great work. I just have a bunch of nits and some clarifications. Thanks for 
contributing this patch.

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, 
just for readability.


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.subList(0, NUM_...), transformMethod);


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 
grouped by packages.


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


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

PS16, Line 88: Used by some ALTER TABLE statements which need partition pruning.
IMO this statement is not really required. Alex's call is final though.


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 490: if (reloadMetadata) {
              :         loadTableMetadata(tbl, newCatalogVersion, 
reloadFileMetadata, reloadTableSchema,
              :             null);
              :         addTableToCatalogUpdate(tbl, response.result);
              :       }
Alex, is this a potential behavioral change (For ex: ADD_PARTITION) ? Without 
this patch,we seem to be doing for all the ops. May be I'm missing some 
background on this.


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


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


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?


-- 
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 <amosb...@gmail.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Amos Bird <amosb...@gmail.com>
Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to