Missed something. Please see below... On Wed, Mar 2, 2016 at 11:49 PM, Gail Badner <gbadner(a)redhat.com&gt; wrote: > ExecutableList#add attempts to keep track of whether the Executable > objects are sorted. [1] > > Except for entity insert actions (which use InsertActionSorter), > ExecutableList#add uses the following to determine if adding the current > Executable to the end invalidates the sort: > > if ( previousLast != null && previousLast.compareTo( executable ) > 0 ) { > sorted = false; > } > > EntityAction#compareTo compares the IDs for the current and previous > EntityAction if they have different entity names; similarly, > CollectionAction compares the collection keys for the current and previous > CollectionAction if they have different entity names. > > In most cases, the ID class implements Comparable, although I don't know > of any requirement that says this needs to be true. > > This breaks down when a byte[] ID is used as described by HHH-8999. The > reason is that AbstractTypeDescriptor#comparator is null because it is > assigned like this: > > this.comparator = Comparable.class.isAssignableFrom( type ) > ? (Comparator<T>) ComparableComparator.INSTANCE > : null; > > PrimitiveByteArrayTypeDescriptor does not override > AbstractTypeDescriptor#getComparator, so null is returned ultimately > causing a NullPointerException in AbstractStandardBasicType#compare: > > public final int compare(Object x, Object y) { > return javaTypeDescriptor.getComparator().compare( (T) x, (T) y ); > } > > The same happens for PrimitiveCharacterArrayTypeDescriptor, > ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. Are there others? > > According to HHH-10413, this also affects version attributes. > > Here are a couple of alternatives: > > A) create comparators for PrimitiveCharacterArrayTypeDescriptor, > ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. > PrimitiveByteArrayTypeDescript should also be in the list for A). > > > B) Change AbstractTypeDescriptor#comparator to: > > this.comparator = Comparable.class.isAssignableFrom( type ) > ? (Comparator<T>) ComparableComparator.INSTANCE > : IncomparableComparator.INSTANCE; > > IncomparableComparator#compare always returns 0, so that comparison would > never invalidate the sort. > > B) is not acceptable for PrimitiveByteArrayTypeDescripter because a version attribute can be of type byte[]. We definitely do not want all byte[] version values to compare as equal. A Comparator needs to be implemented at least for PrimitiveByteArrayTypeDescripter. The following question only applies to PrimitiveCharacterArrayTypeDescriptor, ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. > Does it make sense to sort Executable objects by an ID or collection key > that is an array? In other words, would such IDs be generated in a natural > order? If not, I think B) makes more sense. > > Thoughts? > > Thanks, > Gail > > [1] > https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src... >

Personally I think using IncomparableComparator or null here makes the most sense. I'm not really sure what the sort ordering of a byte array would "mean". On Thu, Mar 10, 2016, 1:41 AM Gail Badner <gbadner(a)redhat.com&gt; wrote: > I've created a pull request implements option A) (creates comparators > for PrimitiveByteArrayTypeDescriptor, > PrimitiveCharacterArrayTypeDescriptor, ByteArrayTypeDescriptor, and > CharacterArrayTypeDescriptor. > > I'm still not sure if it makes sense to have a comparators for arrays. > > Steve, please take a look at the pull request [1] and let me know if this > looks reasonable to you. > > Thanks, > Gail > > [1] https://github.com/hibernate/hibernate-orm/pull/1294 > > On Thu, Mar 3, 2016 at 2:44 AM, Gail Badner <gbadner(a)redhat.com&gt; wrote: > > > Missed something. Please see below... > > > > On Wed, Mar 2, 2016 at 11:49 PM, Gail Badner <gbadner(a)redhat.com&gt; > wrote: > > > >> ExecutableList#add attempts to keep track of whether the Executable > >> objects are sorted. [1] > >> > >> Except for entity insert actions (which use InsertActionSorter), > >> ExecutableList#add uses the following to determine if adding the > current > >> Executable to the end invalidates the sort: > >> > >> if ( previousLast != null && previousLast.compareTo( executable ) > 0 > ) { > >> sorted = false; > >> } > >> > >> EntityAction#compareTo compares the IDs for the current and previous > >> EntityAction if they have different entity names; similarly, > >> CollectionAction compares the collection keys for the current and > previous > >> CollectionAction if they have different entity names. > >> > >> In most cases, the ID class implements Comparable, although I don't > know > >> of any requirement that says this needs to be true. > >> > >> This breaks down when a byte[] ID is used as described by HHH-8999. The > >> reason is that AbstractTypeDescriptor#comparator is null because it is > >> assigned like this: > >> > >> this.comparator = Comparable.class.isAssignableFrom( type ) > >> ? (Comparator<T>) ComparableComparator.INSTANCE > >> : null; > >> > >> PrimitiveByteArrayTypeDescriptor does not override > >> AbstractTypeDescriptor#getComparator, so null is returned ultimately > >> causing a NullPointerException in AbstractStandardBasicType#compare: > >> > >> public final int compare(Object x, Object y) { > >> return javaTypeDescriptor.getComparator().compare( (T) x, (T) y ); > >> } > >> > >> The same happens for PrimitiveCharacterArrayTypeDescriptor, > >> ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. Are there > others? > >> > >> According to HHH-10413, this also affects version attributes. > >> > >> Here are a couple of alternatives: > >> > >> A) create comparators for PrimitiveCharacterArrayTypeDescriptor, > >> ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. > >> > > > > PrimitiveByteArrayTypeDescript should also be in the list for A). > > > > > >> > >> > >> B) Change AbstractTypeDescriptor#comparator to: > >> > >> this.comparator = Comparable.class.isAssignableFrom( type ) > >> ? (Comparator<T>) ComparableComparator.INSTANCE > >> : IncomparableComparator.INSTANCE; > >> > >> IncomparableComparator#compare always returns 0, so that comparison > would > >> never invalidate the sort. > >> > >> > > B) is not acceptable for PrimitiveByteArrayTypeDescripter because a > > version attribute can be of type byte[]. We definitely do not want all > > byte[] version values to compare as equal. A Comparator needs to be > > implemented at least for PrimitiveByteArrayTypeDescripter. > > > > The following question only applies to > > PrimitiveCharacterArrayTypeDescriptor, ByteArrayTypeDescriptor, and > > CharacterArrayTypeDescriptor. > > > > > >> Does it make sense to sort Executable objects by an ID or collection > key > >> that is an array? In other words, would such IDs be generated in a > natural > >> order? If not, I think B) makes more sense. > >> > > > >> Thoughts? > >> > >> Thanks, > >> Gail > >> > >> [1] > >> > https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src... > >> > > > > > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev >

Version can be a byte[] ? I thought JPA was restricting version types actually. On Thu 2016-03-10 12:19, Gail Badner wrote: > As I mentioned before, it is not acceptable for the comparator for > PrimitiveByteArrayTypeDescripter to be IncomparableComparator because a > version attribute can be of type byte[]. We definitely do not want all > byte[] version values to compare as equal. A Comparator needs to be > implemented at least for PrimitiveByteArrayTypeDescripter. > > I'm OK with changing getComparator() for PrimitiveCharacterArrayTypeDescriptor, > ByteArrayTypeDescriptor, and > CharacterArrayTypeDescriptor to return IncomparableComparator.INSTANCE. I > think that is a safer option than B) (making > IncomparableComparator.INSTANCE the default if the type is not assignable > to Comparable). > > I'm also OK with using what I implemented in the pull request. > > On Thu, Mar 10, 2016 at 7:27 AM, Steve Ebersole <steve(a)hibernate.org&gt; wrote: > > > Personally I think using IncomparableComparator or null here makes the > > most sense. I'm not really sure what the sort ordering of a byte array > > would "mean". > > > > On Thu, Mar 10, 2016, 1:41 AM Gail Badner <gbadner(a)redhat.com&gt; wrote: > > > >> I've created a pull request implements option A) (creates comparators > >> for PrimitiveByteArrayTypeDescriptor, > >> PrimitiveCharacterArrayTypeDescriptor, ByteArrayTypeDescriptor, and > >> CharacterArrayTypeDescriptor. > >> > >> I'm still not sure if it makes sense to have a comparators for arrays. > >> > >> Steve, please take a look at the pull request [1] and let me know if this > >> looks reasonable to you. > >> > >> Thanks, > >> Gail > >> > >> [1] https://github.com/hibernate/hibernate-orm/pull/1294 > >> > >> On Thu, Mar 3, 2016 at 2:44 AM, Gail Badner <gbadner(a)redhat.com&gt; wrote: > >> > >> > Missed something. Please see below... > >> > > >> > On Wed, Mar 2, 2016 at 11:49 PM, Gail Badner <gbadner(a)redhat.com&gt; > >> wrote: > >> > > >> >> ExecutableList#add attempts to keep track of whether the Executable > >> >> objects are sorted. [1] > >> >> > >> >> Except for entity insert actions (which use InsertActionSorter), > >> >> ExecutableList#add uses the following to determine if adding the > >> current > >> >> Executable to the end invalidates the sort: > >> >> > >> >> if ( previousLast != null && previousLast.compareTo( executable ) > 0 > >> ) { > >> >> sorted = false; > >> >> } > >> >> > >> >> EntityAction#compareTo compares the IDs for the current and previous > >> >> EntityAction if they have different entity names; similarly, > >> >> CollectionAction compares the collection keys for the current and > >> previous > >> >> CollectionAction if they have different entity names. > >> >> > >> >> In most cases, the ID class implements Comparable, although I don't > >> know > >> >> of any requirement that says this needs to be true. > >> >> > >> >> This breaks down when a byte[] ID is used as described by HHH-8999. The > >> >> reason is that AbstractTypeDescriptor#comparator is null because it is > >> >> assigned like this: > >> >> > >> >> this.comparator = Comparable.class.isAssignableFrom( type ) > >> >> ? (Comparator<T>) ComparableComparator.INSTANCE > >> >> : null; > >> >> > >> >> PrimitiveByteArrayTypeDescriptor does not override > >> >> AbstractTypeDescriptor#getComparator, so null is returned ultimately > >> >> causing a NullPointerException in AbstractStandardBasicType#compare: > >> >> > >> >> public final int compare(Object x, Object y) { > >> >> return javaTypeDescriptor.getComparator().compare( (T) x, (T) y ); > >> >> } > >> >> > >> >> The same happens for PrimitiveCharacterArrayTypeDescriptor, > >> >> ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. Are there > >> others? > >> >> > >> >> According to HHH-10413, this also affects version attributes. > >> >> > >> >> Here are a couple of alternatives: > >> >> > >> >> A) create comparators for PrimitiveCharacterArrayTypeDescriptor, > >> >> ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. > >> >> > >> > > >> > PrimitiveByteArrayTypeDescript should also be in the list for A). > >> > > >> > > >> >> > >> >> > >> >> B) Change AbstractTypeDescriptor#comparator to: > >> >> > >> >> this.comparator = Comparable.class.isAssignableFrom( type ) > >> >> ? (Comparator<T>) ComparableComparator.INSTANCE > >> >> : IncomparableComparator.INSTANCE; > >> >> > >> >> IncomparableComparator#compare always returns 0, so that comparison > >> would > >> >> never invalidate the sort. > >> >> > >> >> > >> > B) is not acceptable for PrimitiveByteArrayTypeDescripter because a > >> > version attribute can be of type byte[]. We definitely do not want all > >> > byte[] version values to compare as equal. A Comparator needs to be > >> > implemented at least for PrimitiveByteArrayTypeDescripter. > >> > > >> > The following question only applies to > >> > PrimitiveCharacterArrayTypeDescriptor, ByteArrayTypeDescriptor, and > >> > CharacterArrayTypeDescriptor. > >> > > >> > > >> >> Does it make sense to sort Executable objects by an ID or collection > >> key > >> >> that is an array? In other words, would such IDs be generated in a > >> natural > >> >> order? If not, I think B) makes more sense. > >> >> > >> > > >> >> Thoughts? > >> >> > >> >> Thanks, > >> >> Gail > >> >> > >> >> [1] > >> >> > >> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src... > >> >> > >> > > >> > > >> _______________________________________________ > >> hibernate-dev mailing list > >> hibernate-dev(a)lists.jboss.org > >> https://lists.jboss.org/mailman/listinfo/hibernate-dev > >> > > > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev

In addition to affecting entity IDs [1], this also affects collection keys [2]. I've also reproduced a similar problem when an implementation of UserType in a composite ID or collection key will fail, unless the implementation extends Comparator. This is because CustomType#getComparator returns (Comparator) userType [3]; UserType and EnhancedUserType do not implement Comparator. IMO, we should not add a requirement that classes used for an ID or collection key must implement Comparable. I'm also not crazy about using IncomparableComparator for ID types that don't implement Comparable. My preference would be for EntityAction#compareTo only compare the IDs if: Comparable.class.isAssignableFrom( persister.getIdentifierType().getReturnedClass() ) Similarly for CollectionAction#compareTo, the collection keys would only be compared if: Comparable.class.isAssignableFrom( persister.getKeyType().getReturnedClass() ) Can the Javadoc for Type#compare be changed to say that Type#compare should only be called if Comparable.class.isAssignableFrom( getReturnedClass() )? The ways that Hibernate uses Type#compare is very limited, I *think* this should be OK. A comparator for VersionType is important when using the 2nd-level cache. The Javadoc for org.hibernate.cache.spi.CacheDataDescription#isVersioned says: "If {@code true}, it is illegal for {@link #getVersionComparator} to return {@code null}." [4] Since byte[] is used for versions, then BinaryType#getComparator needs to return a non-null Comparator that actually compares the values (not IncomparableComparator). IMO, it would be a little strange for BinaryType#compare to throw an exception but have BinaryType#getComparator return a non-null Comparator that actually compares the values. I could be convinced it's OK though. FWIW, an advantage of having BinaryType#getComparator return the Comparator direction (instead of delegating to PrimitiveByteArrayTypeDescriptor) is that the other types that use PrimitiveByteArrayTypeDescriptor, ImageType and MaterializedBlobType, will not have a Comparator defined. I think that makes sense since ImageType and MaterializedBlobType really should not be compared. Please let me know what you think about all this. Unfortunately it looks like this fix will not make it into 5.0.9. [1] https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src... [2] https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src... [3] https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src... [4] https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src... On Fri, Mar 11, 2016 at 11:47 AM, Gail Badner <gbadner(a)redhat.com&gt; wrote: > Hibernate supports byte[] versions. You can see that org.hibernate.type.BinaryType implements VersionType<byte[]>. [1] > > The comment in BinaryType#seed says: > // Note : simply returns null for seed() and next() as the only known > // application of binary types for versioning is for use with the > // TIMESTAMP datatype supported by Sybase and SQL Server, which > // are completely db-generated values... > > There is also a unit test, org.hibernate.test.version.sybase.SybaseTimestampVersioningTest that maps: > > <version name="timestamp" type="binary" generated="always"> > <column name="ts" sql-type="timestamp"/> > </version> > > [1] https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src... > > > On Fri, Mar 11, 2016 at 7:36 AM, Emmanuel Bernard <emmanuel(a)hibernate.org > > wrote: > >> Version can be a byte[] ? I thought JPA was restricting version types >> actually. >> >> On Thu 2016-03-10 12:19, Gail Badner wrote: >> > As I mentioned before, it is not acceptable for the comparator for >> > PrimitiveByteArrayTypeDescripter to be IncomparableComparator because a >> > version attribute can be of type byte[]. We definitely do not want all >> > byte[] version values to compare as equal. A Comparator needs to be >> > implemented at least for PrimitiveByteArrayTypeDescripter. >> > >> > I'm OK with changing getComparator() for >> PrimitiveCharacterArrayTypeDescriptor, >> > ByteArrayTypeDescriptor, and >> > CharacterArrayTypeDescriptor to return >> IncomparableComparator.INSTANCE. I >> > think that is a safer option than B) (making >> > IncomparableComparator.INSTANCE the default if the type is not >> assignable >> > to Comparable). >> > >> > I'm also OK with using what I implemented in the pull request. >> > >> > On Thu, Mar 10, 2016 at 7:27 AM, Steve Ebersole <steve(a)hibernate.org&gt; >> wrote: >> > >> > > Personally I think using IncomparableComparator or null here makes >> the >> > > most sense. I'm not really sure what the sort ordering of a byte >> array >> > > would "mean". >> > > >> > > On Thu, Mar 10, 2016, 1:41 AM Gail Badner <gbadner(a)redhat.com&gt; >> wrote: >> > > >> > >> I've created a pull request implements option A) (creates >> comparators >> > >> for PrimitiveByteArrayTypeDescriptor, >> > >> PrimitiveCharacterArrayTypeDescriptor, ByteArrayTypeDescriptor, and >> > >> CharacterArrayTypeDescriptor. >> > >> >> > >> I'm still not sure if it makes sense to have a comparators for >> arrays. >> > >> >> > >> Steve, please take a look at the pull request [1] and let me know >> if this >> > >> looks reasonable to you. >> > >> >> > >> Thanks, >> > >> Gail >> > >> >> > >> [1] https://github.com/hibernate/hibernate-orm/pull/1294 >> > >> >> > >> On Thu, Mar 3, 2016 at 2:44 AM, Gail Badner <gbadner(a)redhat.com&gt; >> wrote: >> > >> >> > >> > Missed something. Please see below... >> > >> > >> > >> > On Wed, Mar 2, 2016 at 11:49 PM, Gail Badner <gbadner(a)redhat.com&gt; >> > >> wrote: >> > >> > >> > >> >> ExecutableList#add attempts to keep track of whether the >> Executable >> > >> >> objects are sorted. [1] >> > >> >> >> > >> >> Except for entity insert actions (which use InsertActionSorter), >> > >> >> ExecutableList#add uses the following to determine if adding the >> > >> current >> > >> >> Executable to the end invalidates the sort: >> > >> >> >> > >> >> if ( previousLast != null && previousLast.compareTo( executable >> ) > 0 >> > >> ) { >> > >> >> sorted = false; >> > >> >> } >> > >> >> >> > >> >> EntityAction#compareTo compares the IDs for the current and >> previous >> > >> >> EntityAction if they have different entity names; similarly, >> > >> >> CollectionAction compares the collection keys for the current and >> > >> previous >> > >> >> CollectionAction if they have different entity names. >> > >> >> >> > >> >> In most cases, the ID class implements Comparable, although I >> don't >> > >> know >> > >> >> of any requirement that says this needs to be true. >> > >> >> >> > >> >> This breaks down when a byte[] ID is used as described by >> HHH-8999. The >> > >> >> reason is that AbstractTypeDescriptor#comparator is null because >> it is >> > >> >> assigned like this: >> > >> >> >> > >> >> this.comparator = Comparable.class.isAssignableFrom( type ) >> > >> >> ? (Comparator<T>) ComparableComparator.INSTANCE >> > >> >> : null; >> > >> >> >> > >> >> PrimitiveByteArrayTypeDescriptor does not override >> > >> >> AbstractTypeDescriptor#getComparator, so null is returned >> ultimately >> > >> >> causing a NullPointerException in >> AbstractStandardBasicType#compare: >> > >> >> >> > >> >> public final int compare(Object x, Object y) { >> > >> >> return javaTypeDescriptor.getComparator().compare( (T) x, >> (T) y ); >> > >> >> } >> > >> >> >> > >> >> The same happens for PrimitiveCharacterArrayTypeDescriptor, >> > >> >> ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. Are >> there >> > >> others? >> > >> >> >> > >> >> According to HHH-10413, this also affects version attributes. >> > >> >> >> > >> >> Here are a couple of alternatives: >> > >> >> >> > >> >> A) create comparators for PrimitiveCharacterArrayTypeDescriptor, >> > >> >> ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. >> > >> >> >> > >> > >> > >> > PrimitiveByteArrayTypeDescript should also be in the list for A). >> > >> > >> > >> > >> > >> >> >> > >> >> >> > >> >> B) Change AbstractTypeDescriptor#comparator to: >> > >> >> >> > >> >> this.comparator = Comparable.class.isAssignableFrom( type ) >> > >> >> ? (Comparator<T>) ComparableComparator.INSTANCE >> > >> >> : IncomparableComparator.INSTANCE; >> > >> >> >> > >> >> IncomparableComparator#compare always returns 0, so that >> comparison >> > >> would >> > >> >> never invalidate the sort. >> > >> >> >> > >> >> >> > >> > B) is not acceptable for PrimitiveByteArrayTypeDescripter because >> a >> > >> > version attribute can be of type byte[]. We definitely do not >> want all >> > >> > byte[] version values to compare as equal. A Comparator needs to >> be >> > >> > implemented at least for PrimitiveByteArrayTypeDescripter. >> > >> > >> > >> > The following question only applies to >> > >> > PrimitiveCharacterArrayTypeDescriptor, ByteArrayTypeDescriptor, >> and >> > >> > CharacterArrayTypeDescriptor. >> > >> > >> > >> > >> > >> >> Does it make sense to sort Executable objects by an ID or >> collection >> > >> key >> > >> >> that is an array? In other words, would such IDs be generated in >> a >> > >> natural >> > >> >> order? If not, I think B) makes more sense. >> > >> >> >> > >> > >> > >> >> Thoughts? >> > >> >> >> > >> >> Thanks, >> > >> >> Gail >> > >> >> >> > >> >> [1] >> > >> >> >> > >> >> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src... >> > >> >> >> > >> > >> > >> > >> > >> _______________________________________________ >> > >> hibernate-dev mailing list >> > >> hibernate-dev(a)lists.jboss.org >> > >> https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > >> >> > > >> > _______________________________________________ >> > hibernate-dev mailing list >> > hibernate-dev(a)lists.jboss.org >> > https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > >