Bharath Vissapragada has posted comments on this change.

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

Patch Set 16:


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

nit: Could you put a comment on this that its related to consistent logging, 
just for readability.

PS16, Line 83: List<HdfsPartition> sortedPartitions = 
             :           Collections.sort(sortedPartitions);
             :           List<String> sortedPartitionNames = 
             :           int num = 0;
             :           for (HdfsPartition partition : sortedPartitions) {
             :             ++num;
             :             if (num == NUM_PARTITION_LOG_LIMIT) break;
             :           }
nit: you can make this more readable with 
Lists.transform(sortedPartitions.subList(0, NUM_...), transformMethod);
File fe/src/main/java/com/cloudera/impala/catalog/

Line 116: import;
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.
File fe/src/main/java/com/cloudera/impala/planner/

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.
File fe/src/main/java/com/cloudera/impala/service/

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.
File testdata/workloads/functional-query/queries/QueryTest/compute-stats.test:

Line 1: ====
Should we add more tests here with PartitionSet?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c9162fcf9d227b8daf4c2e761d57bab4e26408f
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Amos Bird <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Amos Bird <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-HasComments: Yes

Reply via email to