[
https://issues.apache.org/jira/browse/CASSANDRA-7981?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14211385#comment-14211385
]
Tyler Hobbs commented on CASSANDRA-7981:
----------------------------------------
Preliminary review comments:
* The restriction class hierarchy is confusing, but I don't see an obvious way
to simplify it
* {{AbstracPrimaryKeyRestrictions}} has a typo in the name
* {{MultiColumnRestriction}}
** {{hasIndexedColumns()}} always returns on the first item
** In any case, I don't think the behavior of {{hasIndexedColumns()}} would be
correct for slice relations, since that would require a composite index
(assuming CASSANDRA-4476 gets implemented)
* I think {{hasSupportingIndex()}} would be a more accurate name for
{{hasIndexedColumns()}}
* {{ForwardingPrimaryKeyRestrictions}} could use class-level javadocs
explaining why we need it
* {{StatementRestrictions.getPartitionKeyUnrestrictedComponents()}} should use
{{list.removeAll()}} instead of {{list.remove()}} (and we'll want a test to
cover that)
* {{SelectStatement}} has a ton of {{cql3}} and {{db}} imports that could be
reverted back to a {{*}} import
* There seems to be a conflict between the {{mergeWith(Restriction)}}
signatures in {{Restriction}} and {{Restrictions}}. Perhaps rename
{{Restrictions.mergeWith()}} to {{Restrictions.addRestriction()}} and make sure
the subclasses implement both correctly?
* There are a few javadoc params that are misnamed here and there in the
restriction classes (hopefully Eclipse can point those out for you)
* StatementRestrictions has some unneeded {{<RowPosition>}} generics that can
be replaced with {{<>}}
Other than that, I'm having QA do test coverage analysis on your branch so we
can see if there are any gaps in the testing on the new code.
> Refactor SelectStatement
> ------------------------
>
> Key: CASSANDRA-7981
> URL: https://issues.apache.org/jira/browse/CASSANDRA-7981
> Project: Cassandra
> Issue Type: Bug
> Reporter: Benjamin Lerer
> Assignee: Benjamin Lerer
> Fix For: 3.0
>
>
> The current state of the code of SelectStatement make fixing some issues or
> adding new functionnalities really hard. It also contains some
> functionnalities that we would like to reuse in ModificationStatement but
> cannot for the moment.
> Ideally I would like to:
> * Perform as much validation as possible on Relations instead of performing
> it on Restrictions as it will help for problem like the one of
> #CASSANDRA-6075 (I believe that by clearly separating validation and
> Restrictions building we will also make the code a lot clearer)
> * Provide a way to easily merge restrictions on the same columns as needed
> for #CASSANDRA-7016
> * Have a preparation logic (validation + pre-processing) that we can easily
> reuse for Delete statement #CASSANDRA-6237
> * Make the code much easier to read and safer to modify.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
