Alice Fan 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 4: (14 comments) http://gerrit.cloudera.org:8080/#/c/13050/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13050/2//COMMIT_MSG@9 PS2, Line 9: > line wraps. Please don't exceed the col limit Done http://gerrit.cloudera.org:8080/#/c/13050/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13050/3//COMMIT_MSG@9 PS3, Line 9: insert multiple incompatible type values into a > can you clarify what this mean "insert a union"? Done http://gerrit.cloudera.org:8080/#/c/13050/3//COMMIT_MSG@10 PS3, Line 10: error message should blame on the correct expression. If there > multiple expressions with an incompatible type Done http://gerrit.cloudera.org:8080/#/c/13050/3/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/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2326 PS3, Line 2326: expression for each column i > return a list of the first widest compatible expression for each column Done http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2326 PS3, Line 2326: Retu > where are we logging this? updated comments. thanks! http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2340 PS3, Line 2340: Type compatibleType = firstList.get(i).getType(); > nit: dont think this context is necessary here Done http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2347 PS3, Line 2347: // compatibleType will be updated if a new wider type is encountered > nit: spavce between // and The Done http://gerrit.cloudera.org:8080/#/c/13050/2/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/2/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@690 PS2, Line 690: // widestTypeExpr is widest type expression for column i > +1. Can you simplify this? Done http://gerrit.cloudera.org:8080/#/c/13050/1/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/13050/1/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@53 PS1, Line 53: > Any reason to put this here instead of in UnionStmt? Done http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@53 PS3, Line 53: : protected List<OrderByElement> orderByElements_; : protected LimitElement limitElement_; : : // For a select statment: > can you rephrase this to be more concise. Please look at other comments on Thanks. updated comments http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@58 PS3, Line 58: xprs in sele > Like bharath mentioned, you can add this to UnionStmt instead and in Insert Done http://gerrit.cloudera.org:8080/#/c/13050/2/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/2/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@195 PS2, Line 195: * > Best to remove trailing spaces. Eclipse can do this for you. I think it is Thanks Paul. it is handy! http://gerrit.cloudera.org:8080/#/c/13050/3/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/3/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@196 PS3, Line 196: * widestTypeSrcExpr is the widest type expression for all the source values. : * Error message should blame on the widestTypeSrcExpr instead of the first : * compatible source > looks like an outdated comment, can up update it based on the changes in yo Done http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@232 PS3, Line 232: : : : > since checkTypeCompatibility is only used in 2 places, there is no need to 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: 4 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: Fri, 03 May 2019 05:33:17 +0000 Gerrit-HasComments: Yes
