Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13050 )
Change subject: IMPALA-966: Attribute type error to the right expression in an insert statement ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2332 PS6, Line 2332: exprLists.size() < 2 looks like if exprLists.size() =1 then it should return exprLists.get(0) http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2348 PS6, Line 2348: if (preType != compatibleType) { : widestExprs.set(i, exprLists.get(j).get(i)); : } nit: might fit in one line if (preType != compatibleType) widestExprs.set(i, exprLists.get(j).get(i)); http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@699 PS6, Line 699: Expr widestTypeExpr = null; : if (widestTypeExprList != null) { : widestTypeExpr = widestTypeExprList.get(i); : } nit: can be one line http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java File fe/src/main/java/org/apache/impala/analysis/UnionStmt.java: http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java@55 PS6, Line 55: // Widest (highest precision) expression is a list of the first widest compatible : // expression for each column. The list is stored in the order of target columns. : protected List<Expr> widestExprs_ = new ArrayList<>(); this needs to go after L133 ( in the list of members that need to be reset()). Also, can you update the comment with similar information as given in the members in that list. And update the reset() method too with the new addition http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3404 PS6, Line 3404: on nit: typo: blame the widest http://gerrit.cloudera.org:8080/#/c/13050/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3407 PS6, Line 3407: can you add a one line comment for each test case as to what it is testing. See the test cases in testInsertDynamic -- To view, visit http://gerrit.cloudera.org:8080/13050 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88718fc2cbe1a492165435a542fd2d91d8693a39 Gerrit-Change-Number: 13050 Gerrit-PatchSet: 6 Gerrit-Owner: Alice Fan <[email protected]> Gerrit-Reviewer: Alice Fan <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Tue, 07 May 2019 20:22:23 +0000 Gerrit-HasComments: Yes
