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).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
