Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-1702: Enforce table level consistency accross service ......................................................................
Patch Set 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/4349/9//COMMIT_MSG Commit Message: PS9, Line 7: Enforce table level consistency accross service IMO, this is a little misleading as its sounds similar to consistency guarantees in DBs. Instead we should have something like "Make table-id assignment query local" or something similar. Thoughts? http://gerrit.cloudera.org:8080/#/c/4349/9/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 295: private final HashMap<TableName, Table> referencedTables_ = Maps.newHashMap(); Is this change (maintaining referenced tables cache) still required with the latest patch design with per-query id assignment? Given Catalog doesn't assign an Id anymore, why do we need to have the same reference of the table? May be I'm missing something. http://gerrit.cloudera.org:8080/#/c/4349/9/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: PS9, Line 187: To ensure the table ID is unique : // in the same query, assign a special CTAS ID. The CatalogServer will assign this : // table a proper ID once it is created there as part of the CTAS execution. update this as per new design. http://gerrit.cloudera.org:8080/#/c/4349/9/fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java File fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java: PS9, Line 154: /** : * Connect tupleDescriptors to tableDescriptors with unique table ids. : * Verify table level consistency in the same query by checking references to the same : * table refer to the same table instance. Nit: This should probably be moved to the line before the logic given this is not what this method does. Line 164: HashMap<TableName, Table> referencedTables = Maps.newHashMap(); Same question as in referencedTables_ in Analyzer. Assuming we want to retain referencedTables_ there, why do we need to repeat it here. Can't we reuse it? http://gerrit.cloudera.org:8080/#/c/4349/9/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: Line 37: import org.apache.impala.analysis.TableName; Move it below to org.apache.impala group. -- 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: 9 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
