Copilot commented on code in PR #2453:
URL: https://github.com/apache/groovy/pull/2453#discussion_r3061593061
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/provider/collection/GinqAstWalker.groovy:
##########
@@ -1015,6 +1033,9 @@ class GinqAstWalker implements
GinqAstVisitor<Expression>, SyntaxErrorReportable
}
private void validateGroupCols(List<Expression> expressionList) {
+ if (groupByIntoAlias) {
+ return // In into-mode, access is through the alias; validation
handled by the type system
+ }
if (groupByVisited) {
Review Comment:
In into-mode this method now returns early, so `select` expressions are no
longer validated against the `groupby` columns. Because the generated lambdas
use `def` parameters, Groovy’s type system won’t reliably catch accidental
references to the original datasource alias (or other non-grouped fields),
which previously produced a clear syntax error. Consider adding a dedicated
validation path for `groupby...into` (e.g., reject references to datasource
aliases / `_g` and require access via the into-alias) instead of skipping
validation entirely.
```suggestion
private void validateGroupColsInIntoMode(Expression expression) {
new
ListExpression(Collections.singletonList(expression)).transformExpression(new
ExpressionTransformer() {
@Override
Expression transform(Expression expr) {
if (isExpression(expr, VariableExpression,
PropertyExpression)) {
def text = expr instanceof PropertyExpression ?
expr.propertyAsString : expr.text
if (text && Character.isUpperCase(text.charAt(0))) {
return expr
}
def rootObjectExpression =
findRootObjectExpression(expr).text
if (rootObjectExpression in getAliasNameList() || '_g'
== rootObjectExpression) {
collectSyntaxError(new GinqSyntaxError(
"`${expr.text}` is not available after
`groupby ... into`; use the into alias instead",
expr.getLineNumber(), expr.getColumnNumber()
))
}
} else if (isExpression(expr, AbstractGinqExpression)) {
collectSyntaxError(new GinqSyntaxError(
"sub-query could not be used in the `select`
clause with `groupby`",
expr.getLineNumber(), expr.getColumnNumber()
))
}
return expr.transformExpression(this)
}
})
}
private void validateGroupCols(List<Expression> expressionList) {
if (groupByVisited) {
if (groupByIntoAlias) {
for (Expression expression : expressionList) {
validateGroupColsInIntoMode(expression)
}
return
}
```
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/provider/collection/runtime/GroupResultImpl.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.ginq.provider.collection.runtime;
+
+import groovy.lang.Tuple;
+
+import java.io.Serial;
+
+/**
+ * Default implementation of {@link GroupResult}.
+ *
+ * @param <K> the type of the group key
+ * @param <T> the type of the grouped elements
+ * @since 5.0.0
+ */
+class GroupResultImpl<K, T> extends QueryableCollection<T> implements
GroupResult<K, T> {
+ @Serial private static final long serialVersionUID = -4637595210702145661L;
+
+ private final K key;
+
+ GroupResultImpl(K key, Queryable<T> group) {
+ super(group.toList());
+ this.key = key;
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public K getKey() {
+ // For single-key groupby, the classifier wraps the key in a
NamedRecord;
+ // unwrap it so g.key returns the raw value rather than a
single-element tuple
+ if (key instanceof Tuple && ((Tuple<?>) key).size() == 1) {
+ return (K) ((Tuple<?>) key).get(0);
Review Comment:
`getKey()` unwraps any single-element `Tuple` key. This will change
semantics for callers of the new `groupByInto` API who intentionally use a
`Tuple` (size 1) as the grouping key (e.g., classifier returns
`Tuple.tuple(x)`), because they will receive the raw element instead of the
tuple. Consider narrowing the unwrap check to the GINQ-internal key wrapper
type (e.g., `NamedRecord`/`NamedTuple`) rather than all `Tuple` instances.
```suggestion
// unwrap only that internal wrapper so user-supplied single-element
tuples
// keep their tuple semantics when returned from getKey()
if (key instanceof NamedRecord && ((NamedRecord<?, ?>) key).size()
== 1) {
return (K) ((NamedRecord<?, ?>) key).get(0);
```
--
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]
