Skip to content

HHH-19750, HHH-19753 Fix criteria value-bind parameter and SqmFunction equals and hashing logic #10874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 5, 2025
Merged

HHH-19750, HHH-19753 Fix criteria value-bind parameter and SqmFunction equals and hashing logic #10874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18b984a
HHH-19750 Add test for issue
yrodiere Sep 3, 2025
1ab0e77
HHH-19750 Consider value-bind criteria parameters as instance-unique
mbellade Sep 4, 2025
4338f62
HHH-19753 Correct equals/hashCode for `SqmFunction` and subtypes
mbellade Sep 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions ...ore/src/main/java/org/hibernate/query/sqm/function/SelfRenderingSqmAggregateFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;

import org.hibernate.metamodel.model.domain.ReturnableType;
import org.hibernate.query.sqm.NodeBuilder;
Expand Down Expand Up @@ -123,4 +124,24 @@ public void appendHqlString(StringBuilder hql, SqmRenderContext context) {
hql.append( ')' );
}
}

@Override
public boolean equals(Object o) {
if ( o == null || getClass() != o.getClass() ) {
return false;
}
if ( !super.equals( o ) ) {
return false;
}

SelfRenderingSqmAggregateFunction<?> that = (SelfRenderingSqmAggregateFunction<?>) o;
return Objects.equals( filter, that.filter );
}

@Override
public int hashCode() {
int result = super.hashCode();
result = 31 * result + Objects.hashCode( filter );
return result;
}
}
13 changes: 0 additions & 13 deletions hibernate-core/src/main/java/org/hibernate/query/sqm/function/SelfRenderingSqmFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.function.Supplier;

import org.hibernate.metamodel.mapping.BasicValuedMapping;
Expand Down Expand Up @@ -255,16 +254,4 @@ public MappingModelExpressible<?> get() {
return argumentTypeResolver.resolveFunctionArgumentType( function.getArguments(), argumentIndex, converter );
}
}

@Override
// TODO: override on all subtypes
public boolean equals(Object other) {
return other instanceof SelfRenderingSqmAggregateFunction<?> that
&& Objects.equals( this.toHqlString(), that.toHqlString() );
}

@Override
public int hashCode() {
return toHqlString().hashCode();
}
}
21 changes: 21 additions & 0 deletions ...in/java/org/hibernate/query/sqm/function/SelfRenderingSqmOrderedSetAggregateFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Objects;

import org.hibernate.metamodel.model.domain.ReturnableType;
import org.hibernate.query.sqm.NodeBuilder;
Expand Down Expand Up @@ -173,4 +174,24 @@ public void appendHqlString(StringBuilder hql, SqmRenderContext context) {
hql.append( ')' );
}
}

@Override
public boolean equals(Object o) {
if ( o == null || getClass() != o.getClass() ) {
return false;
}
if ( !super.equals( o ) ) {
return false;
}

SelfRenderingSqmOrderedSetAggregateFunction<?> that = (SelfRenderingSqmOrderedSetAggregateFunction<?>) o;
return Objects.equals( withinGroup, that.withinGroup );
}

@Override
public int hashCode() {
int result = super.hashCode();
result = 31 * result + Objects.hashCode( withinGroup );
return result;
}
}
25 changes: 25 additions & 0 deletions ...e-core/src/main/java/org/hibernate/query/sqm/function/SelfRenderingSqmWindowFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;

import org.hibernate.metamodel.model.domain.ReturnableType;
import org.hibernate.query.sqm.NodeBuilder;
Expand Down Expand Up @@ -157,4 +158,28 @@ public void appendHqlString(StringBuilder hql, SqmRenderContext context) {
hql.append( ')' );
}
}

@Override
public boolean equals(Object o) {
if ( o == null || getClass() != o.getClass() ) {
return false;
}
if ( !super.equals( o ) ) {
return false;
}

SelfRenderingSqmWindowFunction<?> that = (SelfRenderingSqmWindowFunction<?>) o;
return Objects.equals( filter, that.filter )
&& Objects.equals( respectNulls, that.respectNulls )
&& Objects.equals( fromFirst, that.fromFirst );
}

@Override
public int hashCode() {
int result = super.hashCode();
result = 31 * result + Objects.hashCode( filter );
result = 31 * result + Objects.hashCode( respectNulls );
result = 31 * result + Objects.hashCode( fromFirst );
return result;
}
}
11 changes: 9 additions & 2 deletions hibernate-core/src/main/java/org/hibernate/query/sqm/tree/expression/SqmFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,15 @@ public SqmPath<?> resolveIndexedAccess(

@Override
public boolean equals(Object other) {
return other instanceof SqmFunction<?> that
&& Objects.equals( this.functionName, that.functionName )
if ( this == other ) {
return true;
}
if ( other == null || getClass() != other.getClass() ) {
return false;
}

final SqmFunction<?> that = (SqmFunction<?>) other;
return Objects.equals( this.functionName, that.functionName )
&& Objects.equals( this.arguments, that.arguments );
Comment on lines -209 to 218
Copy link
Member

@gavinking gavinking Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this change?

IMO, if the function name and the arguments are the same, it's the exact same function invocation.

The only case where that's not the case is with "fancy" aggregate functions (window functions and ordered set aggregates), and they are, I believe, all subclasses of SelfRenderingSqmFunction which overrides this implementation of equals().

So I believe this implementation is already correct.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the class check? There are other SqmFunction subtypes, other than window and ordered set aggregate, that might have the same name / arguments but be of different type - I know semantically it wouldn't make sense to have the same function name for differently typed functions, but the instance type check is very easy to perform and generally a good practice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other SqmFunction subtypes, other than window and ordered set aggregate, that might have the same name / arguments but be of different type

Yes but that shouldn't matter. If the name and arguments of the function are the same, it's the same function.

What I'm saying is: I don't think we ever need to look at expression types when comparing two SQM trees for equality. Equality is sufficiently determined by structure, and by identifier names. Types are inferred from that information.

Copy link
Member Author

@mbellade mbellade Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps mine was an excess of caution, in principle your implementation should work fine. To be clear, I was explaining the reasoning behind my change, but I'm ok with switching back to the old implementation, I won't push back on this.

Copy link
Member

@gavinking gavinking Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really not a big deal. I'm much more concerned about the ValueBindJpaCriteriaParameter issue.

}

Expand Down
23 changes: 3 additions & 20 deletions .../src/main/java/org/hibernate/query/sqm/tree/expression/ValueBindJpaCriteriaParameter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import org.hibernate.query.sqm.tree.SqmCopyContext;
import org.hibernate.query.sqm.tree.SqmRenderContext;

import java.util.Objects;


/**
Expand Down Expand Up @@ -55,33 +54,17 @@ public void appendHqlString(StringBuilder hql, SqmRenderContext context) {
}

@Override
// TODO: fix this
public int compareTo(SqmParameter<T> parameter) {
return this == parameter ? 0 : 1;
return Integer.compare( hashCode(), parameter.hashCode() );
}

// this is not really a parameter, it's really a literal value
// so use value equality based on its value

@Override
public boolean equals(Object object) {
return object instanceof ValueBindJpaCriteriaParameter<?> that
&& Objects.equals( this.value, that.value );
// && getJavaTypeDescriptor().areEqual( this.value, (T) that.value );
return this == object;
}

@Override
public int hashCode() {
return value == null ? 0 : value.hashCode(); // getJavaTypeDescriptor().extractHashCode( value );
return super.hashCode();
Comment on lines 56 to +68
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach is strictly-speaking correct, but I believe it results in an unacceptable number of cache misses.

Please see the discussion in https://hibernate.atlassian.net/browse/HHH-19556.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, the same discussion was brought up again in https://hibernate.atlassian.net/browse/HHH-19753 - but it's a matter of correctness: the value-bind parameters are bound by instance-identity, and their type is inferred based on their use-sites, so considering two parameters with the same value the same was not correct (other than leading to the error reported in https://hibernate.atlassian.net/browse/HHH-19750).

Copy link
Member

@gavinking gavinking Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said to you on Zulip, I'm pretty sure that if everything else in the query is identical except for, say, the type of a VBJCP, then they really are the same query. (For the purposes of query interpretation caching.)

That is to say, I don't see how you could have two different queries which are identical in every other respect except for the VBJCP.

I think the issue actually arises from how they're used as map keys in JdbcParameterBindingsImpl, and maybe other places you pointed out in Zulip.

So really the problem is that we're trying to use the same equals()/hashCode() for comparing SQM tree equality as we use for comparing parameter equality in JdbcParameterBindingsImpl. And it seems to me that maybe the easiest fix to that is to use something else to compare parameters in JdbcParameterBindingsImpl.

But that's no more than a guess/suggestion. I have not tried it, of course.

}

// @Override
// public boolean equals(Object object) {
// return this == object;
// }
//
// @Override
// public int hashCode() {
// return System.identityHashCode( this );
// }
}
146 changes: 96 additions & 50 deletions ...core/src/test/java/org/hibernate/orm/test/jpa/query/CriteriaUpdateWithParametersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,81 +4,113 @@
*/
package org.hibernate.orm.test.jpa.query;

import org.hibernate.testing.orm.junit.EntityManagerFactoryScope;
import org.hibernate.testing.orm.junit.Jpa;
import org.junit.jupiter.api.Test;

import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
import jakarta.persistence.Id;
import jakarta.persistence.Lob;
import jakarta.persistence.Query;
import jakarta.persistence.criteria.CriteriaBuilder;
import jakarta.persistence.criteria.CriteriaUpdate;
import jakarta.persistence.criteria.Expression;
import jakarta.persistence.criteria.ParameterExpression;
import jakarta.persistence.criteria.Path;
import jakarta.persistence.criteria.Root;
import jakarta.persistence.metamodel.EntityType;
import org.hibernate.query.criteria.HibernateCriteriaBuilder;
import org.hibernate.testing.orm.junit.EntityManagerFactoryScope;
import org.hibernate.testing.orm.junit.Jpa;
import org.junit.jupiter.api.Test;

@Jpa(
annotatedClasses = CriteriaUpdateWithParametersTest.Person.class
)

@Jpa(annotatedClasses = {
CriteriaUpdateWithParametersTest.Person.class,
CriteriaUpdateWithParametersTest.Process.class
})
public class CriteriaUpdateWithParametersTest {

@Test
public void testCriteriaUpdate(EntityManagerFactoryScope scope) {
scope.inTransaction(
entityManager -> {
final CriteriaBuilder criteriaBuilder = entityManager.getCriteriaBuilder();
final CriteriaUpdate<Person> criteriaUpdate = criteriaBuilder.createCriteriaUpdate( Person.class );
final Root<Person> root = criteriaUpdate.from( Person.class );

final ParameterExpression<Integer> intValueParameter = criteriaBuilder.parameter( Integer.class );
final ParameterExpression<String> stringValueParameter = criteriaBuilder.parameter( String.class );

final EntityType<Person> personEntityType = entityManager.getMetamodel().entity( Person.class );

criteriaUpdate.set(
root.get( personEntityType.getSingularAttribute( "age", Integer.class ) ),
intValueParameter
);
criteriaUpdate.where( criteriaBuilder.equal(
root.get( personEntityType.getSingularAttribute( "name", String.class ) ),
stringValueParameter
) );

final Query query = entityManager.createQuery( criteriaUpdate );
query.setParameter( intValueParameter, 9 );
query.setParameter( stringValueParameter, "Luigi" );

query.executeUpdate();
}
);
scope.inTransaction( entityManager -> {
final CriteriaBuilder criteriaBuilder = entityManager.getCriteriaBuilder();
final CriteriaUpdate<Person> criteriaUpdate = criteriaBuilder.createCriteriaUpdate( Person.class );
final Root<Person> root = criteriaUpdate.from( Person.class );

final ParameterExpression<Integer> intValueParameter = criteriaBuilder.parameter( Integer.class );
final ParameterExpression<String> stringValueParameter = criteriaBuilder.parameter( String.class );

final EntityType<Person> personEntityType = entityManager.getMetamodel().entity( Person.class );

criteriaUpdate.set( root.get( personEntityType.getSingularAttribute( "age", Integer.class ) ),
intValueParameter );
criteriaUpdate.where(
criteriaBuilder.equal( root.get( personEntityType.getSingularAttribute( "name", String.class ) ),
stringValueParameter ) );

final Query query = entityManager.createQuery( criteriaUpdate );
query.setParameter( intValueParameter, 9 );
query.setParameter( stringValueParameter, "Luigi" );

query.executeUpdate();
} );
}

@Test
public void testCriteriaUpdate2(EntityManagerFactoryScope scope) {
scope.inTransaction(
entityManager -> {
final CriteriaBuilder criteriaBuilder = entityManager.getCriteriaBuilder();
final CriteriaUpdate<Person> criteriaUpdate = criteriaBuilder.createCriteriaUpdate( Person.class );
final Root<Person> root = criteriaUpdate.from( Person.class );
scope.inTransaction( entityManager -> {
final CriteriaBuilder criteriaBuilder = entityManager.getCriteriaBuilder();
final CriteriaUpdate<Person> criteriaUpdate = criteriaBuilder.createCriteriaUpdate( Person.class );
final Root<Person> root = criteriaUpdate.from( Person.class );

final ParameterExpression<Integer> intValueParameter = criteriaBuilder.parameter( Integer.class );
final ParameterExpression<String> stringValueParameter = criteriaBuilder.parameter( String.class );
final ParameterExpression<Integer> intValueParameter = criteriaBuilder.parameter( Integer.class );
final ParameterExpression<String> stringValueParameter = criteriaBuilder.parameter( String.class );

criteriaUpdate.set( "age", intValueParameter );
criteriaUpdate.where( criteriaBuilder.equal( root.get( "name" ), stringValueParameter ) );
criteriaUpdate.set( "age", intValueParameter );
criteriaUpdate.where( criteriaBuilder.equal( root.get( "name" ), stringValueParameter ) );

final Query query = entityManager.createQuery( criteriaUpdate );
query.setParameter( intValueParameter, 9 );
query.setParameter( stringValueParameter, "Luigi" );
final Query query = entityManager.createQuery( criteriaUpdate );
query.setParameter( intValueParameter, 9 );
query.setParameter( stringValueParameter, "Luigi" );

query.executeUpdate();
}
);
query.executeUpdate();
} );
}

@Test
public void testCriteriaUpdate3(EntityManagerFactoryScope scope) {
scope.inTransaction( em -> {
// test separate value-bind parameters
final CriteriaBuilder cb = em.getCriteriaBuilder();
final CriteriaUpdate<Process> cu = cb.createCriteriaUpdate( Process.class );
final Root<Process> root = cu.from( Process.class );
cu.set( root.get( "name" ), (Object) null );
cu.set( root.get( "payload" ), (Object) null );
em.createQuery( cu ).executeUpdate();
} );

scope.inTransaction( em -> {
// test with the same cb.value( null ) parameter instance
final HibernateCriteriaBuilder cb = (HibernateCriteriaBuilder) em.getCriteriaBuilder();
final CriteriaUpdate<Process> cu = cb.createCriteriaUpdate( Process.class );
final Root<Process> root = cu.from( Process.class );
final Expression<Object> nullValue = cb.value( null );
// a bit unfortunate, but we need to cast here to prevent ambiguous method references
final Path<String> name = root.get( "name" );
final Path<byte[]> payload = root.get( "payload" );
final Expression<? extends String> nullString = cast( nullValue );
final Expression<? extends byte[]> nullBytes = cast( nullValue );
cu.set( name, nullString );
cu.set( payload, nullBytes );
em.createQuery( cu ).executeUpdate();
} );
}

private static <X> Expression<? extends X> cast(Expression<?> expression) {
//noinspection unchecked
return (Expression<? extends X>) expression;
}

@Entity(name = "Person")
public static class Person {

@Id
private String id;

Expand All @@ -101,4 +133,18 @@ public Integer getAge() {
return age;
}
}

@Entity
public static class Process {
@Id
@GeneratedValue
private Long id;

// All attributes below are necessary to reproduce the issue

private String name;

@Lob
private byte[] payload;
}
}
Loading