impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Huaisi Xu (Code Review)" <>
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:

File fe/src/main/java/org/apache/impala/analysis/

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

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

Line 2299:     if (table == null) {
> remove
File fe/src/main/java/org/apache/impala/analysis/

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:
File fe/src/main/java/org/apache/impala/analysis/

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
File fe/src/main/java/org/apache/impala/catalog/

Line 141:    * Ever increasing counter for table id {@link Table#id_}. New id is given when
> TableId doesn't exist anymore.
File fe/src/main/java/org/apache/impala/catalog/

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.
File fe/src/main/java/org/apache/impala/planner/

Line 25:   // Construction only allowed via an IdGenerator.
> newline before comment
File fe/src/main/java/org/apache/impala/service/

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 not sure if that is what you suggested.k

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifad648b72684ae495ec387590ab1bc58ce5b39e2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Huaisi Xu <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Huaisi Xu <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message