This is an automated email from the ASF dual-hosted git repository. stigahuang pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 0bc562a0200f285fe3f526cf7e0ce88cc89e94e7 Author: Daniel Becker <[email protected]> AuthorDate: Tue Sep 27 14:40:12 2022 +0200 IMPALA-10356: Fix SetOperationStmt::toSql() for VALUES statement with single value If the query contains a VALUES clause with a single value, the 'analyzed query' in the level 2 explain plan contains a UNION where both sides are the value from the VALUES clause. For INSERT INTO double_tbl VALUES (-0.43149576573887316); we get SELECT CAST(-0.43149576573887316 AS DECIMAL(17,17)) UNION SELECT CAST(-0.43149576573887316 AS DECIMAL(17,17)); This is because of a bug in 'SetOperationStmt::toSql()'; 'ValuesStmt's are implemented as special 'UnionStmt's, which derive from 'SetOperationStmt'. 'SetOperationStmt::toSql()' implicitly assumes that there are at least 2 operands and always prints the first one and the last one separately, even if they are the same. Note that 'ValuesStmt' is the only 'SetOperationStmt' that may contain a single operand - it is syntactically impossible in SQL to create other 'SetOperationStmt's with only one operand. This patch modifies 'SetOperationStmt::toSql()' so that if there is only one operand, only that operand is printed (so there is no reference to the set operation in the resulting string). This change does not affect how such queries are handled, only how the analyzed query is printed. Testing: - Extended AnalyzeStmtsTest.TestValuesStmt so it now also checks what VALUES clauses with one and two operands are rewritten to during analysis. Change-Id: I952377ed14eba26e3774e7776eb81a95d1d8e76f Reviewed-on: http://gerrit.cloudera.org:8080/19048 Tested-by: Impala Public Jenkins <[email protected]> Reviewed-by: Daniel Becker <[email protected]> --- .../apache/impala/analysis/SetOperationStmt.java | 34 +++++++++++++++------- .../apache/impala/analysis/AnalyzeStmtsTest.java | 6 ++++ .../apache/impala/analysis/ExprRewriterTest.java | 2 -- .../queries/PlannerTest/resource-requirements.test | 4 +-- 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java b/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java index ccf81c30b..d34a2cb20 100644 --- a/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java @@ -510,7 +510,29 @@ public class SetOperationStmt extends QueryStmt { strBuilder.append(" "); } + operandsToSql(options, strBuilder); + + // Order By clause + if (hasOrderByClause()) { + strBuilder.append(" ORDER BY "); + for (int i = 0; i < orderByElements_.size(); ++i) { + strBuilder.append(orderByElements_.get(i).toSql(options)); + strBuilder.append((i + 1 != orderByElements_.size()) ? ", " : ""); + } + } + // Limit clause. + strBuilder.append(limitElement_.toSql(options)); + return strBuilder.toString(); + } + + private void operandsToSql(ToSqlOptions options, StringBuilder strBuilder) { strBuilder.append(operands_.get(0).getQueryStmt().toSql(options)); + + // If there is only one operand we simply print it without any mention of a set + // operator. It is only possible in a 'ValuesStmt' - otherwise it is syntactically + // impossible in SQL. + if (operands_.size() == 1) return; + for (int i = 1; i < operands_.size() - 1; ++i) { String opName = operands_.get(i).getSetOperator() != null ? operands_.get(i).getSetOperator().name() : @@ -525,6 +547,7 @@ public class SetOperationStmt extends QueryStmt { strBuilder.append(")"); } } + // Determine whether we need parentheses around the last union operand. SetOperand lastOperand = operands_.get(operands_.size() - 1); QueryStmt lastQueryStmt = lastOperand.getQueryStmt(); @@ -539,17 +562,6 @@ public class SetOperationStmt extends QueryStmt { } else { strBuilder.append(lastQueryStmt.toSql(options)); } - // Order By clause - if (hasOrderByClause()) { - strBuilder.append(" ORDER BY "); - for (int i = 0; i < orderByElements_.size(); ++i) { - strBuilder.append(orderByElements_.get(i).toSql(options)); - strBuilder.append((i + 1 != orderByElements_.size()) ? ", " : ""); - } - } - // Limit clause. - strBuilder.append(limitElement_.toSql(options)); - return strBuilder.toString(); } @Override diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java index d8b604809..833726784 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java @@ -3507,6 +3507,9 @@ public class AnalyzeStmtsTest extends AnalyzerTest { "partition (year, month) " + "values(1, true, 1, 1, 1, 1, cast(1.0 as float), cast(1.0 as double), " + "'a', 'a', cast(0 as timestamp), 2009, 10)"); + + assertEquals("SELECT 1", AnalyzesOk("values (1)").toSql(ToSqlOptions.REWRITTEN)); + // Values stmt with multiple rows. AnalyzesOk("values((1, 2, 3), (4, 5, 6))"); AnalyzesOk("select * from (values('a', 'b', 'c')) as t"); @@ -3531,6 +3534,9 @@ public class AnalyzeStmtsTest extends AnalyzerTest { "(3, true, 3, 3, 3, 3, cast(3.0 as float), cast(3.0 as double), " + "'c', 'c', cast(0 as timestamp), 2009, 3))"); + assertEquals("SELECT 1 UNION ALL SELECT 2", + AnalyzesOk("values (1), (2)").toSql(ToSqlOptions.REWRITTEN)); + // Test multiple aliases. Values() is like union, the column labels are 'x' and 'y'. AnalyzesOk("values((1 as x, 'a' as y), (2 as k, 'b' as j))"); // Test order by, offset and limit. diff --git a/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java b/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java index 15f34a534..9880db3b2 100644 --- a/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java @@ -493,8 +493,6 @@ public class ExprRewriterTest extends AnalyzerTest { + "values(" + data + ")"; String expectedToSql = "INSERT INTO TABLE " + "functional.alltypesnopart(" + columnName + ") " - + "SELECT CAST(" + data + " AS " + castColumn + ")" - + " UNION " + "SELECT CAST(" + data + " AS " + castColumn + ")"; assertToSqlWithImplicitCasts(ctx, query, expectedToSql); } diff --git a/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test b/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test index 554041c6a..ec49a1388 100644 --- a/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test +++ b/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test @@ -5978,7 +5978,7 @@ insert into functional_kudu.tinyinttable values(1); Max Per-Host Resource Reservation: Memory=0B Threads=1 Per-Host Resource Estimates: Memory=20MB Codegen disabled by planner -Analyzed query: SELECT CAST(1 AS TINYINT) UNION SELECT CAST(1 AS TINYINT) +Analyzed query: SELECT CAST(1 AS TINYINT) F00:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1 | Per-Host Resources: mem-estimate=20.00MB mem-reservation=0B thread-reservation=1 @@ -5995,7 +5995,7 @@ INSERT INTO KUDU [functional_kudu.tinyinttable] Max Per-Host Resource Reservation: Memory=2.00MB Threads=2 Per-Host Resource Estimates: Memory=22MB Codegen disabled by planner -Analyzed query: SELECT CAST(1 AS TINYINT) UNION SELECT CAST(1 AS TINYINT) +Analyzed query: SELECT CAST(1 AS TINYINT) F01:PLAN FRAGMENT [KUDU(KuduPartition(1))] hosts=1 instances=1 | Per-Host Resources: mem-estimate=22.02MB mem-reservation=2.00MB thread-reservation=1
