[
https://issues.apache.org/jira/browse/GROOVY-12724?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18143355#comment-18143355
]
ASF GitHub Bot commented on GROOVY-12724:
-----------------------------------------
blackdrag commented on code in PR #3259:
URL: https://github.com/apache/groovy/pull/3259#discussion_r4545521261
##########
src/main/java/org/codehaus/groovy/ast/tools/GenericsUtils.java:
##########
@@ -368,6 +368,89 @@ public static ClassNode erasure(final ClassNode type) {
return t.getPlainNodeReference();
}
+ /**
+ * Type arguments safe to store on {@code owner}. A class that declares no
+ * type parameters gets none. Each argument is a reference type.
+ *
+ * @param owner the parameterized type
+ * @param arguments the arguments about to be stored
+ * @return the arguments to store, or {@code null} when the class declares
none
+ */
+ public static GenericsType[] legalTypeArguments(final ClassNode owner,
final GenericsType[] arguments) {
+ if (arguments == null) return null;
+ if (arguments.length > 0 && declaresNoTypeParameters(owner)) return
null;
+ GenericsType[] legal = new GenericsType[arguments.length];
+ boolean changed = false;
+ for (int i = 0; i < arguments.length; i += 1) {
+ legal[i] = asReferenceTypeArgument(arguments[i]);
+ changed |= legal[i] != arguments[i];
+ }
+ return changed ? legal : arguments;
+ }
+
+ private static boolean declaresNoTypeParameters(final ClassNode type) {
+ if (type == null || type.isGenericsPlaceHolder()) return false;
+ ClassNode declared = type.redirect();
+ if (declared == null || !declared.isResolved() || declared.isArray())
return false;
Review Comment:
I think it is a problem if this code silently depends on if declared is
resolved or not.
##########
src/main/java/org/codehaus/groovy/ast/tools/GenericsUtils.java:
##########
@@ -368,6 +368,89 @@ public static ClassNode erasure(final ClassNode type) {
return t.getPlainNodeReference();
}
+ /**
+ * Type arguments safe to store on {@code owner}. A class that declares no
+ * type parameters gets none. Each argument is a reference type.
+ *
+ * @param owner the parameterized type
+ * @param arguments the arguments about to be stored
+ * @return the arguments to store, or {@code null} when the class declares
none
+ */
+ public static GenericsType[] legalTypeArguments(final ClassNode owner,
final GenericsType[] arguments) {
+ if (arguments == null) return null;
+ if (arguments.length > 0 && declaresNoTypeParameters(owner)) return
null;
+ GenericsType[] legal = new GenericsType[arguments.length];
+ boolean changed = false;
+ for (int i = 0; i < arguments.length; i += 1) {
+ legal[i] = asReferenceTypeArgument(arguments[i]);
+ changed |= legal[i] != arguments[i];
+ }
+ return changed ? legal : arguments;
+ }
+
+ private static boolean declaresNoTypeParameters(final ClassNode type) {
+ if (type == null || type.isGenericsPlaceHolder()) return false;
+ ClassNode declared = type.redirect();
Review Comment:
redirect is supposed to be always !=null
##########
src/main/java/org/codehaus/groovy/classgen/asm/TypeSignatureWriter.java:
##########
@@ -46,7 +47,47 @@ static void writeParameterizedClass(final StringBuilder ret,
final ClassNode pri
return;
}
ret.append(BytecodeHelper.getTypeDescription(printType, false));
- BytecodeHelper.addSubTypes(ret, printType.getGenericsTypes(), "<",
">");
+ BytecodeHelper.addSubTypes(ret, typeArguments(printType), "<", ">");
+ }
+
+ /**
+ * Type arguments written for {@code printType}. Last resort for JVMS
+ * 4.7.9.1: if a use site still names arguments a class does not declare,
+ * write the raw type javac would emit for the legal type. The type
+ * checker is what must keep {@code Serializable<T>} and {@code
Queryable<int>}
+ * out of the AST.
+ */
+ private static GenericsType[] typeArguments(final ClassNode printType) {
+ GenericsType[] useSite = printType.getGenericsTypes();
+ if (useSite == null || useSite.length == 0) {
+ return useSite;
+ }
+ int declared = declaredTypeParameters(printType);
+ if (declared >= 0 && declared != useSite.length) {
+ return null;
+ }
+ return useSite;
+ }
+
+ /**
+ * Number of type parameters the class actually declares, or {@code -1}
+ * when that is not known yet. An unresolved {@code java.*} name is loaded
+ * so a use site cannot attach arguments to {@code Serializable}.
+ */
+ private static int declaredTypeParameters(final ClassNode printType) {
+ ClassNode resolved = printType.isResolved() ? printType :
printType.redirect();
Review Comment:
we are here on the asm phase, ClassNodes are supposed to be resolved.
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/provider/collection/runtime/Queryable.java:
##########
@@ -516,6 +539,52 @@ default Queryable<T> union(Queryable<? extends T>
queryable) {
*/
List<T> toList();
+ /**
+ * Iterates the materialized elements. A static {@code for} loop calls this
+ * rather than wrapping the query as a single value.
+ *
+ * @return an iterator over the elements
+ * @since 7.0.0
+ */
+ default Iterator<T> iterator() {
+ return toList().iterator();
+ }
+
+ /**
+ * Coerces this query. {@link Queryable}, {@link List}, {@link Collection}
and
+ * {@link Iterable} are answered here. Every other target is delegated to
+ * {@link DefaultGroovyMethods#asType(Object, Class)}, which throws
+ * {@link GroovyCastException} when the value is not compatible with the
target.
+ *
+ * @param type the requested type
+ * @return the coerced value
+ * @throws GroovyCastException when {@code type} is null or the value
cannot be coerced
+ * @since 7.0.0
+ */
+ default Object asType(Class<?> type) {
+ if (type == null) {
+ throw new GroovyCastException("Cannot cast '" +
getClass().getName() + "' to null");
+ }
+ if (type == Queryable.class) {
+ return this;
Review Comment:
this looks not quite clean. I would handle only the toList case and
otherwise let DGM handle it.
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/provider/collection/runtime/QueryableHelper.groovy:
##########
@@ -111,6 +111,36 @@ class QueryableHelper {
return CompletableFuture.supplyAsync(() -> { function.apply(param) },
ThreadPoolHolder.THREAD_POOL)
}
+ /**
+ * Runs one clause body on the shared pool. The declared type is {@code
Object}
+ * so static inference never sees {@code CompletableFuture}: a parallel row
+ * was being typed as that future, and the generated bridge then cast the
row.
+ * The value returned to the runtime is still the future {@code select}
unwraps.
+ *
+ * @param supplier the clause body
+ * @return the asynchronous result
+ * @since 7.0.0
+ */
+ @GroovyABI(since = "7.0.0")
+ static Object supplyClause(Supplier<Object> supplier) {
+ return CompletableFuture.supplyAsync(supplier,
ThreadPoolHolder.THREAD_POOL)
Review Comment:
Here the question is of course about eating our own dog food... why not use
async/await?
##########
src/main/java/org/codehaus/groovy/classgen/asm/BytecodeHelper.java:
##########
@@ -493,19 +493,26 @@ static void addSubTypes(StringBuilder ret, GenericsType[]
types, String start, S
if (type.getType().isArray()) {
ret.append("[");
addSubTypes(ret, new GenericsType[]{new
GenericsType(type.getType().getComponentType())}, "", "");
+ } else if ("<".equals(start) && isPrimitiveType(type.getType()) &&
!isPrimitiveVoid(type.getType())) {
+ // Last resort (JVMS 4.7.9.1): a type argument that is still a
+ // primitive is written as its wrapper, which is what javac
writes
+ // for the legal reference type. Parameters and return types
use a
+ // different start marker and stay primitive.
+ writeGenericsBoundType(ret,
ClassHelper.getWrapper(type.getType()), false);
Review Comment:
First of all we are now mixing features. The issue was about GINQ, not
supporting "? extends int". Next is... why do we no do the wrapping when we
construct the GenericsType already? This kind of code in BytecodeHelper looks
wrong to me.
##########
subprojects/groovy-ginq/src/test/groovy/org/apache/groovy/ginq/GinqCompileStaticTest.groovy:
##########
@@ -0,0 +1,237 @@
+/*
+ * 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
+
+import groovy.ginq.transform.GQ
+import groovy.transform.CompileStatic
+import org.apache.groovy.ginq.provider.collection.runtime.Queryable
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.assertScript
+
+/**
+ * GINQ SQL-like queries compiled with {@code @CompileStatic}.
+ */
+@CompileStatic
+final class GinqCompileStaticTest {
+
+ private final int minimum = 18
+
+ @Test
+ void macroFiltersAndProjects() {
+ List<Integer> numbers = [1, 2, 3, 4]
+ assert [4, 6, 8] == GQ {
+ from n in numbers
+ where n > 1
+ select n * 2
+ }.toList()
+ }
+
+ @Test
+ void macroAsList() {
+ assert [2, 3] == GQL {
+ from n in [1, 2, 3]
+ where n > 1
+ select n
+ }
+ }
+
+ @Test
+ void readsFieldsAndLocals() {
+ int extra = 1
+ assert [18, 21] == GQ {
+ from n in [15, 18, 21]
+ where n >= minimum - extra
+ select n
+ }.toList()
+ }
+
+ @Test
+ void localIsReadWhenTheQueryIsBuilt() {
+ int extra = 1
+ Queryable query = GQ {
+ from n in [1, 2, 3, 4]
+ where n > extra
+ select n
+ }
+ extra = 3
+ // Locals are copied when the expression is evaluated. The copy is
what runs later.
+ assert [2, 3, 4] == query.toList()
+ }
+
+ @Test
+ void joinsProjectsAndGroups() {
+ List<Item> items = [new Item(1, 'Orange'), new Item(2, 'Apple')]
+ List<Sale> sales = [new Sale(1), new Sale(1), new Sale(2)]
+ assert ['Orange', 'Orange', 'Apple'] == GQ {
+ from s in sales
+ join i in items on i.id == s.item
+ select i.name
+ }.toList()
+
+ assert [[0, 6.0], [1, 4.0]] == GQ {
+ from n in [1, 2, 3, 4]
+ groupby n % 2 as parity
+ orderby parity
+ select parity, sum(n)
+ }.toList()
+ }
+
+ @Test
+ void windowRowNumber() {
+ assert [[1, 0], [2, 1], [3, 2]] == GQ {
+ from n in [1, 2, 3]
+ select n, (rowNumber() over(orderby n))
+ }.toList()
+ }
+
+ @Test
+ void nestedQueryAndMapRows() {
+ assert [3] == GQ {
+ from n in [1, 2, 3]
+ where n > (from m in [1, 2] select max(m))
+ select n
+ }.toList()
+
+ List<Map<String, Integer>> rows = [[a: 1, b: 10], [a: 3, b: 30]]
+ assert [30] == GQ {
+ from r in rows
+ where r.a > 1
+ select r.b
+ }.toList()
+ }
+
+ @Test
+ void queryInsideClosure() {
+ List<Integer> numbers = [1, 2, 3]
+ List found = []
+ numbers.each { Integer n ->
+ if (n == 1) {
+ found.addAll(GQ {
+ from m in numbers
+ where m >= n
+ select m
+ }.toList())
+ }
+ }
+ assert [1, 2, 3] == found
+ }
+
+ @Test
+ void orderByDescendingAndMembership() {
+ assert [3, 2, 1] == GQ {
+ from n in [1, 2, 3]
+ orderby n in desc
+ select n
+ }.toList()
+
+ assert [1, 3] == GQ {
+ from n in [1, 2, 3]
+ where n in [1, 3]
+ select n
+ }.toList()
+ }
+
+ @Test
+ void parallelClauseKeepsRowValues() {
+ assert [3, 2, 1] == GQ(parallel: true) {
+ from n in [1, 2, 3]
+ orderby n in desc
+ select n
+ }.toList()
+ }
+
+ @Test
+ void staticMethodAndAnnotation() {
+ assert [1, 2, 3] == staticNumbers()
+ assert [2, 3] == above([1, 2, 3], 2).toList()
+ assert [3, 4] == listed(2)
+ }
+
+ @Test
+ void compiledClassInAScript() {
+ assertScript '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ class Shop {
+ List names(List<String> items) {
+ GQ {
+ from item in items
+ where item.size() > 3
+ select item.toUpperCase()
+ }.toList()
+ }
+ }
+
+ assert ['PEAR'] == new Shop().names(['pear', 'fig'])
+ '''
+ }
+
+ @Test
+ void customAggregateFromTheDsl() {
+ assertScript '''
+ def result = GQ {
+ from n in [1, 2, 3]
+ groupby n % 2 as k
+ select total(n)
+ }.toList()
+ assert result.toSet() == [new BigDecimal('4'), new
BigDecimal('2')].toSet()
Review Comment:
actually this implies the result is a list of BigDecimals. Why should that
be the case? Also if that is the case why not [4.0, 2.0].toSet()?
##########
src/main/java/org/codehaus/groovy/classgen/asm/TypeSignatureWriter.java:
##########
@@ -46,7 +47,47 @@ static void writeParameterizedClass(final StringBuilder ret,
final ClassNode pri
return;
}
ret.append(BytecodeHelper.getTypeDescription(printType, false));
- BytecodeHelper.addSubTypes(ret, printType.getGenericsTypes(), "<",
">");
+ BytecodeHelper.addSubTypes(ret, typeArguments(printType), "<", ">");
+ }
+
+ /**
+ * Type arguments written for {@code printType}. Last resort for JVMS
+ * 4.7.9.1: if a use site still names arguments a class does not declare,
+ * write the raw type javac would emit for the legal type. The type
+ * checker is what must keep {@code Serializable<T>} and {@code
Queryable<int>}
+ * out of the AST.
+ */
+ private static GenericsType[] typeArguments(final ClassNode printType) {
+ GenericsType[] useSite = printType.getGenericsTypes();
+ if (useSite == null || useSite.length == 0) {
+ return useSite;
+ }
+ int declared = declaredTypeParameters(printType);
+ if (declared >= 0 && declared != useSite.length) {
+ return null;
+ }
+ return useSite;
+ }
+
+ /**
+ * Number of type parameters the class actually declares, or {@code -1}
+ * when that is not known yet. An unresolved {@code java.*} name is loaded
+ * so a use site cannot attach arguments to {@code Serializable}.
+ */
+ private static int declaredTypeParameters(final ClassNode printType) {
+ ClassNode resolved = printType.isResolved() ? printType :
printType.redirect();
+ if (resolved != null && resolved.isResolved() && !resolved.isArray()) {
+ return resolved.getTypeClass().getTypeParameters().length;
+ }
+ String name = printType.getName();
+ if (name != null && (name.startsWith("java.") ||
name.startsWith("javax.") || name.startsWith("groovy."))) {
+ try {
+ return Class.forName(name, false,
TypeSignatureWriter.class.getClassLoader()).getTypeParameters().length;
Review Comment:
This is a task for ResolveVisitor
##########
subprojects/groovy-ginq/src/main/groovy/org/apache/groovy/ginq/provider/collection/runtime/QueryableCollection.java:
##########
@@ -684,12 +690,128 @@ public long size() {
}
/**
- * Returns a reusable stream over the current elements.
+ * Returns a new stream over the current elements. Creating the stream
does not
+ * pull. An iterable source is read again by every stream. A one-shot
stream is
+ * pulled into a buffer as it is consumed, and a later call replays that
buffer
+ * instead of returning the stream that was already handed out.
*
- * @return the element stream
+ * @return a stream that has not been consumed
*/
@Override
public Stream<T> stream() {
+ writeLock.lock();
+ try {
+ if (sourceIterable != null) {
+ Stream<T> fresh = toStream(sourceIterable);
+ if (!fresh.isParallel() && isParallel()) {
+ fresh = fresh.parallel();
+ }
+ return fresh;
+ }
+ bindUpstream();
+ return StreamSupport.stream(ReplaySpliterator::new,
upstreamCharacteristics, false);
+ } finally {
+ writeLock.unlock();
+ }
+ }
+
+ /**
+ * Binds the one-shot source. The size is recorded before any element is
pulled,
+ * so a later {@code count} can use it without running projections.
+ */
+ private void bindUpstream() {
+ if (upstreamBound) {
+ return;
+ }
+ upstreamBound = true;
+ upstream = openStream().spliterator();
+ long exact = upstream.getExactSizeIfKnown();
+ upstreamExactSize = exact;
+ upstreamCharacteristics = Spliterator.ORDERED;
+ if (exact >= 0) {
+ upstreamCharacteristics |= Spliterator.SIZED |
Spliterator.SUBSIZED;
+ }
+ }
+
+ /**
+ * One cursor over a one-shot source. Elements are appended to {@link
#replay}
+ * as they are pulled, so another stream can read them from the start.
+ * A sized source reports that size, and {@code Stream.count} then does
not pull.
+ */
+ private final class ReplaySpliterator implements Spliterator<T> {
+ private int cursor;
+
+ @Override
+ public boolean tryAdvance(Consumer<? super T> action) {
+ writeLock.lock();
Review Comment:
that will potentially mean a lot of lick calls
> GINQ: support static compilation and a fluent query API
> -------------------------------------------------------
>
> Key: GROOVY-12724
> URL: https://issues.apache.org/jira/browse/GROOVY-12724
> Project: Groovy
> Issue Type: Improvement
> Components: GINQ
> Reporter: Daniel Sun
> Assignee: Daniel Sun
> Priority: Major
>
> Under @CompileStatic / @TypeChecked, GQ, GQL, and @GQ currently rely on
> dynamic lowering. This improvement makes those entry points usable from a
> statically checked scope while keeping the SQL-like syntax lowered to the
> in-memory collection runtime.
> h3. Goals
> * Allow GQ / GQL / @GQ from @CompileStatic and @TypeChecked without forcing
> the lowered tree through static checking at the expression site.
> * Defer pipeline construction with LazyQueryable until a terminal read
> (toList, count, etc.), copying locals at the expression site (same binding as
> dynamic GQ).
> * Add Ginq.from(...) as a deferred, generic fluent chain on the same runtime,
> with @ClosureParams so where/select/join/aggregate closures can infer element
> types where inference is sound.
> * Preserve default lazy/deferred evaluation and dynamic GQ behavior for map
> rows and other intentionally dynamic cases.
> h3. Non-goals / known limits
> * Stored untyped Closure variables still need a SAM target type for parameter
> inference (same as Java/C#).
> * Raw Query parameters/fields cannot recover a safe element type.
> * Names bound by from inside GQ / GQL / @GQ stay dynamic by design; callers
> that need checked element types use Ginq.from.
> h3. Related work
> GitHub PR: https://github.com/apache/groovy/pull/3259
> Stacked on GEP-26 SQL window work (PR #3167 / gep26-sql-window).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
