impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Huaisi Xu (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1702: Enforce table level consistency
Date Fri, 07 Oct 2016 00:40:13 GMT
Huaisi Xu has posted comments on this change.

Change subject: IMPALA-1702: Enforce table level consistency
......................................................................


Patch Set 10:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4349/9//COMMIT_MSG
Commit Message:

PS9, Line 7: Enforce table level consistency
> IMO, this is a little misleading as its sounds similar to consistency guara
I think this is just a high level description. It hides implementation details. This is the
goal we are trying to achieve. and it contains two things.


http://gerrit.cloudera.org:8080/#/c/4349/9/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 295:     private final HashMap<TableName, Table> referencedTables_ = Maps.newHashMap();
> Thanks for the explanation Alex. Makes sense to me.
Although this is not strictly necessary, this is more of completing the circle.


http://gerrit.cloudera.org:8080/#/c/4349/9/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java:

PS9, Line 187: 
             :       table.load(true, client.getHiveClient(), msTbl);
             :       insertStmt_.setTargetTable(table);
> update this as per new design.
Done


http://gerrit.cloudera.org:8080/#/c/4349/9/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java
File fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java:

PS9, Line 154: /**
             :    * Connect tupleDescriptors to tableDescriptors with unique table ids and
get
             :    * this DescriptorTable ready to be sent to backend.
             :    */
> How about we prefix this comment with a brief description of what this meth
Done


Line 164: 
> Sorry, I see what you mean now. Yes, that would work as well, but we'd need
This is only for table validation so same table references have the same instance. If this
is for validation purposes then it makes more sense to isolate this from anything we used
in the code.


http://gerrit.cloudera.org:8080/#/c/4349/9/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java:

Line 37: import org.apache.kudu.client.KuduClient;
> Move it below to org.apache.impala group.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4349
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifad648b72684ae495ec387590ab1bc58ce5b39e2
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message