CASSANDRA-14797 - CQLSSTableWriter does not support DELETE #1461
Conversation
yifan-c
left a comment
yifan-c
left a comment
There was a problem hiding this comment.
LGTM.
Only have a few cosmetic nits.
There was a problem hiding this comment.
nit: if slices.isEmpty(), it would be unnecessary to create iterators from keys and slices, just like the other call-sites of createSlices
There was a problem hiding this comment.
At first blush, I think it's impossible for createSlices to return an empty set of slices for the use-cases we are supporting (at least all unit tests that hit this have at least one slice), which is I'm assuming why Alex left the check off in his original patch. If slices could be empty, we'd end up having a no-key delete, which wouldn't make any sense (although it would also just skip this whole block anyway, so it wouldn't add a delete).
There was a problem hiding this comment.
nit: it looks like to disable read-before-write for the partition, the most suitable value to be supplied is null instead of empty map, which is fine, but just not necessary. org.apache.cassandra.cql3.UpdateParameters#getPrefetchedRow checks whether the field value is null.
I do not know how empty map is used historically.
There was a problem hiding this comment.
Yeah - the empty map has been there since the code was originally written, and it's always essentially been a no-op from what I can see. Not sure if we want to change it in this PR/change though (it adds one extra call to EmptyMap.get, which always returns null, so I don't think it's functionally that big a deal, and if we do want to change it I'd like to keep it separated from the rest of these changes and test it to make sure I'm not missing something.
There was a problem hiding this comment.
Counter modification statement?
3 participants
No description provided.