impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-1683: Allow REFRESH on a single partition
Date Thu, 07 Jul 2016 22:30:34 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-1683: Allow REFRESH on a single partition

Patch Set 5:


Did you forget to add the analysis tests? (see my comment on the previous patch)
File fe/src/main/java/com/cloudera/impala/analysis/

PS5, Line 42: Preconditions.checkArgument(!isRefresh || name != null);
            :     Preconditions.checkArgument(partitionSpec == null || name != null);
Can't we combine these two into a single check?
File fe/src/main/java/com/cloudera/impala/catalog/

PS5, Line 1164: To achieve that, it drops the partition
              :    * from the table object, fetches a fresh copy of the partition from Hive
              :    * metastore and adds it back to the table.
We no longer do that in this function, so you may want to update the comment.

PS5, Line 1174: Db db = tbl.getDb();
This seems to be used first time in L1195. Let's move it closer to that.

PS5, Line 1175: HdfsPartition refreshedPartition = null;
Where is this variable used?

PS5, Line 1182: // For the case where partition does not exist in Catalog cache but might
              :       // exist in Hive Metastore
I am not sure I follow the comment. I believe it's more accurate to say that you either retrieve
the partition name from an existing partition or you construct it from the partition spec.

PS5, Line 1185: getPartitionNameFromThriftPartitionSpec(
I think it's a bit more clear to call this function constructPartitionName(). The partition
spec is the input param so it's kind of obvious how the partition name gets constructed.

PS5, Line 1187: %s",
              :           tbl.getFullName() + " " + partitionName));
"%s %s", tbl.getFullName(), partitionName);

PS5, Line 1189: long newCatalogVersion = incrementAndGetCatalogVersion();
              :       catalogLock_.writeLock().unlock();
Move this above L1180. We don't want to hold the catalog lock longer than necessary.

Line 1193:         org.apache.hadoop.hive.metastore.api.Partition partition = null;
I would add a comment here saying: "If the partition does not exist in Hive MetaStore, remove
it from the catalog."

PS5, Line 1206:     + tblName.getTable_name() + " " + partitionName,
              :               e);
nit: merge these two lines
File fe/src/main/java/com/cloudera/impala/catalog/

PS5, Line 1944: data
metadata (files and blocks)

PS5, Line 1948: dropPartition(partitionSpec);
We have a dropPartition function that takes as input a Partition object, so I don't think
the partitionSpec parameter is needed here.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I4bf5dd630f2e175be5800187bbf9337c91f82bd8
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Bikramjeet Vig <>
Gerrit-Reviewer: Bikramjeet Vig <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-HasComments: Yes

View raw message