Alex Behm has posted comments on this change. Change subject: IMPALA-4336: Cast exprs after unnesting union operands. ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/4815/1//COMMIT_MSG Commit Message: PS1, Line 15: Also clarifies the use of resultExprs vs. baseTblResultExprs : in unions. We should consistently use resultExprs because the : final expr substitution happens during planning. : The only place where baseTblResultExprs should be used is in : UnionStmt.materializeRequiredSlots() because that is called : before plan generation and we need to mark the slots of resolved : exprs as materialized. > I am not sure this belongs to the commit message. If you don't have it alre Shrunk comment here. Moved expanded comment to UnionStmt class comment. http://gerrit.cloudera.org:8080/#/c/4815/1/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java File fe/src/main/java/org/apache/impala/analysis/UnionStmt.java: PS1, Line 202: Propagates DISTINCT from left to right, and checks that all union operands are : * are union compatible, adding implicit casts if necessary. > I think you need to update this comment. This function seems to be doing a I removed this comment because it's redundant with the class comment. Let me know if you feel the class comment still needs to be expanded/moved. PS1, Line 203: are > remove Done PS1, Line 218: // Analyze all operands and make sure they return an equal number of exprs. : for (int i = 0; i < operands_.size(); ++i) { : try { : operands_.get(i).analyze(analyzer); : QueryStmt firstQuery = operands_.get(0).getQueryStmt(); : List<Expr> firstExprs = operands_.get(0).getQueryStmt().getResultExprs(); : QueryStmt query = operands_.get(i).getQueryStmt(); : List<Expr> exprs = query.getResultExprs(); : if (firstExprs.size() != exprs.size()) { : throw new AnalysisException("Operands have unequal number of columns:\n" + : "'" + queryStmtToSql(firstQuery) + "' has " + : firstExprs.size() + " column(s)\n" + : "'" + queryStmtToSql(query) + "' has " + exprs.size() + " column(s)"); : } : } catch (AnalysisException e) { : if (analyzer.getMissingTbls().isEmpty()) throw e; : } : } > This function has grown a bit too much. I would put this block is a separat Done Line 241: // Remember SQL string before unnesting operands. > the Done PS1, Line 258: // Cast all result exprs to a compatible type. > move above L263 We need to collect them first before we can analyzer.castToUnionCompatibleTypes(), so seems relevant to this loop as well. Modified comment. PS1, Line 282: // Should never happen : throw new AnalysisException("Error creating agg info in UnionStmt.analyze()"); > Maybe convert it to Preconditions.checkState(false, "Error....")? Modified to an ISE which is what the Preconditions would also throw. I prefer the ISe because we can give it a cause. http://gerrit.cloudera.org:8080/#/c/4815/1/testdata/workloads/functional-query/queries/QueryTest/union.test File testdata/workloads/functional-query/queries/QueryTest/union.test: PS1, Line 1030: (select 80 union all select 90) > Maybe add a test case with nested union distinct, if not already there. A nested union distinct cannot be unnested, so is not really related to this bug. We have tests for that elsewhere. -- To view, visit http://gerrit.cloudera.org:8080/4815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5e3ab7349df7d67d0d9c2baf4a56342d3f04e76d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-HasComments: Yes
