Thanks for the review.
0xFF looks suspicious to me - should perhaps be 0xFFFF?
Correct, good catch (pushed a fix to the same branch as before).
It looks like we're interning TimeUUIDType column identifiers
I don't think that's the case but we may have to be a little more precise here. Bug notwithstanding, we only intern names that are part of the column_metadata in thrift parlance. Now thrift does let you declare a column_metadata that is a timeUUID (it's useless and probably nobody does it, but it's possible) but even in that case, we'll only intern the timeUUID that were declared. We do not intern dynamic names in particular if that is what you meant (contrarily to the current code I might add).
CellName is confusing, as it isn't a name-of-a-cell, but a Cell-with-a-name
Well, actually no. It is a name-of-a-cell. At that point, I need to add that what we refer as 'cell' nowadays is what is currently the Column class. Obviously, the naming would be a lot more sensible if the Column class was actually named Cell, and we plan on changing that (
CASSANDRA-6063), but I didn't wanted to include it in this patch-set because it's big enough as it is ( CASSANDRA-6063 is marked 3.0 but I'm definitively for doing at least the Column->Cell renaming along with this patch (though I'd leave the rest of CASSANDRA-6063 for later)). Anyway, this also means than in your "nits" patch, I'd rather not do the cellNameFromByteBuffer->cellFromByteBuffer renaming.
Compound(.*)CellName was also a little confusing (to me, at least), suggest either Prefixed/Clustered and (Blank)/Unclustered
Can't say I'm overjoy with Compound myself. However, Prefixed and/or Clustered doesn't really apply here (a "simple" cell name is not more or less prefixed/clustered than a compound one) so I don't think it's better. The difference between Simple and Compound (using the naming of the patch) is largely an implementation detail, namely whether the underlying encoding starts by 'number of components' or not (I would have loved not leaking the isCompound() method in the CellName interface in particular, to really nail the point that it's an implementation detail (for backward compatibility) but there still is a handful of places where we kind of need it so I've let it be).
Remove Sparse from *SparseCellName - misleading, really it is just !Dense, so leave out Dense from the name
At the risk of sounding obtuse, I've tried that initially and I kind of like it the way it is. Let me note that the Sparse naming doesn't really leak outside the 'composite' packages, most of the code just use the 'isDense' method of CellName and that's it. But as far as the implementations of CellName goes, I think SimpleCellName would suggest it's somewhat a super-class of SimpleDenseCellName which it's not (see the point below too). We do have 4 largely orthogonal layout and I like making that very explicit in the implementation names. Probably a matter of personal preference though.
I would possibly suggest removing CellName interface, and having SimpleSparseCellName be simply NamedCell
That would be wrong imo. There is no meaningful inheritance relations between the cell names implementations (each pair of implementations do share a number of characteristics, which is why there is a few Abstract classes to avoid code duplication, but at the concrete implementation level there is no inheritance relation). I also like having CellName being an interface because that allows to cleanly have a few specific implementations like the EMPTY one (and the "FakeCellName" from ColumnSlice, though that one is largely a hack so it's probably a bad example).
Also, suggest renaming CompositeBound to one of BoundedComposite
Changed toByteBuffer to always return a ByteBuffer that cannot affect the state of the callee, and modified callers that were using .duplicate() to no longer do so
I'd rather not. We have an implicit rule in the code base that a caller should never expect it can modify the state of a ByteBuffer and it should call duplicate() if it needs to (one goal being to avoid defensive duplicate() when it's not needed since most of the code is written in a way that don't modify the ByteBuffers anyway) so the patch is consistent with the code base in that respect. Besides, in this particular case, thrift queries will call toByteBuffer() all the time without needing the duplication so leaving it to the caller does save some.
Following code comment in CompositeType appears to be incomplete
Definitively not the most clear comment but as far as I can tell it's not incomplete. Can you clarify what you mean by "it suggests different outcomes for same spec".