[
https://issues.apache.org/jira/browse/GROOVY-11919?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18072821#comment-18072821
]
ASF GitHub Bot commented on GROOVY-11919:
-----------------------------------------
Copilot commented on code in PR #2458:
URL: https://github.com/apache/groovy/pull/2458#discussion_r3067840300
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/dsl/GinqAstBuilder.java:
##########
@@ -420,6 +433,27 @@ public void visitBinaryExpression(BinaryExpression
expression) {
@Override
public void visitVariableExpression(VariableExpression expression) {
+ if (SET_OP_SET.contains(expression.getText())) {
+ if (latestGinqExpression == null) {
+ this.collectSyntaxError(new GinqSyntaxError(
+ "`" + expression.getText() + "` must follow a complete
GINQ expression (from...select)",
+ expression.getLineNumber(),
expression.getColumnNumber()));
+ } else {
+ if (setOperationLeft != null) {
+ // chaining: wrap previous left + op + current right into
a SetOperationExpression
+ latestGinqExpression.putNodeMetaData(ROOT_GINQ_EXPRESSION,
latestGinqExpression);
+ SetOperationExpression prev = new
SetOperationExpression(setOperationLeft, setOperationOp, latestGinqExpression);
+ prev.setSourcePosition(setOperationLeft);
+ setOperationLeft = prev;
+ } else {
+ setOperationLeft = latestGinqExpression;
+ }
+ setOperationOp = expression.getText();
+ latestGinqExpression = null;
+ }
+ return;
+ }
Review Comment:
Set operations are tracked by stashing `setOperationLeft`/`setOperationOp`
and clearing `latestGinqExpression`, but the combined `SetOperationExpression`
is only materialized later in `getGinqExpression(ASTNode)` at the end of
`buildAST`. This means set operations won’t work correctly in nested-query
contexts that consume `latestGinqExpression` during the AST walk (e.g.
`(...).exists()`, `x in (...)`, casts/argument lists): those sites will see
only the *rightmost* `from...select` and the prior `union/intersect/minus` will
be lost. To fix this, construct and assign a combined `AbstractGinqExpression`
as soon as the RHS `select` is completed (and store the latest nested
expression as `AbstractGinqExpression`, not just `GinqExpression`), or
otherwise scope pending set-op state per nesting level so nested replacements
use the full set-op tree.
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/dsl/GinqAstVisitor.java:
##########
@@ -51,6 +52,7 @@ public interface GinqAstVisitor<R> {
R visitOrderExpression(OrderExpression orderExpression);
R visitLimitExpression(LimitExpression limitExpression);
R visitSelectExpression(SelectExpression selectExpression);
+ R visitSetOperationExpression(SetOperationExpression
setOperationExpression);
R visitShutdownExpression(ShutdownExpression shutdownExpression);
Review Comment:
`SetOperationExpression` is imported/used here, but there is no
`org.apache.groovy.ginq.dsl.expression.SetOperationExpression` class in the
source tree (the `dsl/expression` package currently has no such file). This
will break compilation; add the missing AST node class (with `left`,
`operation`, `right`, and `accept(...)`/visitor dispatch) or adjust the
imports/usages to reference the correct existing type.
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/dsl/GinqAstBuilder.java:
##########
@@ -31,6 +31,7 @@
import org.apache.groovy.ginq.dsl.expression.OnExpression;
import org.apache.groovy.ginq.dsl.expression.OrderExpression;
import org.apache.groovy.ginq.dsl.expression.SelectExpression;
+import org.apache.groovy.ginq.dsl.expression.SetOperationExpression;
Review Comment:
`SetOperationExpression` is referenced but the corresponding class does not
exist in `org.apache.groovy.ginq.dsl.expression` (no file in that package
defines it). This will prevent `GinqAstBuilder` from compiling; ensure the new
AST node type is added to `dsl/expression` and wired into the visitor pattern.
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/dsl/GinqAstBuilder.java:
##########
@@ -110,30 +113,40 @@ public AbstractGinqExpression buildAST(ASTNode astNode) {
return getGinqExpression(astNode);
}
- private GinqExpression getGinqExpression(ASTNode astNode) {
+ private AbstractGinqExpression getGinqExpression(ASTNode astNode) {
if (null == latestGinqExpression) {
ASTNode node = ginqExpressionStack.isEmpty() ? astNode :
ginqExpressionStack.peek();
this.collectSyntaxError(new GinqSyntaxError("`select` clause is
missing",
node.getLineNumber(), node.getColumnNumber()));
}
- latestGinqExpression.visit(new GinqAstBaseVisitor() {
+ AbstractGinqExpression result;
+ if (setOperationLeft != null && latestGinqExpression != null) {
+ latestGinqExpression.putNodeMetaData(ROOT_GINQ_EXPRESSION,
latestGinqExpression);
+ SetOperationExpression setOpExpr = new
SetOperationExpression(setOperationLeft, setOperationOp, latestGinqExpression);
+ setOpExpr.setSourcePosition(setOperationLeft);
+ result = setOpExpr;
+ } else {
+ latestGinqExpression.putNodeMetaData(ROOT_GINQ_EXPRESSION,
latestGinqExpression);
+ result = latestGinqExpression;
+ }
Review Comment:
When building a `SetOperationExpression`, the `ROOT_GINQ_EXPRESSION`
metadata is only set on `latestGinqExpression` (the right-hand query) and not
on the left side (or the overall set-op node). `GinqAstWalker` uses this
metadata to enable window-function support and `parallel` setup/teardown, so
the left operand of a set operation will currently be treated as non-root and
may break queries that use `over(...)` or `parallel` on the left side. Consider
marking *all* operand `GinqExpression`s in the set-op tree as roots (or
alternatively teaching the walker to treat the set-op root as the root
context). Also note that returning `SetOperationExpression` as the root means
`transformGinqCode` will skip `GinqAstOptimizer` (it only optimizes when the
root is a `GinqExpression`), so set-op queries won’t be optimized unless the
optimizer path is updated to traverse set-op nodes.
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/provider/collection/GinqAstWalker.groovy:
##########
@@ -36,6 +36,7 @@ import org.apache.groovy.ginq.dsl.expression.LimitExpression
import org.apache.groovy.ginq.dsl.expression.OnExpression
import org.apache.groovy.ginq.dsl.expression.OrderExpression
import org.apache.groovy.ginq.dsl.expression.SelectExpression
+import org.apache.groovy.ginq.dsl.expression.SetOperationExpression
import org.apache.groovy.ginq.dsl.expression.ShutdownExpression
import org.apache.groovy.ginq.dsl.expression.WhereExpression
Review Comment:
`SetOperationExpression` is imported/used here, but the
`org.apache.groovy.ginq.dsl.expression.SetOperationExpression` class is not
present in the repository. This will cause compilation failures for the
collection provider; add the missing AST node class under `dsl/expression` (and
ensure it supports the visitor API).
> Provide set operators for GINQ, e.g. union, intersect
> -----------------------------------------------------
>
> Key: GROOVY-11919
> URL: https://issues.apache.org/jira/browse/GROOVY-11919
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
