Bikramjeet Vig 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 7: (4 comments) Looks good just a few more nits 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() == > Thanks for catching this! yes it will be but in that case you will have to change the description of this function to not return anything if exprLists.size() == 1. I think its more consistent if we return something for a non empty exprLists http://gerrit.cloudera.org:8080/#/c/13050/7/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/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2326 PS7, Line 2326: Return a list of the first widest compatible expression for each column in an : * order consistent with the order of target columns. sorry for changing the suggestions for the comments again and again, the terminology is kinda tricky. I think the following should be sufficient and generic enough to make sense in the context of this method's input params: Returns a list of exprs such that for every i-th expr in that list, it is the first widest compatible expression encountered among all i-th exprs in the expr lists. http://gerrit.cloudera.org:8080/#/c/13050/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2332 PS7, Line 2332: if (exprLists == null || exprLists.size() == 0) return null; I think having an if statement for exprLists.size() == 1 wont hurt. Just return exprLists.get(0) in that case. http://gerrit.cloudera.org:8080/#/c/13050/7/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/7/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java@161 PS7, Line 161: // this contains a list of the first widest (highest precision) compatible : // expression for each column. Contains a list of exprs such that for every i-th expr in that list, it is the first widest compatible expression encountered among all i-th exprs in every result expr list of the union operands. -- 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: 7 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: Thu, 09 May 2019 01:19:51 +0000 Gerrit-HasComments: Yes
