Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12221 )
Change subject: IMPALA-5872: Testcase builder for query planner ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/analysis/HdfsUri.java File fe/src/main/java/org/apache/impala/analysis/HdfsUri.java: http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/analysis/HdfsUri.java@79 PS7, Line 79: boolean registerPrivReq, boolean throwIfNotExists) throws AnalysisException { > Point was, is helpful for the variable to state the behavior it controls (" Thanks for clarifying, done. http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@1173 PS7, Line 1173: "supported in a single expression: " + conjunct.toSql()); > Agreed. But the wording sounds much like what we'd issue elsewhere with an Agree with your comment. I backed this up with code comments. Please let me know if it isn't clear. http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@411 PS7, Line 411: if (ret != null) { > Then we have an inconsistency. In Catalog.java, the code is: Done http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@424 PS7, Line 424: t.getLock().lock(); > Makes sense. Perhaps a comment or two in the code to explain this thinking Done -- To view, visit http://gerrit.cloudera.org:8080/12221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec83eeb2dc5136768b70ed581fb8d3ed0335cb52 Gerrit-Change-Number: 12221 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Balazs Jeszenszky <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Thu, 07 Feb 2019 20:00:50 +0000 Gerrit-HasComments: Yes
