You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/04/22 07:03:47 UTC
[GitHub] [incubator-doris] EmmyMiao87 opened a new pull request #3378: Forbidden correlated having clause
EmmyMiao87 opened a new pull request #3378:
URL: https://github.com/apache/incubator-doris/pull/3378
Fixed #3377
The correlated slot ref should be bound by the agg tuple of outer query.
However, the correlated having clause should 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.
The limit offset should be rewritten also 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.
The view which has subquery in having clause should not be created.
The reason is that the toSql function of query stmt is incorrect when there is subquery in having clause.
So the view def sql is incorrect and could not pass the sql parser.
Now, the view def sql use the original sql directly to fix this error.
Change-Id: If24206a8dee94013225fe7fdd30ab1fb034de34f
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on a change in pull request #3378: Forbidden correlated having clause
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #3378:
URL: https://github.com/apache/incubator-doris/pull/3378#discussion_r413053666
##########
File path: fe/src/main/java/org/apache/doris/analysis/LimitElement.java
##########
@@ -42,9 +42,9 @@ public LimitElement(long limit) {
offset = 0;
}
- public LimitElement(long offset, long limit) {
- this.offset = offset;
+ public LimitElement(long limit, long offset) {
Review comment:
Why change the order?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] xy720 commented on a change in pull request #3378: Forbidden correlated having clause
Posted by GitBox <gi...@apache.org>.
xy720 commented on a change in pull request #3378:
URL: https://github.com/apache/incubator-doris/pull/3378#discussion_r412851569
##########
File path: fe/src/main/java/org/apache/doris/analysis/BaseViewStmt.java
##########
@@ -109,33 +112,37 @@ protected void createColumnAndViewDefs(Analyzer analyzer) throws AnalysisExcepti
}
// format view def string
- originalViewDef = viewDefStmt.toSql();
+ String lowerCaseOriginSql = getOriginSql().toLowerCase();
+
+ originalViewDef = lowerCaseOriginSql.substring(lowerCaseOriginSql.indexOf(KW_AS) + 3);
+ LOG.debug("origin view def sql is {}", originalViewDef);
if (cols == null) {
inlineViewDef = originalViewDef;
+ LOG.debug("inline view def sql is {}", inlineViewDef);
return;
}
Analyzer tmpAnalyzer = new Analyzer(analyzer);
List<String> colNames = cols.stream().map(c -> c.getColName()).collect(Collectors.toList());
cloneStmt.substituteSelectList(tmpAnalyzer, colNames);
- inlineViewDef = cloneStmt.toSql();
-
-// StringBuilder sb = new StringBuilder();
-// sb.append("SELECT ");
-// for (int i = 0; i < finalCols.size(); ++i) {
-// if (i != 0) {
-// sb.append(", ");
-// }
-// String colRef = viewDefStmt.getColLabels().get(i);
-// if (!colRef.startsWith("`")) {
-// colRef = "`" + colRef + "`";
-// }
-// String colAlias = finalCols.get(i).getName();
-// sb.append(String.format("`%s`.%s AS `%s`", tableName.getTbl(), colRef, colAlias));
-// }
-// sb.append(String.format(" FROM (%s) %s", originalViewDef, tableName.getTbl()));
-// inlineViewDef = sb.toString();
+
+ StringBuilder sb = new StringBuilder();
+ sb.append("SELECT ");
+ for (int i = 0; i < colNames.size(); ++i) {
+ if (i != 0) {
+ sb.append(", ");
+ }
+ String colRef = viewDefStmt.getColLabels().get(i);
+ if (!colRef.startsWith("`")) {
+ colRef = "`" + colRef + "`";
+ }
+ String colAlias = colNames.get(i);
+ sb.append(String.format("`%s`.%s AS `%s`", tableName.getTbl(), colRef, colAlias));
Review comment:
It is work for function call?
For example, if user input:
"create view test_view (h1, h2) as select function(a, b), col2 from testTbl"
will become
"select function(a, b) as h1, col2 as h2 from (select function(a, b), col2 from testTbl)"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on issue #3378: Forbidden correlated having clause
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on issue #3378:
URL: https://github.com/apache/incubator-doris/pull/3378#issuecomment-618182114
> `The reason is that the toSql function of query stmt is incorrect when there is subquery in having clause.`
>
> @EmmyMiao87 Hi, why can't we make the toSql function correct ?
OK, I see.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] EmmyMiao87 commented on a change in pull request #3378: Forbidden correlated having clause
Posted by GitBox <gi...@apache.org>.
EmmyMiao87 commented on a change in pull request #3378:
URL: https://github.com/apache/incubator-doris/pull/3378#discussion_r413742575
##########
File path: fe/src/main/java/org/apache/doris/analysis/LimitElement.java
##########
@@ -42,9 +42,9 @@ public LimitElement(long limit) {
offset = 0;
}
- public LimitElement(long offset, long limit) {
- this.offset = offset;
+ public LimitElement(long limit, long offset) {
Review comment:
changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on a change in pull request #3378: Forbidden correlated having clause
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #3378:
URL: https://github.com/apache/incubator-doris/pull/3378#discussion_r413054209
##########
File path: fe/src/main/java/org/apache/doris/analysis/SelectList.java
##########
@@ -98,6 +98,24 @@ public void rewriteExprs(ExprRewriter rewriter, Analyzer analyzer)
item.setExpr(rewriter.rewrite(item.getExpr(), analyzer));
}
}
+
+ public String toOriginSql() {
+ // Select list
+ StringBuilder strBuilder = new StringBuilder();
+ strBuilder.append("SELECT ");
+ if (isDistinct) {
+ strBuilder.append("DISTINCT ");
+ }
+ for (int i = 0; i < items.size(); ++i) {
+ // strBuilder.append(selectList.getItems().get(i).toSql());
Review comment:
Remove.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on a change in pull request #3378: Forbidden correlated having clause
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #3378:
URL: https://github.com/apache/incubator-doris/pull/3378#discussion_r413053666
##########
File path: fe/src/main/java/org/apache/doris/analysis/LimitElement.java
##########
@@ -42,9 +42,9 @@ public LimitElement(long limit) {
offset = 0;
}
- public LimitElement(long offset, long limit) {
- this.offset = offset;
+ public LimitElement(long limit, long offset) {
Review comment:
Why do we change the order?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] EmmyMiao87 commented on a change in pull request #3378: Forbidden correlated having clause
Posted by GitBox <gi...@apache.org>.
EmmyMiao87 commented on a change in pull request #3378:
URL: https://github.com/apache/incubator-doris/pull/3378#discussion_r413742426
##########
File path: fe/src/main/java/org/apache/doris/analysis/BaseViewStmt.java
##########
@@ -109,33 +112,37 @@ protected void createColumnAndViewDefs(Analyzer analyzer) throws AnalysisExcepti
}
// format view def string
- originalViewDef = viewDefStmt.toSql();
+ String lowerCaseOriginSql = getOriginSql().toLowerCase();
+
+ originalViewDef = lowerCaseOriginSql.substring(lowerCaseOriginSql.indexOf(KW_AS) + 3);
+ LOG.debug("origin view def sql is {}", originalViewDef);
if (cols == null) {
inlineViewDef = originalViewDef;
+ LOG.debug("inline view def sql is {}", inlineViewDef);
return;
}
Analyzer tmpAnalyzer = new Analyzer(analyzer);
List<String> colNames = cols.stream().map(c -> c.getColName()).collect(Collectors.toList());
cloneStmt.substituteSelectList(tmpAnalyzer, colNames);
- inlineViewDef = cloneStmt.toSql();
-
-// StringBuilder sb = new StringBuilder();
-// sb.append("SELECT ");
-// for (int i = 0; i < finalCols.size(); ++i) {
-// if (i != 0) {
-// sb.append(", ");
-// }
-// String colRef = viewDefStmt.getColLabels().get(i);
-// if (!colRef.startsWith("`")) {
-// colRef = "`" + colRef + "`";
-// }
-// String colAlias = finalCols.get(i).getName();
-// sb.append(String.format("`%s`.%s AS `%s`", tableName.getTbl(), colRef, colAlias));
-// }
-// sb.append(String.format(" FROM (%s) %s", originalViewDef, tableName.getTbl()));
-// inlineViewDef = sb.toString();
+
+ StringBuilder sb = new StringBuilder();
+ sb.append("SELECT ");
+ for (int i = 0; i < colNames.size(); ++i) {
+ if (i != 0) {
+ sb.append(", ");
+ }
+ String colRef = viewDefStmt.getColLabels().get(i);
+ if (!colRef.startsWith("`")) {
+ colRef = "`" + colRef + "`";
+ }
+ String colAlias = colNames.get(i);
+ sb.append(String.format("`%s`.%s AS `%s`", tableName.getTbl(), colRef, colAlias));
Review comment:
This change has been removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on issue #3378: Forbidden correlated having clause
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on issue #3378:
URL: https://github.com/apache/incubator-doris/pull/3378#issuecomment-617831817
`The reason is that the toSql function of query stmt is incorrect when there is subquery in having clause.`
@EmmyMiao87 Hi, why can't we make the toSql function correct ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request #3378: Forbidden correlated having clause
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3378:
URL: https://github.com/apache/incubator-doris/pull/3378#discussion_r413008206
##########
File path: fe/src/main/java/org/apache/doris/analysis/BaseViewStmt.java
##########
@@ -109,33 +112,37 @@ protected void createColumnAndViewDefs(Analyzer analyzer) throws AnalysisExcepti
}
// format view def string
- originalViewDef = viewDefStmt.toSql();
+ String lowerCaseOriginSql = getOriginSql().toLowerCase();
Review comment:
I think you can not just call `toLowerCase()`, what if the statement is
```
select * from tbl where k1 like "%ABC%";
```
And why not just save the entire origin statement with `CREATE VIEW AS` inside it? And you can just get the `QueryStmt` part when parsing it again.
##########
File path: fe/src/main/java/org/apache/doris/qe/ConnectProcessor.java
##########
@@ -225,7 +226,13 @@ private void handleQuery() {
SqlScanner input = new SqlScanner(new StringReader(originStmt), ctx.getSessionVariable().getSqlMode());
SqlParser parser = new SqlParser(input);
try {
- return SqlParserUtils.getMultiStmts(parser);
+ List<StatementBase> result = SqlParserUtils.getMultiStmts(parser);
+ String[] originStmtList = originStmt.split(";");
Review comment:
There maybe `;` inside the query statement
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org