impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1683: Allow REFRESH on a single partition
Date Thu, 07 Jul 2016 22:30:34 GMT
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 <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message