Alice Fan 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) Thanks for catching this! Originally, I thought the exprLists.size() == 1 case will be handle by statementBase.checkTypeCompatibility(). widestExpr == null, basically will be replaced by srcExpr, which is the 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 Done 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 Done 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( Moved into the list of "members that need to be reset()". Remove the part for stored in order, because there are other list members doesn't not explain the order. Also, the order is explained at comment area of analyzer.castToUnionCompatibleTypes() when the widestExprs is returned. reset() method has been updated at previous patch. which is at line648. also update clone at line 199 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 Updated comments. 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. added short comment for each of test. -- 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 <fan...@gmail.com> Gerrit-Reviewer: Alice Fan <fan...@gmail.com> Gerrit-Reviewer: Anonymous Coward <xiaom...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Comment-Date: Wed, 08 May 2019 22:18:57 +0000 Gerrit-HasComments: Yes
