Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-1683: Allow REFRESH on a single partition ......................................................................
Patch Set 5: (12 comments) Did you forget to add the analysis tests? (see my comment on the previous patch) http://gerrit.cloudera.org:8080/#/c/3385/5/fe/src/main/java/com/cloudera/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/com/cloudera/impala/analysis/ResetMetadataStmt.java: PS5, Line 42: Preconditions.checkArgument(!isRefresh || name != null); : Preconditions.checkArgument(partitionSpec == null || name != null); Can't we combine these two into a single check? http://gerrit.cloudera.org:8080/#/c/3385/5/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java: 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 http://gerrit.cloudera.org:8080/#/c/3385/5/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: 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 http://gerrit.cloudera.org:8080/3385 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4bf5dd630f2e175be5800187bbf9337c91f82bd8 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
