Alex Behm has posted comments on this change. Change subject: IMPALA-1702: Enforce table level consistency accross service ......................................................................
Patch Set 9: (2 comments) 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 How about we prefix this comment with a brief description of what this method does? I'd prefer to keep the validation comment in the class comment, there seems to be no good place to put it inside the method, and callers might be interested in knowing about the behavior without reading the whole method. Line 164: HashMap<TableName, Table> referencedTables = Maps.newHashMap(); > The map is only for validation purposes. Sorry, I see what you mean now. Yes, that would work as well, but we'd need to pass the Analyzer in here which does not seem to make sense. An alternative approach is to move Analyzer.referencedTables_ into the DescriptorTable. Either the current or the latter solutions work for me. We don't save/lose much either way. -- 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
