Alex Behm has posted comments on this change. Change subject: IMPALA-1702: Enforce table level consistency accross service ......................................................................
Patch Set 5: (4 comments) 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 > The only place that this actually becomes useful is for partition pruning? I still don't understand what this has to do with partition pruning. The purpose of this list is correctly described in the original comment. It is to ship the descriptors of these tables to the BE. If we don't do this, then the BE will not know where to write data to in an insert because the base and partition paths are part of the table metadata. The purpose really is to ship the table metadata to the BE and I don't see relevance to partition pruning. Imo, this should be like you had it before. It should be called targetTable_ or insertTable_ or insertTargetTable_ or something like that. http://gerrit.cloudera.org:8080/#/c/4349/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: Line 141: * TableId does not exist anymore. Sorry I got confused, you can remove this sentence. I incorrectly thought the @link below referenced the TableId class. 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(); > remove which? ?removed INVALID_TABLE_ID. not sure if this is what you wante The metastoreAccessLock_ is unused. Please remove. http://gerrit.cloudera.org:8080/#/c/4349/5/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: Line 79: protected final long id_; As discussed, thinking about this a little more I think the right fix is to get rid of this id altogether. The table id is only used for mapping tuple descriptors to tables in the BE - a query local mapping. So I think the better fix is to remove the table id here and instead assign ids on-the-fly (or establish the mapping some other way) in DescriptorTbl.toThrift(). -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
