[
https://issues.apache.org/jira/browse/CASSANDRA-8613?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14327926#comment-14327926
]
Tyler Hobbs commented on CASSANDRA-8613:
----------------------------------------
Overall, this looks excellent. Nice job on keeping it simple, and thanks for
the large test suites.
Some review comments, mostly around code clarity:
* In the 2.0 patch, the comment in {{SelectStatement.buildBound()}} about
preserving pre-6875 behavior appears to be stale and is a little confusing
* In {{SelectStatement.updateRestrictionsForRelation()}} (2.0 and 2.1), the
logic for checking for overlapping multi-column relations could use an
explanatory comment (or perhaps be changed for clarity):
{noformat}
if ((i == 0 && name.position != 0 && stmt.columnRestrictions[name.position - 1]
== existing)
|| (i != 0 && previous != existing)).
{noformat}
The error message helps, but it still took me a couple of minutes to figure out
how the checks worked.
* In {{SelectStatement.processColumnRestrictions()}} the check
{{!(previousRestriction != null && restriction == previousRestriction)}} can be
simplified to {{restriction != previousRestriction}}. If they're equal, it's a
multi-column relation. The comment above it could be broken up to clarify
where we're testing for each case.
* In the same method, there's a typo in the error message: "tulpe".
Additionally, it seems like we could give clearer error messages. How about
one error message for the "unrestricted previous column" case, and another for
non-eq restrictions where we just use the operator in the error message?
* The javadoc for {{testBuildBoundWithEqAndInRelation}} says the test is for a
single clustering column, but there are two
* In the 2.1 patch in {{SelectStatement}}, try to avoid using nested ternaries.
Comments specific to the trunk patch:
* {{RestrictionSet.getRestriction()}} is unused
* In {{RestrictionSet.addRestriction()}}, why is the TreeMap of existing
restrictions cloned instead of updated? (Maybe explain this in a comment.)
* In {{RestrictionSet.mergeRestriction()}}, I'm not sure what the comment is
trying to point out. Is it just that {{mergeRestriction()}} may throw an IRE?
* In the {{PrimaryKeyRestrictionSet}} constructor, it might be clearer to
rename {{lastColumn}} and {{newColumn}} to something like
{{lastRestrictionStart}} and {{newRestrictionStart}}, respectively
* In {{CompositesBuilder.addEachElementToAll()}}, if an empty values list is
passed in, everything will be removed from {{elementsList}}. It looks like
this is okay right now because {{hasMissingElements}} gets set to {{true}}, but
it seems like it may be a bug, especially because {{addAllElementsToAll()}}
doesn't have the same behavior with empty lists.
If you can make any changes on a git branch and link them, I would appreciate
it. It gets difficult to follow changes with large patches.
> Regression in mixed single and multi-column relation support
> ------------------------------------------------------------
>
> Key: CASSANDRA-8613
> URL: https://issues.apache.org/jira/browse/CASSANDRA-8613
> Project: Cassandra
> Issue Type: Bug
> Components: Core
> Reporter: Tyler Hobbs
> Assignee: Benjamin Lerer
> Fix For: 2.1.4
>
> Attachments: CASSANDRA-8613-2.0.txt, CASSANDRA-8613-2.1.txt,
> CASSANDRA-8613-trunk.txt
>
>
> In 2.0.6 through 2.0.8, a query like the following was supported:
> {noformat}
> SELECT * FROM mytable WHERE clustering_0 = ? AND (clustering_1, clustering_2)
> > (?, ?)
> {noformat}
> However, after CASSANDRA-6875, you'll get the following error:
> {noformat}
> Clustering columns may not be skipped in multi-column relations. They should
> appear in the PRIMARY KEY order. Got (c, d) > (0, 0)
> {noformat}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
