[
https://issues.apache.org/jira/browse/GROOVY-11915?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18072469#comment-18072469
]
ASF GitHub Bot commented on GROOVY-11915:
-----------------------------------------
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);
```
> GINQ: Add groupby...into with first-class GroupResult type
> ----------------------------------------------------------
>
> Key: GROOVY-11915
> URL: https://issues.apache.org/jira/browse/GROOVY-11915
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> For GINQ, introduce an optional "into <alias>" clause for groupby that binds
> a GroupResult<K, T> variable (extending Queryable<T> with a key property).
> When used, this would replace the current magic aggregate function rewriting
> with real method calls on a first-class type, enabling composable group
> queries (e.g. {{g.where(...).toList()}}), simplifying the correctVars logic
> in GinqAstWalker, and making the backend Queryable API directly usable for
> grouped queries.
> The existing groupby syntax (without "into") would continue to work unchanged.
> If we ever provided a GINQ SQL provider, GroupResult maps cleanly to SQL
> GROUP BY + aggregate functions.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
