Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13050 )
Change subject: IMPALA-966: Type errors are attributed to wrong expression with insert ...................................................................... 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 : IMPALA-966: Attribute type errors to the right expression in an insert statement Currently if an insert statement contains multiple expressions that are incompatible with the column type, the error message returned attributes the error to the wrong expression. This patch makes sure the right expression is blamed. If there are multiple incompatible type values for the target column, then the error is attributed to the first widest (highest precision) incompatible type expression. 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 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 before the loop and use it if not null 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 .., analyzer.isDecimalV2(), null /*widestTypeSrcExpr*/); 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: widestTypeSrcExpr nit: add quotes since this refers to an input param 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 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 sure the right expression is blamed in the error message 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)" here. Also, can you mention what order they are stored in and add a full stop at the end 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 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? 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 incompatible type and the first one gets blamed. -- 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 00:24:10 +0000 Gerrit-HasComments: Yes
