cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tyler Hobbs (JIRA)" <>
Subject [jira] [Commented] (CASSANDRA-6783) Collections should have a proper compare() method for UDT
Date Wed, 12 Mar 2014 21:16:46 GMT


Tyler Hobbs commented on CASSANDRA-6783:

In, and ListType.compareListOrSet you have:

        if (o1 == null || !o1.hasRemaining())                                            
            return o2 == null || o2.hasRemaining() ? -1 : 0;
This should be:
        if (o1 == null || !o1.hasRemaining())                                            
            return o2 == null || !o2.hasRemaining() ? 0 : -1;

Can you add some unit tests?

bq. I can separate that trivial refactor in a separate patch if we really need to

Just committing the ByteBuffer changes separately should be fine.

> Collections should have a proper compare() method for UDT
> ---------------------------------------------------------
>                 Key: CASSANDRA-6783
>                 URL:
>             Project: Cassandra
>          Issue Type: Bug
>            Reporter: Sylvain Lebresne
>            Assignee: Sylvain Lebresne
>             Fix For: 2.1 beta2
>         Attachments: 6783.txt
> So far, ListType, SetType and MapType don't have a proper implementation of compare()
(they throw UnsupportedOperationException) because we haven't need one since as far as the
cell comparator is concenred, only parts of a collection ends up in the comparator and need
to be compared, but the full collection itself does not.
> But with UDT can nest a collection and that sometimes require to be able to compare them.
Typically, I pushed a dtest [here|]
that ends up throwing:
> {noformat}
> java.lang.UnsupportedOperationException: CollectionType should not be use directly as
a comparator
>         at
>         at
>         at org.apache.cassandra.db.marshal.AbstractType.compareCollectionMembers(
>         at
>         at
>         at ~[na:1.7.0_45]
>         at java.util.TreeMap.put( ~[na:1.7.0_45]
>         at java.util.TreeSet.add( ~[na:1.7.0_45]
>         at org.apache.cassandra.cql3.Sets$DelayedValue.bind( ~[main/:na]
>         at org.apache.cassandra.cql3.Sets$Literal.prepare( ~[main/:na]
>         at org.apache.cassandra.cql3.UserTypes$Literal.prepare( ~[main/:na]
>         at org.apache.cassandra.cql3.Operation$SetElement.prepare(
>         at org.apache.cassandra.cql3.statements.UpdateStatement$ParsedUpdate.prepareInternal(
> ...
> {noformat}
> Note that this stack doesn't involve cell name comparison at all, it's just that CQL3
sometimes uses a SortedSet underneath to deal with set literals (since internal sets are sorted
by their value), and so when a set contains UDT that has set themselves, we need the collection
comparison. That being said, for some cases like having a UDT as a map key, we do would need
collections to be comparable for the purpose of cell name comparison.
> Attaching relatively simple patch. The patch is a bit bigger than it should be because
while adding the 3 simple compare() method, I realized that we had methods to read a short
length (2 unsigned short) from a ByteBuffer duplicated all over the place and that it was
time to consolidate that in ByteBufferUtil where it should have been from day one (thus removing
the duplication). I can separate that trivial refactor in a separate patch if we really need
to, but really, the new stuff is the compare() method implementation in ListType, SetType
and MapType and the rest is a bit of trivial cleanup. 

This message was sent by Atlassian JIRA

View raw message