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
