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 accross service
Date Tue, 04 Oct 2016 17:45:09 GMT
Huaisi Xu has posted comments on this change.

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


Patch Set 4:

(11 comments)

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

Line 294:     // Catalog cache has the latest tables from the statestore, in order to use
the
> The Impalad Catalog has ...
Done


Line 295:     // same table version in a single query, we cache all referenced tables and
reuse them
> ... query , we cache all referenced tables here.
Done


Line 2299:     if (table == null) {
> remove
Done


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

Line 187:       // SelectStmt (or the BE will be very confused). Assign a unique table ID
to it.
> The previous longer comment was clearer. How about:
Done


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

Line 44:   // List of tables to exclude from partition pruning, and thus all partitions are
sent to
> Afaik, these tables have nothing to do with partition pruning, so the name 
The only place that this actually becomes useful is for partition pruning? so for "INSERT
INTO a PARTITION(b='d') select c from a;
if c is in another partition other than partition 'b', then without this partition b's metadata
wont be sent. it has no other meaningful usage than this I think. maybe I just change this
to a HashSet?


Line 181:       referencedPartitions = getReferencedPartitions(tbl);
> will remove this
Done


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

Line 141:    * Ever increasing counter for table id {@link Table#id_}. New id is given when
> TableId doesn't exist anymore.
Done


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

Line 64:   private static final Object metastoreAccessLock_ = new Object();
> Please remove while you are here.
remove which? ?removed INVALID_TABLE_ID. not sure if this is what you wanted.


Line 72:   public static final long LOCAL_TABLE_ID = -4;
> LOCAL_VIEW_ID is more explicit. Sorry to make you change it again.
Done


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/planner/JoinTableId.java
File fe/src/main/java/org/apache/impala/planner/JoinTableId.java:

Line 25:   // Construction only allowed via an IdGenerator.
> newline before comment
Done


http://gerrit.cloudera.org:8080/#/c/4349/4/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

Line 1126:    * Check that we don't have any duplicate table IDs (see IMPALA-1702).
> We should declare IMPALA-1702 as fixed and not mention it here. Instead, we
I added something in ImpaladCatalog.java::addTable(). not sure if that is what you suggested.k


-- 
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: 4
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