You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by mo...@apache.org on 2020/04/24 13:34:52 UTC
[incubator-doris] branch master updated: Forbidden correlated
having clause (#3378)
This is an automated email from the ASF dual-hosted git repository.
morningman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-doris.git
The following commit(s) were added to refs/heads/master by this push:
new 07a9401 Forbidden correlated having clause (#3378)
07a9401 is described below
commit 07a9401f825d61e9c2a14b8776b4d0821e79627f
Author: EmmyMiao87 <52...@qq.com>
AuthorDate: Fri Apr 24 21:34:40 2020 +0800
Forbidden correlated having clause (#3378)
1. The correlated slot ref should be bound by the agg tuple of outer query.
However, the correlated having clause can not be analyzed correctly so the result is incorrect.
For example:
```
SELECT k1 FROM test GROUP BY k1
HAVING EXISTS(SELECT k1 FROM baseall GROUP BY k1 HAVING SUM(test.k1) = k1);
```
The correlated predicate is not executed.
2. The limit offset should also be rewritten when there is subquery in having clause.
For example:
```
select k1, count(*) cnt from test group by k1 having k1 in
(select k1 from baseall order by k1 limit 2) order by k1 limit 5 offset 3;
```
The new stmt should has a limit element with offset.
---
.../java/org/apache/doris/analysis/QueryStmt.java | 2 ++
.../java/org/apache/doris/analysis/SelectStmt.java | 26 ++++++++++++++++++++++
.../apache/doris/analysis/SetOperationStmt.java | 7 ++++++
.../org/apache/doris/analysis/StmtRewriter.java | 6 ++---
.../java/org/apache/doris/analysis/Subquery.java | 11 +++++++++
.../main/java/org/apache/doris/catalog/View.java | 2 +-
.../org/apache/doris/analysis/SelectStmtTest.java | 20 +++++++++++++----
.../java/org/apache/doris/utframe/DorisAssert.java | 16 ++++++++++---
8 files changed, 79 insertions(+), 11 deletions(-)
diff --git a/fe/src/main/java/org/apache/doris/analysis/QueryStmt.java b/fe/src/main/java/org/apache/doris/analysis/QueryStmt.java
index a54dd7b..467fd6b 100644
--- a/fe/src/main/java/org/apache/doris/analysis/QueryStmt.java
+++ b/fe/src/main/java/org/apache/doris/analysis/QueryStmt.java
@@ -437,6 +437,8 @@ public abstract class QueryStmt extends StatementBase {
*/
public abstract void collectTableRefs(List<TableRef> tblRefs);
+ abstract List<TupleId> collectTupleIds();
+
public ArrayList<OrderByElement> getOrderByElements() {
return orderByElements;
}
diff --git a/fe/src/main/java/org/apache/doris/analysis/SelectStmt.java b/fe/src/main/java/org/apache/doris/analysis/SelectStmt.java
index 39adcdf..d8c3e1f 100644
--- a/fe/src/main/java/org/apache/doris/analysis/SelectStmt.java
+++ b/fe/src/main/java/org/apache/doris/analysis/SelectStmt.java
@@ -495,6 +495,20 @@ public class SelectStmt extends QueryStmt {
return result;
}
+ @Override
+ public List<TupleId> collectTupleIds() {
+ List<TupleId> result = Lists.newArrayList();
+ resultExprs.stream().forEach(expr -> expr.getIds(result, null));
+ result.addAll(getTableRefIds());
+ if (whereClause != null) {
+ whereClause.getIds(result, null);
+ }
+ if (havingClauseAfterAnaylzed != null) {
+ havingClauseAfterAnaylzed.getIds(result, null);
+ }
+ return result;
+ }
+
private void whereClauseRewrite() {
Expr deDuplicatedWhere = deduplicateOrs(whereClause);
if (deDuplicatedWhere != null) {
@@ -1077,6 +1091,18 @@ public class SelectStmt extends QueryStmt {
LOG.debug("desctbl: " + analyzer.getDescTbl().debugString());
LOG.debug("resultexprs: " + Expr.debugString(resultExprs));
}
+
+ if (havingClauseAfterAnaylzed != null) {
+ // forbidden correlated subquery in having clause
+ List<Subquery> subqueryInHaving = Lists.newArrayList();
+ havingClauseAfterAnaylzed.collect(Subquery.class, subqueryInHaving);
+ for (Subquery subquery : subqueryInHaving) {
+ if (subquery.isCorrelatedPredicate(getTableRefIds())) {
+ throw new AnalysisException("The correlated having clause is not supported");
+ }
+ }
+ }
+
/*
* All of columns of result and having clause are replaced by new slot ref which is bound by top tuple of agg info.
* For example:
diff --git a/fe/src/main/java/org/apache/doris/analysis/SetOperationStmt.java b/fe/src/main/java/org/apache/doris/analysis/SetOperationStmt.java
index 01c4d58..f03cecc 100644
--- a/fe/src/main/java/org/apache/doris/analysis/SetOperationStmt.java
+++ b/fe/src/main/java/org/apache/doris/analysis/SetOperationStmt.java
@@ -542,6 +542,13 @@ public class SetOperationStmt extends QueryStmt {
}
@Override
+ public List<TupleId> collectTupleIds() {
+ List<TupleId> result = Lists.newArrayList();
+ for (SetOperand op: operands) result.addAll(op.getQueryStmt().collectTupleIds());
+ return result;
+ }
+
+ @Override
public String toSql() {
if (toSqlString != null) {
return toSqlString;
diff --git a/fe/src/main/java/org/apache/doris/analysis/StmtRewriter.java b/fe/src/main/java/org/apache/doris/analysis/StmtRewriter.java
index 29636a9..05674f5 100644
--- a/fe/src/main/java/org/apache/doris/analysis/StmtRewriter.java
+++ b/fe/src/main/java/org/apache/doris/analysis/StmtRewriter.java
@@ -161,7 +161,7 @@ public class StmtRewriter {
Preconditions.checkState(havingClause != null);
Preconditions.checkState(havingClause.getSubquery() != null);
List<OrderByElement> orderByElements = stmt.getOrderByElementsAfterAnalyzed();
- long limit = stmt.getLimit();
+ LimitElement limitElement = new LimitElement(stmt.getOffset(), stmt.getLimit());
TableAliasGenerator tableAliasGenerator = stmt.getTableAliasGenerator();
/*
@@ -260,7 +260,7 @@ public class StmtRewriter {
newTableRefList.add(inlineViewRef);
FromClause newFromClause = new FromClause(newTableRefList);
SelectStmt result = new SelectStmt(newSelectList, newFromClause, newWherePredicate, null, null,
- newOrderByElements, new LimitElement(limit));
+ newOrderByElements, limitElement);
result.setTableAliasGenerator(tableAliasGenerator);
try {
result.analyze(analyzer);
@@ -514,7 +514,7 @@ public class StmtRewriter {
&& ((BinaryPredicate) conjunct).getOp() == BinaryPredicate.Operator.EQ) {
return;
}
- throw new AnalysisException("scalar subquery's unCorrelatedPredicates's operator must be EQ");
+ throw new AnalysisException("scalar subquery's correlatedPredicates's operator must be EQ");
}
}
diff --git a/fe/src/main/java/org/apache/doris/analysis/Subquery.java b/fe/src/main/java/org/apache/doris/analysis/Subquery.java
index 5dc0ccd..d36d3ca 100644
--- a/fe/src/main/java/org/apache/doris/analysis/Subquery.java
+++ b/fe/src/main/java/org/apache/doris/analysis/Subquery.java
@@ -158,6 +158,17 @@ public class Subquery extends Expr {
return new StructType(structFields);
}
+ @Override
+ public boolean isCorrelatedPredicate(List<TupleId> tupleIdList) {
+ List<TupleId> tupleIdFromSubquery = stmt.collectTupleIds();
+ for (TupleId tupleId : tupleIdList) {
+ if (tupleIdFromSubquery.contains(tupleId)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
/**
* Returns true if the toSql() of the Subqueries is identical. May return false for
* equivalent statements even due to minor syntactic differences like parenthesis.
diff --git a/fe/src/main/java/org/apache/doris/catalog/View.java b/fe/src/main/java/org/apache/doris/catalog/View.java
index b090bad..24a7c6a 100644
--- a/fe/src/main/java/org/apache/doris/catalog/View.java
+++ b/fe/src/main/java/org/apache/doris/catalog/View.java
@@ -157,7 +157,7 @@ public class View extends Table {
node = (ParseNode) SqlParserUtils.getFirstStmt(parser);
} catch (Exception e) {
LOG.info("stmt is {}", inlineViewDef);
- LOG.info("exception because: {}", e);
+ LOG.info("exception because: ", e);
LOG.info("msg is {}", inlineViewDef);
// Do not pass e as the exception cause because it might reveal the existence
// of tables that the user triggering this load may not have privileges on.
diff --git a/fe/src/test/java/org/apache/doris/analysis/SelectStmtTest.java b/fe/src/test/java/org/apache/doris/analysis/SelectStmtTest.java
index 92db5b1..f58776c 100644
--- a/fe/src/test/java/org/apache/doris/analysis/SelectStmtTest.java
+++ b/fe/src/test/java/org/apache/doris/analysis/SelectStmtTest.java
@@ -19,10 +19,9 @@ package org.apache.doris.analysis;
import org.apache.doris.common.AnalysisException;
import org.apache.doris.qe.ConnectContext;
-import org.apache.doris.rewrite.ExprRewriter;
-import org.apache.doris.thrift.TPrimitiveType;
import org.apache.doris.utframe.DorisAssert;
import org.apache.doris.utframe.UtFrameUtils;
+
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
@@ -30,7 +29,6 @@ import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
-import java.io.IOException;
import java.util.UUID;
public class SelectStmtTest {
@@ -50,9 +48,11 @@ public class SelectStmtTest {
UtFrameUtils.createMinDorisCluster(runningDir);
String createTblStmtStr = "create table db1.tbl1(k1 varchar(32), k2 varchar(32), k3 varchar(32), k4 int) "
+ "AGGREGATE KEY(k1, k2,k3,k4) distributed by hash(k1) buckets 3 properties('replication_num' = '1');";
+ String createBaseAllStmtStr = "create table db1.baseall(k1 int) distributed by hash(k1) "
+ + "buckets 3 properties('replication_num' = '1');";
dorisAssert = new DorisAssert();
dorisAssert.withDatabase("db1").useDatabase("db1");
- dorisAssert.withTable(createTblStmtStr);
+ dorisAssert.withTable(createTblStmtStr).withTable(createBaseAllStmtStr);
}
@Test
@@ -283,4 +283,16 @@ public class SelectStmtTest {
Assert.assertTrue(stmt8.toSql().contains("((`t2`.`k1` IS NOT NULL) AND (`t1`.`k1` IS NOT NULL))" +
" AND (`t1`.`k1` IS NOT NULL)"));
}
+
+ @Test
+ public void testForbiddenCorrelatedSubqueryInHavingClause() throws Exception {
+ String sql = "SELECT k1 FROM baseall GROUP BY k1 HAVING EXISTS(SELECT k4 FROM tbl1 GROUP BY k4 HAVING SUM"
+ + "(baseall.k1) = k4);";
+ try {
+ dorisAssert.query(sql).explainQuery();
+ Assert.fail("The correlated subquery in having clause should be forbidden.");
+ } catch (AnalysisException e) {
+ System.out.println(e.getMessage());
+ }
+ }
}
diff --git a/fe/src/test/java/org/apache/doris/utframe/DorisAssert.java b/fe/src/test/java/org/apache/doris/utframe/DorisAssert.java
index ed7dd11..b808fec 100644
--- a/fe/src/test/java/org/apache/doris/utframe/DorisAssert.java
+++ b/fe/src/test/java/org/apache/doris/utframe/DorisAssert.java
@@ -18,18 +18,18 @@
package org.apache.doris.utframe;
import org.apache.doris.alter.AlterJobV2;
-import org.apache.doris.analysis.AlterClause;
import org.apache.doris.analysis.AlterTableStmt;
import org.apache.doris.analysis.CreateDbStmt;
import org.apache.doris.analysis.CreateMaterializedViewStmt;
import org.apache.doris.analysis.CreateTableStmt;
import org.apache.doris.analysis.DropTableStmt;
-import org.apache.doris.analysis.InsertStmt;
import org.apache.doris.catalog.Catalog;
import org.apache.doris.cluster.ClusterNamespace;
+import org.apache.doris.common.AnalysisException;
import org.apache.doris.common.Config;
import org.apache.doris.planner.Planner;
import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.QueryState;
import org.apache.doris.qe.StmtExecutor;
import org.apache.doris.system.SystemInfoService;
import org.apache.doris.thrift.TExplainLevel;
@@ -140,9 +140,19 @@ public class DorisAssert {
Assert.assertFalse(explainQuery().contains(s));
}
- private String explainQuery() throws Exception {
+ public String explainQuery() throws Exception {
StmtExecutor stmtExecutor = new StmtExecutor(connectContext, "explain " + sql);
stmtExecutor.execute();
+ QueryState queryState = connectContext.getState();
+ if (queryState.getStateType() == QueryState.MysqlStateType.ERR) {
+ switch (queryState.getErrType()){
+ case ANALYSIS_ERR:
+ throw new AnalysisException(queryState.getErrorMessage());
+ case OTHER_ERR:
+ default:
+ throw new Exception(queryState.getErrorMessage());
+ }
+ }
Planner planner = stmtExecutor.planner();
String explainString = planner.getExplainString(planner.getFragments(), TExplainLevel.VERBOSE);
System.out.println(explainString);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org