You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kgyrtkirk (via GitHub)" <gi...@apache.org> on 2024/07/16 13:01:00 UTC
[PR] HAVING clauses may not contain window functions (druid)
kgyrtkirk opened a new pull request, #16742:
URL: https://github.com/apache/druid/pull/16742
Rejects having clauses if they contain windowed expressions. (most likely it will be fixed via [CALCITE-6473](https://issues.apache.org/jira/browse/CALCITE-6473))
Also added a check to produce a more descriptive error if an
OVER expression reaches the filter translation layer.
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
Fixes #XXXX.
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
#### Fixed the bug ...
#### Renamed the class ...
#### Added a forbidden-apis entry ...
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Key changed/added classes in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "rohangarg (via GitHub)" <gi...@apache.org>.
rohangarg commented on code in PR #16742:
URL: https://github.com/apache/druid/pull/16742#discussion_r1691131168
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java:
##########
@@ -238,6 +241,10 @@ public static DruidExpression toDruidExpressionWithPostAggOperands(
final SqlKind kind = rexNode.getKind();
if (kind == SqlKind.INPUT_REF) {
return inputRefToDruidExpression(rowSignature, rexNode);
+ } else if (rexNode instanceof RexOver) {
+ throw new CannotBuildQueryException(
+ StringUtils.format("Encountered an OVER during Filter translation [%s].", rexNode)
Review Comment:
Agreed that it will fail for sure, what I meant to point was that I'm unsure about the error message `Encountered an OVER during Filter translation` - it doesn't seem that only the filter translation code flow goes through this.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "asdf2014 (via GitHub)" <gi...@apache.org>.
asdf2014 commented on code in PR #16742:
URL: https://github.com/apache/druid/pull/16742#discussion_r1680611171
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,44 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate) {
Review Comment:
To fix check style check failures:
```java
Error: /home/runner/work/druid/druid/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:898:5: '}' at column 5 should be alone on a line. [RightCurly]
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,44 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate) {
+ try {
+ SqlVisitor<Void> visitor =
+ new SqlBasicVisitor<Void>() {
+ @Override public Void visit(SqlCall call) {
Review Comment:
To fix check style check failures:
```java
Error: /home/runner/work/druid/druid/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:889:55: '{' at column 55 should be on a new line. [LeftCurly]
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,44 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate) {
+ try {
+ SqlVisitor<Void> visitor =
+ new SqlBasicVisitor<Void>() {
+ @Override public Void visit(SqlCall call) {
+ if (callPredicate.test(call)) {
+ throw new Util.FoundOne(call);
+ }
+ return super.visit(call);
+ }
+ };
+ node.accept(visitor);
+ return false;
+ } catch (Util.FoundOne e) {
Review Comment:
To fix check style check failures:
```java
Error: /home/runner/work/druid/druid/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:898:5: '}' at column 5 should be alone on a line. [RightCurly]
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "rohangarg (via GitHub)" <gi...@apache.org>.
rohangarg commented on code in PR #16742:
URL: https://github.com/apache/druid/pull/16742#discussion_r1687991446
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,50 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ if (having == null) {
+ return false;
+ }
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate)
+ {
+ try {
+ SqlVisitor<Void> visitor =
Review Comment:
is it not possible to make this a boolean visitor? `true` indicates presence, and `false` and `null` indicating non-presence?
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java:
##########
@@ -238,6 +241,10 @@ public static DruidExpression toDruidExpressionWithPostAggOperands(
final SqlKind kind = rexNode.getKind();
if (kind == SqlKind.INPUT_REF) {
return inputRefToDruidExpression(rowSignature, rexNode);
+ } else if (rexNode instanceof RexOver) {
+ throw new CannotBuildQueryException(
+ StringUtils.format("Encountered an OVER during Filter translation [%s].", rexNode)
Review Comment:
I am not sure whether this code gets exercised only in during filter translation. Seems to be used for other things as well.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "rohangarg (via GitHub)" <gi...@apache.org>.
rohangarg merged PR #16742:
URL: https://github.com/apache/druid/pull/16742
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #16742:
URL: https://github.com/apache/druid/pull/16742#discussion_r1688096724
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,50 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ if (having == null) {
+ return false;
+ }
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate)
+ {
+ try {
+ SqlVisitor<Void> visitor =
Review Comment:
I'm not sure if that would be beneficial as this is just a copy of `SqlUtil#containsCall` until we upgrade to a version which has the fix - after that it should be removed
at the time of the upgrade `CalciteSelectQueryTest#testRejectHavingWithWindowExpression` will fail and suggests to remove these changes
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #16742:
URL: https://github.com/apache/druid/pull/16742#discussion_r1688106495
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java:
##########
@@ -238,6 +241,10 @@ public static DruidExpression toDruidExpressionWithPostAggOperands(
final SqlKind kind = rexNode.getKind();
if (kind == SqlKind.INPUT_REF) {
return inputRefToDruidExpression(rowSignature, rexNode);
+ } else if (rexNode instanceof RexOver) {
+ throw new CannotBuildQueryException(
+ StringUtils.format("Encountered an OVER during Filter translation [%s].", rexNode)
Review Comment:
if there is a `RexOver` at this point that will fail for sure - it used to fail with some cryptic aggregation error
<details>
<summary>example exception when it fails without this</summary>
```
java.lang.RuntimeException: SQL query requires 'SUM' operator that is not supported.
at org.apache.druid.sql.calcite.expression.ExpressionTestHelper.testExpression(ExpressionTestHelper.java:332)
at org.apache.druid.sql.calcite.expression.ExpressionTestHelper.testExpression(ExpressionTestHelper.java:320)
at org.apache.druid.sql.calcite.expression.ExpressionsTest.testPresenceOfOverIsInvalid(ExpressionsTest.java:2853)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:728)
at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:147)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:86)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$7(TestMethodTestDescriptor.java:218)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:214)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:139)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:69)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:151)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:147)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:90)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:55)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:102)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:54)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:95)
at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:91)
at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:60)
at org.eclipse.jdt.internal.junit5.runner.JUnit5TestReference.run(JUnit5TestReference.java:98)
at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:529)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:756)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)
```
</details>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #16742:
URL: https://github.com/apache/druid/pull/16742#discussion_r1688096724
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,50 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ if (having == null) {
+ return false;
+ }
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate)
+ {
+ try {
+ SqlVisitor<Void> visitor =
Review Comment:
I'm not sure if that would be beneficial as this is just a copy of `SqlUtil#containsCall` until we upgrade to a version which has the fix - after that it should be 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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "writer-jill (via GitHub)" <gi...@apache.org>.
writer-jill commented on PR #16742:
URL: https://github.com/apache/druid/pull/16742#issuecomment-2343495086
@sreemanamala this came up in the review of the release notes for Druid 31. Can you please write a user-facing release note for this change?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on PR #16742:
URL: https://github.com/apache/druid/pull/16742#issuecomment-2232873432
thank you for updating the PR @asdf2014 ; I must have forgot to format those lines
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "rohangarg (via GitHub)" <gi...@apache.org>.
rohangarg commented on code in PR #16742:
URL: https://github.com/apache/druid/pull/16742#discussion_r1691132175
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,50 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ if (having == null) {
+ return false;
+ }
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate)
+ {
+ try {
+ SqlVisitor<Void> visitor =
Review Comment:
Ah ok, that makes sense then - thanks for the clarification.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] HAVING clauses may not contain window functions (druid)
Posted by "asdf2014 (via GitHub)" <gi...@apache.org>.
asdf2014 commented on code in PR #16742:
URL: https://github.com/apache/druid/pull/16742#discussion_r1680501194
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,44 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate) {
+ try {
+ SqlVisitor<Void> visitor =
+ new SqlBasicVisitor<Void>() {
+ @Override public Void visit(SqlCall call) {
+ if (callPredicate.test(call)) {
+ throw new Util.FoundOne(call);
+ }
+ return super.visit(call);
+ }
+ };
+ node.accept(visitor);
+ return false;
+ } catch (Util.FoundOne e) {
Review Comment:
```suggestion
}
catch (Util.FoundOne e) {
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,44 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate) {
+ try {
+ SqlVisitor<Void> visitor =
+ new SqlBasicVisitor<Void>() {
+ @Override public Void visit(SqlCall call) {
Review Comment:
```suggestion
@Override public Void visit(SqlCall call)
{
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -857,4 +861,44 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx)
}
return src;
}
+
+ @Override
+ protected void validateHavingClause(SqlSelect select)
+ {
+ super.validateHavingClause(select);
+ SqlNode having = select.getHaving();
+ if (containsOver(having)) {
+ throw buildCalciteContextException("Window functions are not allowed in HAVING", having);
+ }
+ }
+
+ private boolean containsOver(SqlNode having)
+ {
+ final Predicate<SqlCall> callPredicate = call -> call.getOperator() instanceof SqlOverOperator;
+ return containsCall(having, callPredicate);
+ }
+
+ // copy of SqlUtil#containsCall
+ /** Returns whether an AST tree contains a call that matches a given
+ * predicate. */
+ private static boolean containsCall(SqlNode node,
+ Predicate<SqlCall> callPredicate) {
Review Comment:
```suggestion
Predicate<SqlCall> callPredicate)
{
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org