Huaisi Xu has posted comments on this change.

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


Patch Set 4:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/4349/3//COMMIT_MSG
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?
Done


PS3, Line 22: 
> ..issue.. (same in next statement too.)
Done


http://gerrit.cloudera.org:8080/#/c/4349/3/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
> "Caches all tables referenced in a single query"
Done


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 if
              :    * the table has not yet been loaded in the local catalog 
cache.
> Please update this to reflect the new logic.
Done


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


Line 2291:       throws AnalysisException, TableLoadingException {
> How about this:
Done


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

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.


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

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
Done


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

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


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
Done


http://gerrit.cloudera.org:8080/#/c/4349/3/fe/src/main/java/org/apache/impala/common/Id.java
File fe/src/main/java/org/apache/impala/common/Id.java:

Line 42: 
> revert
Done


Line 47: 
> revert
Done


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

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 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 <h...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <h...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to