Bikramjeet Vig has posted comments on this change. Change subject: IMPALA-1683: Allow REFRESH on a single partition ......................................................................
Patch Set 9: (17 comments) Note: I will be moving this check-in to the ASF gerrit repo as soon as I get +2 http://gerrit.cloudera.org:8080/#/c/3385/9/fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java: PS9, Line 1182: ((HdfsTable) tbl); > nit: extraneous parentheses Done Line 1185: // Retrieve partition name from existing partition or construct it from > does it make sense to do some basic sanity testing here? like checking the the partition spec we receive here already goes through analysis check after parsing step. If there was any incompatibility then it would be caught there PS9, Line 1204: if (hdfspartition != null) { : hdfsTable.dropPartition(partitionSpec); : tbl.setCatalogVersion(newCatalogVersion); : } > is this path covered by your tests at all? Yes, this is covered by 'test_drop_hive_partition_and_refresh' in test_refresh_partition.py http://gerrit.cloudera.org:8080/#/c/3385/9/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: PS9, Line 1929: public String > can this be static? Done PS9, Line 1931: for (int i = 0; i < getNumClusteringCols(); ++i) { : partitionCols.add(getColumns().get(i).getName()); : } > seems like a good idea to add Table.getClusteringCols() for this purpose. I realized that we don't really need anything from the HdfsTable object, this makes it simpler to change this method to static and only use the TPartitionKeyValue objects to construct the name PS9, Line 1936: kv : > tiny nit: we don't put a space before a colon in for expressions like this. Done PS9, Line 1952: Partition hmsPartition) > can this parameter fit on the previous line? Done Line 1953: dropPartition(oldPartition); > can you check (via precondition) that oldPartition and hmsPartition refer t Done http://gerrit.cloudera.org:8080/#/c/3385/9/tests/metadata/test_refresh_partition.py File tests/metadata/test_refresh_partition.py: PS9, Line 21: nsion, \ > long line Done PS9, Line 27: class ImpalaTableWrapper(object): > My take is that while this could be useful for some test code, it's a bit s No, I don't anticipate any such test. Therefore I think the best thing would be to remove this class from here. PS9, Line 41: (self.table_name, self.table_spec)) > combine with previous line? Done PS9, Line 109: cant > can't Done PS9, Line 168: s > exist Done PS9, Line 172: db_name + '.' + self.unique_string() > since you do this on every (?) construction of an ImpalaTableWrapper, why n Done PS9, Line 266: \ > nit: remove Done PS9, Line 281: dst_path = "%s/year=2010/month=1/%s" % (table_location, file_name) : check_call(["hadoop", "fs", "-cp", "-f", src_file, dst_path], shell=False) : dst_path = "%s/year=2010/month=2/%s" % (table_location, file_name) : check_call(["hadoop", "fs", "-cp", "-f", src_file, dst_path], shell=False) > nit: it's maybe worth, in this case, replacing this with a two or three lin Done PS9, Line 295: % > long line 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: 9 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-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
