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