From 85b3ad297f09462ae3631a5ab5cd6a60c1e5d8b6 Mon Sep 17 00:00:00 2001 From: Steve Ebersole Date: Wed, 11 Nov 2009 20:27:39 +0000 Subject: [PATCH] HHH-4065 - Incorrect SQL is used for HQL if the number of values for a filter collection parameter is changed git-svn-id: https://svn.jboss.org/repos/hibernate/core/trunk@17959 1b8cb986-b30d-0410-93ca-fae66ebed9b2 --- .../engine/query/QueryPlanCache.java | 98 ++++++++++++++----- .../org/hibernate/util/CollectionHelper.java | 40 +++++++- .../test/filter/DynamicFilterTest.java | 28 ++++-- 3 files changed, 131 insertions(+), 35 deletions(-) diff --git a/core/src/main/java/org/hibernate/engine/query/QueryPlanCache.java b/core/src/main/java/org/hibernate/engine/query/QueryPlanCache.java index 3e870c6671..462692dad4 100644 --- a/core/src/main/java/org/hibernate/engine/query/QueryPlanCache.java +++ b/core/src/main/java/org/hibernate/engine/query/QueryPlanCache.java @@ -26,10 +26,13 @@ package org.hibernate.engine.query; import org.hibernate.util.SimpleMRUCache; import org.hibernate.util.SoftLimitMRUCache; +import org.hibernate.util.CollectionHelper; import org.hibernate.engine.SessionFactoryImplementor; import org.hibernate.engine.query.sql.NativeSQLQuerySpecification; import org.hibernate.QueryException; import org.hibernate.MappingException; +import org.hibernate.impl.FilterImpl; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -40,6 +43,7 @@ import java.util.Iterator; import java.util.Set; import java.util.HashSet; import java.util.Collections; +import java.util.Collection; /** * Acts as a cache for compiled query plans, as well as query-parameter metadata. @@ -174,7 +178,7 @@ public class QueryPlanCache implements Serializable { private static class HQLQueryPlanKey implements Serializable { private final String query; private final boolean shallow; - private final Set filterNames; + private final Set filterKeys; private final int hashCode; public HQLQueryPlanKey(String query, boolean shallow, Map enabledFilters) { @@ -182,17 +186,23 @@ public class QueryPlanCache implements Serializable { this.shallow = shallow; if ( enabledFilters == null || enabledFilters.isEmpty() ) { - filterNames = Collections.EMPTY_SET; + filterKeys = Collections.EMPTY_SET; } else { - Set tmp = new HashSet(); - tmp.addAll( enabledFilters.keySet() ); - this.filterNames = Collections.unmodifiableSet( tmp ); + Set tmp = new HashSet( + CollectionHelper.determineProperSizing( enabledFilters ), + CollectionHelper.LOAD_FACTOR + ); + Iterator itr = enabledFilters.values().iterator(); + while ( itr.hasNext() ) { + tmp.add( new DynamicFilterKey( ( FilterImpl ) itr.next() ) ); + } + this.filterKeys = Collections.unmodifiableSet( tmp ); } int hash = query.hashCode(); hash = 29 * hash + ( shallow ? 1 : 0 ); - hash = 29 * hash + filterNames.hashCode(); + hash = 29 * hash + filterKeys.hashCode(); this.hashCode = hash; } @@ -206,17 +216,64 @@ public class QueryPlanCache implements Serializable { final HQLQueryPlanKey that = ( HQLQueryPlanKey ) o; - if ( shallow != that.shallow ) { - return false; + return shallow == that.shallow + && filterKeys.equals( that.filterKeys ) + && query.equals( that.query ); + + } + + public int hashCode() { + return hashCode; + } + } + + private static class DynamicFilterKey implements Serializable { + private final String filterName; + private final Map parameterMetadata; + private final int hashCode; + + private DynamicFilterKey(FilterImpl filter) { + this.filterName = filter.getName(); + if ( filter.getParameters().isEmpty() ) { + parameterMetadata = Collections.EMPTY_MAP; } - if ( !filterNames.equals( that.filterNames ) ) { - return false; + else { + parameterMetadata = new HashMap( + CollectionHelper.determineProperSizing( filter.getParameters() ), + CollectionHelper.LOAD_FACTOR + ); + Iterator itr = filter.getParameters().entrySet().iterator(); + while ( itr.hasNext() ) { + final Integer valueCount; + final Map.Entry entry = ( Map.Entry ) itr.next(); + if ( Collection.class.isInstance( entry.getValue() ) ) { + valueCount = new Integer( ( (Collection) entry.getValue() ).size() ); + } + else { + valueCount = new Integer(1); + } + parameterMetadata.put( entry.getKey(), valueCount ); + } } - if ( !query.equals( that.query ) ) { + + int hash = filterName.hashCode(); + hash = 31 * hash + parameterMetadata.hashCode(); + this.hashCode = hash; + } + + public boolean equals(Object o) { + if ( this == o ) { + return true; + } + if ( o == null || getClass() != o.getClass() ) { return false; } - return true; + DynamicFilterKey that = ( DynamicFilterKey ) o; + + return filterName.equals( that.filterName ) + && parameterMetadata.equals( that.parameterMetadata ); + } public int hashCode() { @@ -262,20 +319,11 @@ public class QueryPlanCache implements Serializable { final FilterQueryPlanKey that = ( FilterQueryPlanKey ) o; - if ( shallow != that.shallow ) { - return false; - } - if ( !filterNames.equals( that.filterNames ) ) { - return false; - } - if ( !query.equals( that.query ) ) { - return false; - } - if ( !collectionRole.equals( that.collectionRole ) ) { - return false; - } + return shallow == that.shallow + && filterNames.equals( that.filterNames ) + && query.equals( that.query ) + && collectionRole.equals( that.collectionRole ); - return true; } public int hashCode() { diff --git a/core/src/main/java/org/hibernate/util/CollectionHelper.java b/core/src/main/java/org/hibernate/util/CollectionHelper.java index ed65b550f9..b051f25ed9 100755 --- a/core/src/main/java/org/hibernate/util/CollectionHelper.java +++ b/core/src/main/java/org/hibernate/util/CollectionHelper.java @@ -29,6 +29,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; /** * Various help for handling collections. @@ -37,6 +38,8 @@ import java.util.Map; * @author Steve Ebersole */ public final class CollectionHelper { + public static final int MINIMUM_INITIAL_CAPACITY = 16; + public static final float LOAD_FACTOR = 0.75f; public static final List EMPTY_LIST = Collections.unmodifiableList( new ArrayList(0) ); public static final Collection EMPTY_COLLECTION = Collections.unmodifiableCollection( new ArrayList(0) ); @@ -54,7 +57,40 @@ public final class CollectionHelper { * @return The sized map. */ public static Map mapOfSize(int size) { - final int currentSize = (int) (size / 0.75f); - return new HashMap( Math.max( currentSize+ 1, 16), 0.75f ); + return new HashMap( determineProperSizing( size ), LOAD_FACTOR ); + } + + /** + * Given a map, determine the proper initial size for a new Map to hold the same number of values. + * Specifically we want to account for load size and load factor to prevent immediate resizing. + * + * @param original The original map + * @return The proper size. + */ + public static int determineProperSizing(Map original) { + return determineProperSizing( original.size() ); + } + + /** + * Given a set, determine the proper initial size for a new set to hold the same number of values. + * Specifically we want to account for load size and load factor to prevent immediate resizing. + * + * @param original The original set + * @return The proper size. + */ + public static int determineProperSizing(Set original) { + return determineProperSizing( original.size() ); + } + + /** + * Determine the proper initial size for a new collection in order for it to hold the given a number of elements. + * Specifically we want to account for load size and load factor to prevent immediate resizing. + * + * @param numberOfElements The number of elements to be stored. + * @return The proper size. + */ + public static int determineProperSizing(int numberOfElements) { + int actual = ( (int) (numberOfElements / LOAD_FACTOR) ) + 1; + return Math.max( actual, MINIMUM_INITIAL_CAPACITY ); } } diff --git a/testsuite/src/test/java/org/hibernate/test/filter/DynamicFilterTest.java b/testsuite/src/test/java/org/hibernate/test/filter/DynamicFilterTest.java index d25df135e1..090e6f9ee5 100644 --- a/testsuite/src/test/java/org/hibernate/test/filter/DynamicFilterTest.java +++ b/testsuite/src/test/java/org/hibernate/test/filter/DynamicFilterTest.java @@ -39,6 +39,7 @@ import org.slf4j.LoggerFactory; * * @author Steve */ +@SuppressWarnings({ "WhileLoopReplaceableByForEach", "unchecked" }) public class DynamicFilterTest extends FunctionalTestCase { private Logger log = LoggerFactory.getLogger( DynamicFilterTest.class ); @@ -99,7 +100,7 @@ public class DynamicFilterTest extends FunctionalTestCase { ts = ( ( SessionImplementor ) session ).getTimestamp(); session.enableFilter( "fulfilledOrders" ).setParameter( "asOfDate", testData.lastMonth.getTime() ); sp = ( Salesperson ) session.createQuery( "from Salesperson as s where s.id = :id" ) - .setLong( "id", testData.steveId.longValue() ) + .setLong( "id", testData.steveId ) .uniqueResult(); assertEquals( "Filtered-collection not bypassing 2L-cache", 1, sp.getOrders().size() ); @@ -142,6 +143,17 @@ public class DynamicFilterTest extends FunctionalTestCase { session.clear(); + session.disableFilter("regionlist"); + session.enableFilter( "regionlist" ).setParameterList( "regions", new String[]{"LA", "APAC", "APAC"} ); + // Second test retreival through hql with the collection as non-eager with different region list + salespersons = session.createQuery( "select s from Salesperson as s" ).list(); + assertEquals( "Incorrect salesperson count", 1, salespersons.size() ); + sp = ( Salesperson ) salespersons.get( 0 ); + assertEquals( "Incorrect order count", 1, sp.getOrders().size() ); + + session.clear(); + + // test retreival through hql with the collection join fetched salespersons = session.createQuery( "select s from Salesperson as s left join fetch s.orders" ).list(); assertEquals( "Incorrect salesperson count", 1, salespersons.size() ); @@ -224,7 +236,7 @@ public class DynamicFilterTest extends FunctionalTestCase { log.info( "Criteria query against Product..." ); List products = session.createCriteria( Product.class ) - .add( Restrictions.eq( "stockNumber", new Integer( 124 ) ) ) + .add( Restrictions.eq( "stockNumber", 124 ) ) .list(); assertEquals( "Incorrect product count", 1, products.size() ); @@ -288,10 +300,10 @@ public class DynamicFilterTest extends FunctionalTestCase { session.enableFilter("region").setParameter("region", "APAC"); DetachedCriteria lineItemSubquery = DetachedCriteria.forClass(LineItem.class) - .add(Restrictions.ge( "quantity", new Long(1L) )) - .createCriteria("product") - .add(Restrictions.eq("name", "Acme Hair Gel")) - .setProjection(Property.forName("id")); + .add( Restrictions.ge( "quantity", 1L ) ) + .createCriteria( "product" ) + .add( Restrictions.eq( "name", "Acme Hair Gel" ) ) + .setProjection( Property.forName( "id" ) ); List orders = session.createCriteria(Order.class) .add(Subqueries.exists(lineItemSubquery)) @@ -309,7 +321,7 @@ public class DynamicFilterTest extends FunctionalTestCase { .setProjection(Property.forName("id")); lineItemSubquery = DetachedCriteria.forClass(LineItem.class) - .add(Restrictions.ge("quantity", new Long(1L) )) + .add(Restrictions.ge("quantity", 1L )) .createCriteria("product") .add(Subqueries.propertyIn("id", productSubquery)) .setProjection(Property.forName("id")); @@ -662,7 +674,7 @@ public class DynamicFilterTest extends FunctionalTestCase { // Force the categories to not get initialized here List result = session.createQuery( "from Product as p where p.id = :id" ) - .setLong( "id", testData.prod1Id.longValue() ) + .setLong( "id", testData.prod1Id ) .list(); assertTrue( "No products returned from HQL", !result.isEmpty() );