Copilot commented on code in PR #2458:
URL: https://github.com/apache/groovy/pull/2458#discussion_r3067845637
##########
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:
`getGinqExpression` can throw a NullPointerException when a set operation is
started but the right-hand query is missing (e.g. the script ends after
`union`). In that situation `latestGinqExpression` is null but the code still
calls `latestGinqExpression.putNodeMetaData(...)`. Consider short-circuiting
when `latestGinqExpression` is null (and reporting a dedicated syntax error
like “right-hand query for set operation is missing”).
##########
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;
Review Comment:
When parsing the first set operator, `setOperationLeft =
latestGinqExpression` does not mark that left query as `ROOT_GINQ_EXPRESSION`.
As a result, in `GinqAstWalker.visitGinqExpression` the left query won’t be
treated as “root” and won’t enable window-function/parallel scaffolding even if
it uses `over(...)` or parallel config. Consider setting
`latestGinqExpression.putNodeMetaData(ROOT_GINQ_EXPRESSION,
latestGinqExpression)` before assigning it to `setOperationLeft` (and similarly
ensure each top-level query in a set-op chain is tagged).
--
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]
