Bikramjeet Vig has posted comments on this change. Change subject: IMPALA-1683: Allow REFRESH on a single partition ......................................................................
Patch Set 6: (14 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; > Did you remove the second check? Yes, I realized that the first condition makes sure that the table name is not null if its a refresh statement, therefore my added check was redundant 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 operations are very cheap and the synchronized block protects a speci Done 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 Done PS6, Line 1182: getPartitionFromThriftPartitionSpec > I believe we should name this getPartition(). The name simply describes the this method was already there in the hdfs table class. the reason i think they did this was because there is already a method 'getPartition()' that takes input of type List<PartitionKeyValue>, if I rename this method as well, it would result in an error since this one takes input of type List<TPartitionKeyValue> which is also of List<> type. Would you suggest I create a single getPartition method which calls the right method according to the generic type of the List<> ? 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? Done PS6, Line 1943: from the Partition object > maybe "Reloads the file and block metadata of a partition by dropping it an Done 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 TestReset Done http://gerrit.cloudera.org:8080/#/c/3385/6/tests/metadata/test_refresh_partition.py File tests/metadata/test_refresh_partition.py: PS6, Line 16: import logging : import pytest > Removed unused imports Done PS6, Line 19: import shlex > Unused import Done PS6, Line 21: import subprocess > Unused import Done PS6, Line 23: import * > "import *" is generally regarded as bad practice in python. It's better to Done PS6, Line 52: class ImpalaTableWrapper(object): > Sorry -- I didn't notice before that this class is nested within the outer Done Line 82: for row in rows: > This is perfectly well formed, although it would have been also possible to Done PS6, Line 99: range(0, 16) > You actually don't need the 0, and in python 2.x, it's more customary to us Done -- 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
