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