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 <st...@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 <gbad...@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 <gbad...@redhat.com> wrote: >> >> > Missed something. Please see below... >> > >> > On Wed, Mar 2, 2016 at 11:49 PM, Gail Badner <gbad...@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@lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/hibernate-dev >> > _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
