[Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Internal Jenkins has posted comments on this change. Change subject: IMPALA-4336: Cast exprs after unnesting union operands. .. Patch Set 3: Verified+1 -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Internal Jenkins Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Internal Jenkins has submitted this change and it was merged. Change subject: IMPALA-4336: Cast exprs after unnesting union operands. .. IMPALA-4336: Cast exprs after unnesting union operands. The bug was that we cast the result exprs of operands before unnesting them. If we unnested an operands, casts were missing on those unnested operands' result exprs. The fix is to first unnest operands and then cast result exprs. Also clarifies the use of resultExprs vs. baseTblResultExprs. Change-Id: I5e3ab7349df7d67d0d9c2baf4a56342d3f04e76d Reviewed-on: http://gerrit.cloudera.org:8080/4815 Reviewed-by: Alex Behm Tested-by: Internal Jenkins --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java M fe/src/main/java/org/apache/impala/planner/UnionNode.java M testdata/workloads/functional-query/queries/QueryTest/union.test 5 files changed, 137 insertions(+), 104 deletions(-) Approvals: Internal Jenkins: Verified Alex Behm: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/4815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5e3ab7349df7d67d0d9c2baf4a56342d3f04e76d Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Internal Jenkins
[Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Alex Behm has posted comments on this change. Change subject: IMPALA-4336: Cast exprs after unnesting union operands. .. Patch Set 3: Code-Review+2 rebase -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Alex Behm has posted comments on this change. Change subject: IMPALA-4336: Cast exprs after unnesting union operands. .. Patch Set 2: Code-Review+2 -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4336: Cast exprs after unnesting union operands. .. Patch Set 2: Code-Review+1 (1 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 282: operands_.get(i).analyze(analyzer); : QueryStmt firstQuery = operands_.get(0).getQueryStmt(); > Modified to an ISE which is what the Preconditions would also throw. I pref Sounds good to me. -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
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 firstExprs = operands_.get(0).getQueryStmt().getResultExprs(); : QueryStmt query = operands_.get(i).getQueryStmt(); : List 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Alex Behm has uploaded a new patch set (#2). Change subject: IMPALA-4336: Cast exprs after unnesting union operands. .. IMPALA-4336: Cast exprs after unnesting union operands. The bug was that we cast the result exprs of operands before unnesting them. If we unnested an operands, casts were missing on those unnested operands' result exprs. The fix is to first unnest operands and then cast result exprs. Also clarifies the use of resultExprs vs. baseTblResultExprs. Change-Id: I5e3ab7349df7d67d0d9c2baf4a56342d3f04e76d --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java M fe/src/main/java/org/apache/impala/planner/UnionNode.java M testdata/workloads/functional-query/queries/QueryTest/union.test 5 files changed, 137 insertions(+), 104 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/4815/2 -- To view, visit http://gerrit.cloudera.org:8080/4815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5e3ab7349df7d67d0d9c2baf4a56342d3f04e76d Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Dimitris Tsirogiannis
[Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4336: Cast exprs after unnesting union operands. .. Patch Set 1: (8 comments) Flushing some initial comments. Still need to wrap my head around all the moving parts. 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 already, you may want to put it as a comment somewhere in the code. 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 lot more than simply propagating distinct and checking union operand compatibility. PS1, Line 203: are remove 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 firstExprs = operands_.get(0).getQueryStmt().getResultExprs(); : QueryStmt query = operands_.get(i).getQueryStmt(); : List 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 separate function called analyzeOperands(). Line 241: // Remember SQL string before unnesting operands. the PS1, Line 258: // Cast all result exprs to a compatible type. move above L263 PS1, Line 282: // Should never happen : throw new AnalysisException("Error creating agg info in UnionStmt.analyze()"); Maybe convert it to Preconditions.checkState(false, "Error")? 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. -- 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 Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4336: Cast exprs after unnesting union operands.
Alex Behm has uploaded a new change for review. http://gerrit.cloudera.org:8080/4815 Change subject: IMPALA-4336: Cast exprs after unnesting union operands. .. IMPALA-4336: Cast exprs after unnesting union operands. The bug was that we cast the result exprs of operands before unnesting them. If we unnested an operands, casts were missing on those unnested operands' result exprs. The fix is to first unnest operands and then cast result exprs. 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. Change-Id: I5e3ab7349df7d67d0d9c2baf4a56342d3f04e76d --- M fe/src/main/java/org/apache/impala/analysis/Expr.java M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java M fe/src/main/java/org/apache/impala/planner/UnionNode.java M testdata/workloads/functional-query/queries/QueryTest/union.test 5 files changed, 111 insertions(+), 89 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/4815/1 -- To view, visit http://gerrit.cloudera.org:8080/4815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5e3ab7349df7d67d0d9c2baf4a56342d3f04e76d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm