Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-1683: Allow REFRESH on a single partition ......................................................................
Patch Set 6: (7 comments) 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); : this.tableName_ = name; > Done Did you remove the second 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 1174: Db db = tbl.getDb(); > These 3 variables can be moved to L1195 but I kept them here because I want These operations are very cheap and the synchronized block protects a specific table object so I am not really worried about them. So, lets move them closer to where they've been used to improve the readability of this function. http://gerrit.cloudera.org:8080/#/c/3385/6/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java: PS6, Line 1164: To achieve that, it fetches a fresh copy of : * the partition from Hive metastore and passes to it the table object to : * reload the metadata from it No need to describe the implementation unless it is something non-intuitive that a reader will not be able to figure out by reading the code. PS6, Line 1182: getPartitionFromThriftPartitionSpec I believe we should name this getPartition(). The name simply describes the type of the input param which is kind of redundant. http://gerrit.cloudera.org:8080/#/c/3385/6/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: PS6, Line 1926: public String constructPartitionName(List<TPartitionKeyValue> partitionSpec) { Can you add a function comment? PS6, Line 1943: from the Partition object maybe "Reloads the file and block metadata of a partition by dropping it and adding it back to the table". You need to comment on the input params. It's not clear why you need the partitionSpec even though you have the partition object. http://gerrit.cloudera.org:8080/#/c/3385/6/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java: PS6, Line 785: //Positive cases for checking refresh partition : AuthzOk("refresh functional.alltypesagg partition (year=2010, month =1, day = 1)"); : AuthzOk("refresh functional.alltypes partition (year =2009, month=1)"); : AuthzOk("refresh functional_seq_snap.alltypes partition (year =2009, month=1)"); You also need to add some analysis tests in AnalyzerTest.java (fn TestResetMetadata()). -- 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: 6 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
