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 00:28:27 GMT
Huaisi Xu has posted comments on this change.

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

Patch Set 4:

Commit Message:

PS3, Line 12: all. Backend could
            : overwrites each table in an 1:1 table ID to table descriptor
            : map.
> might need a little rephrase.
what about this?

PS3, Line 18: analyzing.
> operations?

PS3, Line 22: 
> ..issue.. (same in next statement too.)
File fe/src/main/java/org/apache/impala/analysis/

Line 294:     // Catalog cache has the latest tables from the statestore, in order to use
> "Caches all tables referenced in a single query"

PS3, Line 296: same qu
> I think its better to use the parent Map<> class here (that seems to be con
I saw a lot of HashMap in the code as well?

PS3, Line 2282: 
              :    * Returns the Catalog Table object for the given database and table name.
The same
              :    * table reference is returned for multiple references in the same query.
              :    * Adds the table to this analyzer's "missingTbls_" and throws an AnalysisException
              :    * the table has not yet been loaded in the local catalog cache.
> Please update this to reflect the new logic.

Line 2290:   public Table getTable(String dbName, String tableName)
> I'm not sure if this will deal with case-insensitivity properly. Why not us

Line 2291:       throws AnalysisException, TableLoadingException {
> How about this:
File fe/src/main/java/org/apache/impala/analysis/

PS3, Line 45: mple, the ou
> Just curious what the rationale behind this clean up. Is it because we use 
The reason for this is that previously I tried to add the cache here and wanted to use the
name referencedTable_, which was already used.. So I have to give it another name, which I
failed to think of.. I ended up just replace this with something else, and I am not sure if
I should change it back.. the name is quite confusing for now because we only uses it for
insert table. we definitely should change its name to be something sounds like more specific
for partition pruning.
File fe/src/main/java/org/apache/impala/catalog/

Line 34: import java.util.concurrent.atomic.AtomicLong;
> Unused import now that we switched to Long?
done.. there are many unused import.. I will leave the rest there?

Line 141:    * Ever increasing counter for table id {@link Table#id_}. New id is given when
> Add comment that explains the use and behavior of this id, i.e. the id is e
File fe/src/main/java/org/apache/impala/catalog/

Line 65:   // IDs not covered in all other cases.
> Add comment for these subtly different special ids.

Line 67:   // ID used for IncompleteTable when it is uninitialized.
> I think we should add LOCAL_TABLE_ID and TEST_TABLE_ID as well and describe
File fe/src/main/java/org/apache/impala/common/

Line 42: 
> revert

Line 47: 
> revert
File fe/src/main/java/org/apache/impala/service/

Line 1130
> I think we should still maintain something like this to check and document 
Done. the new guarantee in Impala frontend is that same query uses the same table reference
for the same table. Different tables with same ID is guaranteed in catalog.

Line 1134
> There is similar code in a few places in the BE. We should remove that, too
as discussed offline. wont do

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