jdaugherty commented on code in PR #16505:
URL: https://github.com/apache/grails-core/pull/16505#discussion_r4178328800
##########
grails-data-hibernate7/core/src/main/groovy/grails/orm/HibernateCriteriaBuilder.java:
##########
@@ -1112,6 +1115,46 @@ public Criteria sizeLt(String propertyName, int size) {
return this;
}
+ /**
+ * Builds a junction, whose block {@link DetachedCriteria} resolves its
calls against, with
+ * {@code sqlRestriction} added to the detached criteria of the block and
of the association blocks inside it.
+ */
+ private void inJunction(Runnable junction) {
+ GroovyCategorySupport.use(SqlRestrictionCategory.class, new
Closure<Object>(this) {
Review Comment:
**[P1] The category scope invalidates every Groovy call site in the JVM, on
every junction.** In Groovy 5.1.3, `GroovyCategorySupport.use` calls
`VMPluginFactory.getPlugin().invalidateCallSites()` when it enters the scope
and again when it leaves it (`ThreadCategoryInfo.newScope` and `endScope`).
That replaces the global `SwitchPoint` that guards every indy call site. Only
the method lookup is limited to the current thread. So every Hibernate 7
criteria query with an `and`, `or` or `not` block now makes the call sites of
every thread relink, and deoptimizes the compiled code that inlined them,
whether or not it uses `sqlRestriction`.
Measured on JDK 21/H2:
- `IndyInterface.switchPoint` before and after `createCriteria().list { or {
eq('title', 'Alpha'); eq('title', 'Beta') } }` is invalidated at edc5f687cb and
still valid at the base 620d549caa. The same query without the `or` leaves it
valid on both.
- A Groovy method with 300 dynamic call sites, timed after each query
(median of 1,500 runs), takes about 60 µs after either query at the base. At
edc5f687cb it takes about 900 µs after the `or` query, and 150-170 µs after
plain queries once `or` queries have run.
The category's methods work unchanged as a Groovy extension module, which
Groovy loads once and which needs no scope. I tried registering
`SqlRestrictionCategory` in
`META-INF/groovy/org.codehaus.groovy.runtime.ExtensionModule` and building the
junctions directly again. All of `HibernateCriteriaBuilderSqlRestrictionSpec`
passed, including `or { and { chapters { ... } } }`, the switch point stayed
valid, and the 300 call sites took about 60 µs after both queries.
`grails-test-examples/hibernate7/criteria-extension` adds criteria methods the
same way.
One difference: `sqlRestriction` is then also callable on any detached
criteria, for example `new DetachedCriteria(Book).build { sqlRestriction(...)
}`, not only inside the builder's junctions. If you keep that, please document
and test it. Otherwise, any approach without a category scope works.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/PredicateGenerator.java:
##########
@@ -448,6 +453,38 @@ private Predicate handlePropertyCriterion(
throw new UnsupportedOperationException("Unsupported criterion: " +
pc.getClass().getName());
}
+ /**
+ * Returns a column of the entity's own table, whose table alias replaces
{@code {alias}} in a SQL restriction.
+ */
+ private static Expression<?> aliasColumn(From<?, ?> root,
GrailsHibernatePersistentEntity entity) {
+ PersistentProperty identity = entity.getIdentity();
+ if (identity == null) {
+ HibernatePersistentProperty[] compositeIdentity =
entity.getCompositeIdentity();
+ if (compositeIdentity == null || compositeIdentity.length == 0) {
+ throw new ConfigurationException("Cannot use sqlRestriction on
class [" + entity.getJavaClass().getName() + "] without an identifier");
+ }
+ identity = compositeIdentity[0];
+ }
+ Path<?> path = root.get(identity.getName());
+ if (identity instanceof Association<?> association &&
association.getAssociatedEntity() != null) {
+ path =
path.get(association.getAssociatedEntity().getIdentity().getName());
Review Comment:
Verified at edc5f687cb: the `Printing`/`Edition` mapping now works with and
without `{alias}`.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/SqlRestriction.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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
+ *
+ * https://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.grails.orm.hibernate.query;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.grails.datastore.mapping.query.Query;
+
+/**
+ * Criterion that restricts the results with a native SQL condition, created
by {@code sqlRestriction} in a
+ * criteria query. In the SQL, {@code {alias}} stands for the table alias of
the queried entity, and each
+ * {@code ?} for one of the values, which are bound as parameters.
+ *
+ * @param sql the SQL condition
+ * @param values the values of the {@code ?} placeholders, in order
+ * @since 8.0.0
+ */
+public record SqlRestriction(String sql, List<?> values) implements
Query.Criterion {
+
+ public SqlRestriction {
+ if (sql == null) {
+ throw new IllegalArgumentException("The SQL of a sqlRestriction
must not be null");
+ }
+ if (values == null) {
+ values = List.of();
+ }
+ List<Object> copy = new ArrayList<>(values.size());
+ for (Object value : values) {
+ if (value == null) {
+ throw new IllegalArgumentException("The values of a
sqlRestriction must not be null: " + sql);
+ }
+ copy.add(value instanceof CharSequence ? value.toString() : value);
+ }
+ long placeholders = sql.chars().filter(c -> c == '?').count();
+ if (placeholders != copy.size()) {
+ throw new IllegalArgumentException("The SQL of a sqlRestriction
has " + placeholders +
+ " ? placeholders but " + copy.size() + " values: " + sql);
Review Comment:
Verified at edc5f687cb: `{alias}.title like '%?' and {alias}.pages > ?` with
`[50]` now returns the matching book, through the shared scan in validation and
rendering.
--
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]
