Bikramjeet Vig has posted comments on this change. Change subject: IMPALA-1683: Allow REFRESH on a single partition ......................................................................
Patch Set 3: (19 comments) http://gerrit.cloudera.org:8080/#/c/3385/3//COMMIT_MSG Commit Message: PS3, Line 13: query format > syntax Done http://gerrit.cloudera.org:8080/#/c/3385/3/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: PS3, Line 190: 5: optional list<CatalogObjects.TPartitionKeyValue> partition_spec > Can you plz add a comment? Done http://gerrit.cloudera.org:8080/#/c/3385/3/fe/src/main/java/com/cloudera/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/com/cloudera/impala/analysis/ResetMetadataStmt.java: PS3, Line 35: //null if refreshing the whole table or invalidating the entire catalog. > Let's reverse this comment :) "not-null when ...." Done PS3, Line 41: Preconditions.checkArgument(partitionSpec == null : || (partitionSpec != null && name != null && !name.isEmpty())); > isn't this equivalent to partitionSpec == null || name != null? I believe t Done PS3, Line 46: if (partitionSpec_ != null) { : partitionSpec_.setTableName(tableName_); : } > single line? Done http://gerrit.cloudera.org:8080/#/c/3385/3/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java: PS3, Line 1175: HdfsPartition hdfspartition = hdfsTable : .getPartitionFromThriftPartitionSpec(partitionSpec); : // For the case where partition does not exist in Catalog cache but might : // exist in Hive Metastore : String partitionName = hdfspartition == null : ? hdfsTable.getPartitionNameFromThriftPartitionSpec(partitionSpec) : : hdfspartition.getPartitionName(); > You seem to be accessing partition info from a table without protecting it Done PS3, Line 1200: dropPartition(tbl, partitionSpec); > Why are you calling the dropPartition from this class and not the HdfsTable Done PS3, Line 1208: dropPartition(tbl, partitionSpec); : refreshedPartition = hdfsTable.createPartition(partition.getSd(), partition); : addPartition(refreshedPartition); > You're exposing the process of refreshing a table partition to the reader o Done http://gerrit.cloudera.org:8080/#/c/3385/3/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: PS3, Line 1921: this. > Remove 'this' from here and everywhere else, it's not needed. Done PS3, Line 1927: kv.getValue() == null || kv.getValue().isEmpty() > You can probably use Guava's Strings.isNullOrEmpty(kv.getValue()) here Done PS3, Line 1933: if(partitionCols.size()!=partitionVals.size()) return null; > Are you sure this is possible? Won't we catch it during the analysis of the Done Line 1936: partitionCols,partitionVals); > nit: space after ',' Done http://gerrit.cloudera.org:8080/#/c/3385/2/tests/metadata/test_refresh_partition.py File tests/metadata/test_refresh_partition.py: PS2, Line 48: ImpalaDbWrapper > This is a nicely-implemented context manager for setting up and tearing dow Done http://gerrit.cloudera.org:8080/#/c/3385/3/tests/metadata/test_refresh_partition.py File tests/metadata/test_refresh_partition.py: PS3, Line 33: class TestRefreshPartition(ImpalaTestSuite): > Comment what this class is testing Done Line 104: > nit: remove empty line Done PS3, Line 116: """ : Partitions added in Hive can be viewed in Impala after refreshing : partition. : """ > nit: can you keep the comment formatting consistent (e.g. """blah wwfwfw""" Done PS3, Line 137: Partitions > "Partition" Done PS3, Line 158: test_add_data_and_refresh > Another interesting test is to create 2 partitions, insert data through hiv Done Line 267: assert result.data == [str(file_num_rows)] > You can also continue that test by removing the data file, calling refresh 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: 3 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
