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 Tue, 12 Jul 2016 03:31:15 GMT
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 <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