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 f93ac89 [fix](lateral-view) fix bugs of lateral view with CTE or
where clause (#7865)
f93ac89 is described below
commit f93ac89a67765b7ced743ecaa1c511003ca4d1b6
Author: Mingyu Chen <morningman....@gmail.com>
AuthorDate: Fri Jan 28 22:24:23 2022 +0800
[fix](lateral-view) fix bugs of lateral view with CTE or where clause
(#7865)
fix bugs of lateral view with CTE or where clause.
The error case can be found in newly added tests in
`TableFunctionPlanTest.java`
But there are still some bugs not being fixed, so the unit test is
annotated with @Ignore
This PR contains the change is #7824 :
> Issue Number: close #7823
>
> After the subquery is rewritten, the rewritten stmt needs to be reset
> (that is, the content of the first analyze semantic analysis is cleared),
> and then the rewritten stmt can be reAnalyzed.
>
> The lateral view ref in the previous implementation forgot to implement
the reset function.
> This caused him to keep the first error message in the second analyze.
> Eventually, two duplicate tupleIds appear in the new stmt and are marked
with different tuple.
> From the explain string, the following syntax will have an additional
wrong join predicate.
> ```
> Query: explain select k1 from test_explode lateral view explode_split(k2,
",") tmp as e1 where k1 in (select k3 from tbl1);
> Error equal join conjunct: `k3` = `k3`
> ```
>
> This pr mainly adds the reset function of the lateral view
> to avoid possible errors in the second analyze
> when the lateral view and subquery rewrite occur at the same time.
---
.../org/apache/doris/analysis/LateralViewRef.java | 29 ++++++++++
.../java/org/apache/doris/analysis/SelectStmt.java | 11 +++-
.../java/org/apache/doris/analysis/TableRef.java | 65 +++++++++++++++++-----
.../doris/planner/TableFunctionPlanTest.java | 59 ++++++++++++++++++++
4 files changed, 147 insertions(+), 17 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/analysis/LateralViewRef.java
b/fe/fe-core/src/main/java/org/apache/doris/analysis/LateralViewRef.java
index 5736d23..bee7032 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/analysis/LateralViewRef.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/LateralViewRef.java
@@ -85,6 +85,11 @@ public class LateralViewRef extends TableRef {
isAnalyzed = true; // true now that we have assigned desc
}
+ @Override
+ public TableRef clone() {
+ return new LateralViewRef(this.expr.clone(), this.viewName,
this.columnName);
+ }
+
private void analyzeFunctionExpr(Analyzer analyzer) throws
AnalysisException {
fnExpr = (FunctionCallExpr) expr;
fnExpr.setTableFnCall(true);
@@ -178,5 +183,29 @@ public class LateralViewRef extends TableRef {
throw new AnalysisException("Subquery is not allowed in lateral
view");
}
}
+
+ @Override
+ public String toSql() {
+ return "lateral view " + fnExpr.toSql() + " " + viewName + " as " +
columnName;
+ }
+
+ @Override
+ public String toString() {
+ return toSql();
+ }
+
+ @Override
+ public void reset() {
+ isAnalyzed = false;
+ expr.reset();
+ fnExpr = null;
+ originSlotRefList = Lists.newArrayList();
+ view = null;
+ explodeSlotRef = null;
+ // There is no need to call the reset function of @relatedTableRef
here.
+ // The main reason is that @lateralViewRef itself is an attribute of
@relatedTableRef
+ // The reset of @lateralViewRef happens in the reset() of
@relatedTableRef.
+ }
}
+
diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java
b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java
index 22db67b..1c52f12 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/SelectStmt.java
@@ -438,7 +438,7 @@ public class SelectStmt extends QueryStmt {
if (groupByClause != null && groupByClause.isGroupByExtension()) {
for (SelectListItem item : selectList.getItems()) {
if (item.getExpr() instanceof FunctionCallExpr &&
item.getExpr().fn instanceof AggregateFunction) {
- for (Expr expr: groupByClause.getGroupingExprs()) {
+ for (Expr expr : groupByClause.getGroupingExprs()) {
if (item.getExpr().contains(expr)) {
throw new AnalysisException("column: " +
expr.toSql() + " cannot both in select list and "
+ "aggregate functions when using GROUPING
SETS/CUBE/ROLLUP, please use union"
@@ -877,6 +877,12 @@ public class SelectStmt extends QueryStmt {
expandStar(new TableName(tableRef.getAliasAsName().getDb(),
tableRef.getAliasAsName().getTbl()),
tableRef.getDesc());
+
+ if (tableRef.lateralViewRefs != null) {
+ for (LateralViewRef lateralViewRef : tableRef.lateralViewRefs)
{
+ expandStar(lateralViewRef.getName(),
lateralViewRef.getDesc());
+ }
+ }
}
}
@@ -1049,7 +1055,7 @@ public class SelectStmt extends QueryStmt {
groupByClause.analyze(analyzer);
createAggInfo(groupByClause.getGroupingExprs(), aggExprs,
analyzer);
} else {
- createAggInfo( new ArrayList<>(), aggExprs, analyzer);
+ createAggInfo(new ArrayList<>(), aggExprs, analyzer);
}
// combine avg smap with the one that produces the final agg output
@@ -1874,3 +1880,4 @@ public class SelectStmt extends QueryStmt {
return this.id.equals(((SelectStmt) obj).id);
}
}
+
diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/TableRef.java
b/fe/fe-core/src/main/java/org/apache/doris/analysis/TableRef.java
index 95f90f2..c0c39ed 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/analysis/TableRef.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/TableRef.java
@@ -99,9 +99,9 @@ public class TableRef implements ParseNode, Writable {
private boolean isForcePreAggOpened;
// ///////////////////////////////////////
// BEGIN: Members that need to be reset()
-
+
protected Expr onClause;
-
+
// the ref to the left of us, if we're part of a JOIN clause
protected TableRef leftTblRef;
@@ -133,7 +133,7 @@ public class TableRef implements ParseNode, Writable {
// END: Members that need to be reset()
// ///////////////////////////////////////
-
+
public TableRef() {
// for persist
}
@@ -161,6 +161,7 @@ public class TableRef implements ParseNode, Writable {
this.commonHints = commonHints;
isAnalyzed = false;
}
+
// Only used to clone
// this will reset all the 'analyzed' stuff
protected TableRef(TableRef other) {
@@ -187,7 +188,13 @@ public class TableRef implements ParseNode, Writable {
allMaterializedTupleIds_ =
Lists.newArrayList(other.allMaterializedTupleIds_);
correlatedTupleIds_ = Lists.newArrayList(other.correlatedTupleIds_);
desc = other.desc;
- lateralViewRefs = other.lateralViewRefs;
+ lateralViewRefs = null;
+ if (other.lateralViewRefs != null) {
+ lateralViewRefs = Lists.newArrayList();
+ for (LateralViewRef viewRef : other.lateralViewRefs) {
+ lateralViewRefs.add((LateralViewRef) viewRef.clone());
+ }
+ }
}
public PartitionNames getPartitionNames() {
@@ -279,13 +286,17 @@ public class TableRef implements ParseNode, Writable {
* Returns true if this table ref has a resolved path that is rooted at a
registered
* tuple descriptor, false otherwise.
*/
- public boolean isRelative() { return false; }
+ public boolean isRelative() {
+ return false;
+ }
/**
* Indicates if this TableRef directly or indirectly references another
TableRef from
* an outer query block.
*/
- public boolean isCorrelated() { return !correlatedTupleIds_.isEmpty(); }
+ public boolean isCorrelated() {
+ return !correlatedTupleIds_.isEmpty();
+ }
public Table getTable() {
return desc.getTable();
@@ -417,7 +428,7 @@ public class TableRef implements ParseNode, Writable {
* The join clause can only be analyzed after the left table has been
analyzed
* and the TupleDescriptor (desc) of this table has been created.
*/
- public void analyzeJoin(Analyzer analyzer) throws AnalysisException {
+ public void analyzeJoin(Analyzer analyzer) throws AnalysisException {
Preconditions.checkState(leftTblRef == null || leftTblRef.isAnalyzed);
Preconditions.checkState(desc != null);
analyzeJoinHints();
@@ -540,7 +551,7 @@ public class TableRef implements ParseNode, Writable {
// of the outer join; those can be evaluated directly when
materializing tuples
// without violating outer join semantics.
analyzer.registerOnClauseConjuncts(conjuncts, this);
- for (Expr e: conjuncts) {
+ for (Expr e : conjuncts) {
List<TupleId> tupleIds = Lists.newArrayList();
e.getIds(tupleIds, null);
onClauseTupleIds.addAll(tupleIds);
@@ -611,7 +622,16 @@ public class TableRef implements ParseNode, Writable {
// if (resolvedPath_ != null) path =
resolvedPath_.getFullyQualifiedRawPath();
// return ToSqlUtils.getPathSql(path) + ((aliasSql != null) ? " " +
aliasSql : "");
- return name.toSql() + ((aliasSql != null) ? " " + aliasSql : "");
+ // tbl1
+ // tbl1 alias_tbl1
+ // tbl1 alias_tbl1 lateral view explode_split(k1, ",") tmp1 as e1
+ String tblName = name.toSql() + ((aliasSql != null) ? " " + aliasSql :
"");
+ if (lateralViewRefs != null) {
+ for (LateralViewRef viewRef : lateralViewRefs) {
+ tblName += " " + viewRef.toSql();
+ }
+ }
+ return tblName;
}
@Override
@@ -652,21 +672,27 @@ public class TableRef implements ParseNode, Writable {
/**
* Returns all legal aliases of this table ref.
*/
- public String[] getAliases() { return aliases_; }
+ public String[] getAliases() {
+ return aliases_;
+ }
/**
* Returns the explicit alias or the fully-qualified implicit alias. The
returned alias
* is guaranteed to be unique (i.e., column/field references against the
alias cannot
* be ambiguous).
*/
- public String getUniqueAlias() { return aliases_[0]; }
+ public String getUniqueAlias() {
+ return aliases_[0];
+ }
/**
* Returns true if this table ref has an explicit alias.
* Note that getAliases().length() == 1 does not imply an explicit alias
because
* nested collection refs have only a single implicit alias.
*/
- public boolean hasExplicitAlias() { return hasExplicitAlias_; }
+ public boolean hasExplicitAlias() {
+ return hasExplicitAlias_;
+ }
/**
* Returns the explicit alias if this table ref has one, null otherwise.
@@ -676,7 +702,10 @@ public class TableRef implements ParseNode, Writable {
return null;
}
- public boolean isAnalyzed() { return isAnalyzed; }
+ public boolean isAnalyzed() {
+ return isAnalyzed;
+ }
+
public boolean isResolved() {
return !getClass().equals(TableRef.class);
}
@@ -722,6 +751,11 @@ public class TableRef implements ParseNode, Writable {
allMaterializedTupleIds_.clear();
correlatedTupleIds_.clear();
desc = null;
+ if (lateralViewRefs != null) {
+ for (LateralViewRef lateralViewRef : lateralViewRefs) {
+ lateralViewRef.reset();
+ }
+ }
}
/**
@@ -755,7 +789,7 @@ public class TableRef implements ParseNode, Writable {
out.writeBoolean(true);
partitionNames.write(out);
}
-
+
if (hasExplicitAlias()) {
out.writeBoolean(true);
Text.writeString(out, getExplicitAlias());
@@ -783,7 +817,8 @@ public class TableRef implements ParseNode, Writable {
if (in.readBoolean()) {
String alias = Text.readString(in);
- aliases_ = new String[] { alias };
+ aliases_ = new String[]{alias};
}
}
}
+
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/planner/TableFunctionPlanTest.java
b/fe/fe-core/src/test/java/org/apache/doris/planner/TableFunctionPlanTest.java
index 7e610dc..a2b26e4 100644
---
a/fe/fe-core/src/test/java/org/apache/doris/planner/TableFunctionPlanTest.java
+++
b/fe/fe-core/src/test/java/org/apache/doris/planner/TableFunctionPlanTest.java
@@ -19,6 +19,7 @@ package org.apache.doris.planner;
import org.apache.doris.analysis.CreateDbStmt;
import org.apache.doris.analysis.CreateTableStmt;
+import org.apache.doris.analysis.CreateViewStmt;
import org.apache.doris.analysis.FunctionCallExpr;
import org.apache.doris.catalog.Catalog;
import org.apache.doris.qe.ConnectContext;
@@ -28,6 +29,7 @@ import org.apache.commons.io.FileUtils;
import org.junit.After;
import org.junit.Assert;
import org.junit.BeforeClass;
+import org.junit.Ignore;
import org.junit.Test;
import java.io.File;
@@ -60,6 +62,11 @@ public class TableFunctionPlanTest {
+ "distributed by hash(k1) buckets 3
properties('replication_num' = '1');";
createTableStmt = (CreateTableStmt)
UtFrameUtils.parseAndAnalyzeStmt(createTblStmtStr, ctx);
Catalog.getCurrentCatalog().createTable(createTableStmt);
+
+ createTblStmtStr = "create table db1.table_for_view (k1 int, k2 int,
k3 varchar(100)) distributed by hash(k1)" +
+ "properties('replication_num' = '1');";
+ createTableStmt = (CreateTableStmt)
UtFrameUtils.parseAndAnalyzeStmt(createTblStmtStr, ctx);
+ Catalog.getCurrentCatalog().createTable(createTableStmt);
}
// test planner
@@ -441,6 +448,7 @@ public class TableFunctionPlanTest {
Assert.assertTrue(explainString.contains("table function:
explode_json_array_double('[1.1, 2.2, 3.3]')"));
Assert.assertTrue(explainString.contains("output slot id: 0 1"));
}
+
/*
Case4 agg and order column in the same stmt with lateral view
select min(c1) from (select k1 as c1, min(k2) as c2 from tbl1 group by k1)
tmp1
@@ -470,4 +478,55 @@ public class TableFunctionPlanTest {
Assert.assertTrue(explainString.contains("output slot id: 2"));
Assert.assertTrue(explainString.contains("tuple ids: 1 3"));
}
+
+ @Test
+ public void testLaterViewWithView() throws Exception {
+ // test 1
+ String createViewStr = "create view db1.v1 (k1,e1) as select k1,e1
from db1.table_for_view lateral view explode_split(k3,',') tmp as e1;";
+ CreateViewStmt createViewStmt = (CreateViewStmt)
UtFrameUtils.parseAndAnalyzeStmt(createViewStr, ctx);
+ Catalog.getCurrentCatalog().createView(createViewStmt);
+
+ String sql = "select * from db1.v1;";
+ String explainString = UtFrameUtils.getSQLPlanOrErrorMsg(ctx, sql,
true);
+ Assert.assertTrue(explainString.contains("output slot id: 1 2"));
+ // query again to see if it has error
+ explainString = UtFrameUtils.getSQLPlanOrErrorMsg(ctx, sql, true);
+ Assert.assertTrue(explainString.contains("output slot id: 1 2"));
+ }
+
+ @Test
+ public void testLaterViewWithWhere() throws Exception {
+ String sql = "select k1,e1 from db1.table_for_view lateral view
explode_split(k3,',') tmp as e1 where k1 in (select k2 from
db1.table_for_view);";
+ String explainString = UtFrameUtils.getSQLPlanOrErrorMsg(ctx, sql,
true);
+ Assert.assertTrue(explainString.contains("join op: LEFT SEMI JOIN
(BROADCAST)"));
+ Assert.assertTrue(explainString.contains("equal join conjunct: `k1` =
`k2`"));
+ Assert.assertTrue(!explainString.contains("equal join conjunct: `k2` =
`k2`"));
+ }
+
+ @Test
+ public void testLaterViewWithCTE() throws Exception {
+ String sql = "with tmp as (select k1,e1 from db1.table_for_view
lateral view explode_split(k3,',') tmp2 as e1) select * from tmp;";
+ String explainString = UtFrameUtils.getSQLPlanOrErrorMsg(ctx, sql,
true);
+ Assert.assertTrue(explainString.contains("table function:
explode_split(`default_cluster:db1`.`table_for_view`.`k3`, ',') "));
+ }
+
+ @Ignore
+ // errCode = 2, detailMessage = Unknown column 'e1' in 'table list'
+ public void testLaterViewWithCTEBug() throws Exception {
+ String sql = "with tmp as (select * from db1.table_for_view where
k2=1) select k1,e1 from tmp lateral view explode_split(k3,',') tmp2 as e1;";
+ String explainString = UtFrameUtils.getSQLPlanOrErrorMsg(ctx, sql,
true);
+ Assert.assertTrue(!explainString.contains("Unknown column 'e1' in
'table list'"));
+ }
+
+ @Ignore
+ // errCode = 2, detailMessage = Unknown column 'e1' in 'table list'
+ public void testLaterViewUnknowColumnBug() throws Exception {
+ // test2
+ String createViewStr = "create view db1.v2 (k1,k3) as select k1,k3
from db1.table_for_view;";
+ CreateViewStmt createViewStmt = (CreateViewStmt)
UtFrameUtils.parseAndAnalyzeStmt(createViewStr, ctx);
+ Catalog.getCurrentCatalog().createView(createViewStmt);
+ String sql = "select k1,e1 from db1.v2 lateral view
explode_split(k3,',') tmp as e1;";
+ String explainString = UtFrameUtils.getSQLPlanOrErrorMsg(ctx, sql,
true);
+ Assert.assertTrue(!explainString.contains("Unknown column 'e1' in
'table list'"));
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
