cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tyler Hobbs (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-11354) PrimaryKeyRestrictionSet should be refactored
Date Thu, 14 Apr 2016 22:51:25 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-11354?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15242083#comment-15242083
] 

Tyler Hobbs commented on CASSANDRA-11354:
-----------------------------------------

The patch looks good to me overall.  The new class structure is much more understandable.
I think the only remaining confusing part is the difference between {{PartitionKeyRestrictions}}
and {{PartitionKeyRestrictionSet}}, which is not obvious at first.  I think perhaps renaming
{{PartitionKeyRestrictionSet}} to {{PartitionKeySingleRestrictionSet}} may make this clearer
(although the new name is long).  Editing the javadocs to clarify that {{PartitionKeyRestrictionSet}}
specifically doesn't include token restrictions would also help.

Other than that, I only have a couple of nitpicks:
* In {{ClusteringColumnRestrictions}}, most of the checks in the private constructor can be
moved to {{mergeWith()}} to make it more clear what is going on.
* {{Restrictions}} has two redundant methods that are also declared in {{Restriction}}: {{getColumnDefs()}}
and {{getFunctions()}}

With those fixed, +1

> PrimaryKeyRestrictionSet should be refactored
> ---------------------------------------------
>
>                 Key: CASSANDRA-11354
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-11354
>             Project: Cassandra
>          Issue Type: Improvement
>          Components: CQL
>            Reporter: Benjamin Lerer
>            Assignee: Benjamin Lerer
>
> While reviewing CASSANDRA-11310 I realized that the code of {{PrimaryKeyRestrictionSet}}
was really confusing.
> The main 2 issues are:
> * the fact that it is used for partition keys and clustering columns restrictions whereas
those types of column required different processing
> * the {{isEQ}}, {{isSlice}}, {{isIN}} and {{isContains}} methods should not be there
as the set of restrictions might not match any of those categories when secondary indexes
are used.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message