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 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/13050/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13050/1//COMMIT_MSG@7 PS1, Line 7: Type errors are attributed to wrong expression with insert nit: Fix type errors being attributed to wrong expressions 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 a union statement with incompatible types can you clarify what this mean "insert a union"? http://gerrit.cloudera.org:8080/#/c/13050/3//COMMIT_MSG@10 PS3, Line 10: make error message blame on correct expression. If there are multiple multiple expressions with an incompatible type 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: return a list of widestExprs return a list of the first widest compatible expression for each column http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2326 PS3, Line 2326: log where are we logging this? http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2340 PS3, Line 2340: // Remember widest compatible Expr for error reporting. nit: dont think this context is necessary here http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2347 PS3, Line 2347: //The compatibleType will be the wider type of previous and next Exprs nit: spavce between // and The nit: maybe write something like: compatibleType only changes if a new wider type is encountered 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: // When a union statement runs castToUnionCompatibleTypes(), : // a list of widest type expression for each column will be returned. : // widestExpr is the expression with highest precision data type. : // The widestExprs_ is stored as a member of QueryStmt : // is because it is used when an insert statement prepares expression can you rephrase this to be more concise. Please look at other comments on variable to get an idea on what kind of info should be mentioned. http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@58 PS3, Line 58: widestExprs_ Like bharath mentioned, you can add this to UnionStmt instead and in InsertStmt, just check if its on UnionStmt, cast it and use an assessor method or something to get the required widest expression 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: * QueryStmt's widestExprs_ holds list of source expression with widest (highest : * precision) type. Error message will blame the source expression with widest : * incompatible type. looks like an outdated comment, can up update it based on the changes in your latest patchset http://gerrit.cloudera.org:8080/#/c/13050/3/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@232 PS3, Line 232: protected Expr checkTypeCompatibility(String dstTableName, Column dstCol, : Expr srcExpr, boolean strictDecimal) throws AnalysisException { : return checkTypeCompatibility(dstTableName, dstCol, srcExpr, strictDecimal, srcExpr); : } since checkTypeCompatibility is only used in 2 places, there is no need to have another method just to have a default param. Also instead of passing srcExprm, you can just pass null since you are already handling that case in L209 http://gerrit.cloudera.org:8080/#/c/13050/1/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/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@3889 PS1, Line 3889: "Column permutation mentions fewer columns (9) than the SELECT /" + : " VALUES clause returns (11)"); : // Omitting the row-key column is an error : AnalysisError("insert " + qualifier + " table functional_hbase.alltypesagg" + : "(bool_col, tinyint_col, smallint_col, int_col, bigin did you change the clone method before incrementing this? -- 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: 3 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: Wed, 01 May 2019 23:15:04 +0000 Gerrit-HasComments: Yes
