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 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/13050/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13050/5//COMMIT_MSG@7 PS5, Line 7: IMPALA-966: Type errors are attributed to wrong expression with insert : : When insert multiple incompatible type values into a table, : error message should blame on the correct expression. If there : are multiple incompatible type values for a single target : column, error should blame on the first widest incompatible type : expression. > how about : Nice example. Much appreciate for the help! http://gerrit.cloudera.org:8080/#/c/13050/5/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/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@692 PS5, Line 692: // If the queryStmt_ is a unionStmt, it will return a WidestExprs list : // when do castToUnionCompatibleTypes(). : // widestTypeExpr will be null if the queryStmt_ is a SelectStmt > nit: superfluous comment Done http://gerrit.cloudera.org:8080/#/c/13050/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@695 PS5, Line 695: UnionStmt unionStmt = : (queryStmt_ instanceof UnionStmt) ? (UnionStmt) queryStmt_ : null; : if (unionStmt != null && unionStmt.getWidestExprs() != null : && unionStmt.getWidestExprs().size() > 0) { : widestTypeExpr = unionStmt.getWidestExprs().get(i); : } > nit: instead of doing this in every loop maybe just get the widestExprList Done http://gerrit.cloudera.org:8080/#/c/13050/5/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java File fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java: http://gerrit.cloudera.org:8080/#/c/13050/5/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java@292 PS5, Line 292: null > nit: remove the comment above and add an inline comment here like Done http://gerrit.cloudera.org:8080/#/c/13050/5/fe/src/main/java/org/apache/impala/analysis/StatementBase.java File fe/src/main/java/org/apache/impala/analysis/StatementBase.java: http://gerrit.cloudera.org:8080/#/c/13050/5/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@196 PS5, Line 196: for > nit: among Done http://gerrit.cloudera.org:8080/#/c/13050/5/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@196 PS5, Line 196: widestTypeSrcExpr > nit: add quotes since this refers to an input param Done http://gerrit.cloudera.org:8080/#/c/13050/5/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@197 PS5, Line 197: Error message should blame on the widestTypeSrcExpr instead of the first : * compatible source expression. > nit: is only used when constructing an AnalysisException message to make su Done http://gerrit.cloudera.org:8080/#/c/13050/5/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/5/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java@56 PS5, Line 56: // widestExprs_ is a list of the first widest compatible expression for each column > nit: you can remove the first line and write "widest (highest precision)" Done http://gerrit.cloudera.org:8080/#/c/13050/5/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/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3404 PS5, Line 3404: on > nit: the Done http://gerrit.cloudera.org:8080/#/c/13050/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3404 PS5, Line 3404: // Error should blame on correct expression. : // The widest (highest precision) expression and type should appear in error. > nit: these two are a bit repetitive. can you consolidate both into one? Done http://gerrit.cloudera.org:8080/#/c/13050/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3422 PS5, Line 3422: } > can you add a case where there are multiple expressions with the same incom Done -- 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: 5 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: Tue, 07 May 2019 06:52:32 +0000 Gerrit-HasComments: Yes
