[hibernate-dev] HHH-8999/HHH-10413 and Comparable IDs

Wed Mar 16 01:10:54 EDT 2016

HHH-9322 involves a custom user type for an ID.

On Mon, Mar 14, 2016 at 12:38 AM, Gail Badner <gbadner at redhat.com> wrote:

> 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/main/java/org/hibernate/action/internal/EntityAction.java#L154
> [2]
> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/action/internal/CollectionAction.java#L159
> [3]
> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/type/CustomType.java#L182
> [4]
> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/cache/spi/CacheDataDescription.java#L29
>
>
> On Fri, Mar 11, 2016 at 11:47 AM, Gail Badner <gbadner at redhat.com> 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/main/java/org/hibernate/type/BinaryType.java
>>
>>
>> On Fri, Mar 11, 2016 at 7:36 AM, Emmanuel Bernard <emmanuel at 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 at hibernate.org>
>>> 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 at redhat.com>
>>> 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 at redhat.com>
>>> wrote:
>>> > >>
>>> > >> > Missed something. Please see below...
>>> > >> >
>>> > >> > On Wed, Mar 2, 2016 at 11:49 PM, Gail Badner <gbadner at redhat.com>
>>> > >> 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/main/java/org/hibernate/engine/spi/ExecutableList.java#L194
>>> > >> >>
>>> > >> >
>>> > >> >
>>> > >> _______________________________________________
>>> > >> hibernate-dev mailing list
>>> > >> hibernate-dev at lists.jboss.org
>>> > >> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>> > >>
>>> > >
>>> > _______________________________________________
>>> > hibernate-dev mailing list
>>> > hibernate-dev at lists.jboss.org
>>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>>
>>
>>
>