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

[Gail Badner]

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
>>
>