[
https://issues.apache.org/jira/browse/CASSANDRA-6237?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14725919#comment-14725919
]
Joshua McKenzie commented on CASSANDRA-6237:
--------------------------------------------
Finished first pass of review and getting familiar with the code. On the whole,
the refactors look really solid and the new functionality looks thoroughly
implemented. For future reference: as a reviewer it would have been very
helpful to have the StatementType / StatementRestrictions refactors mentioned
in the comments above kept as discrete commits leading up to the work for Range
deletions, rather than as one monolithic commit to review.
Some feedback:
* ModificationStatement.Parsed.prepareConditions is a little odd to me, as its
implicit relationship between condition precedence is {{ifExists > ifNotExists
> conditions.isEmpty() > columnConditions}}. We might want to consider
Assertions in the Parsed ctor to communicate the intent of these various
conditions on Parsed. At the least, we should preserve the comment that was in
{{ModificationStatement.addConditions}} that used to explain this relationship.
* In Operations.java, rename "applyToX" to "appliesToX" to better capture the
intent of the method
* Same with ModificationStatement.applyOnlyTo*
* nit: on UpdateStatement.java:L219, revert change of "statements" to
"statement". Should read "INSERT statements are not allowed"
* AbstractConditions.java: remove redundant @Override on interface
implementations as per [coding
style|https://wiki.apache.org/cassandra/CodeStyle]
* ColumnConditions.java: inconsistent paradigms between {{getColumns()}} and
{{getFunctions()}} w/regards to building your Iterable/Collections
* IfExistsCondition.java: Missing apache header
* IfNotExistsCondition.java: Missing apache header
* Json.java: redundant @Override on abstract implementations
* CBuilder.java: In the two {{buildWith}}, there's no need to change to
System.arraycopy as Arrays.copyOf uses that behind the scenes.
* I'm hesitant about us crystallizing our unit tests around the exact verbiage
of the error messages we're throwing (see: {{assertInvalidMessage}} being added
into DeleteTest and InsertTest). Checking exception type alone doesn't buy us
the 100% confirmation that the exception we think was raised was actually
raised, however it allows for future modifications of the details of those
exceptions without having to track down failed unit tests that are explicitly
dependent upon them. I'm on the fence with this one; if you're strongly
attached to the {{assertInvalidMessage}} approach, stick with it.
* nit: Might want to wrap the new entries in NEWS.txt to try and keep them
under 100 or so characters, wherever the other lines tend to fall. The new
entries stick out as incongruous.
I'll take some more time tomorrow to further consider the patch and also plan
on taking a look at the rebased 3.0 branch when that becomes available.
> Allow range deletions in CQL
> ----------------------------
>
> Key: CASSANDRA-6237
> URL: https://issues.apache.org/jira/browse/CASSANDRA-6237
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Sylvain Lebresne
> Assignee: Benjamin Lerer
> Priority: Minor
> Labels: cql, docs
> Fix For: 3.0 beta 2
>
> Attachments: CASSANDRA-6237.txt
>
>
> We uses RangeTombstones internally in a number of places, but we could expose
> more directly too. Typically, given a table like:
> {noformat}
> CREATE TABLE events (
> id text,
> created_at timestamp,
> content text,
> PRIMARY KEY (id, created_at)
> )
> {noformat}
> we could allow queries like:
> {noformat}
> DELETE FROM events WHERE id='someEvent' AND created_at < 'Jan 3, 2013';
> {noformat}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
