[
https://issues.apache.org/jira/browse/CASSANDRA-5417?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13831351#comment-13831351
]
Sylvain Lebresne commented on CASSANDRA-5417:
---------------------------------------------
Thanks for the review.
bq. 0xFF looks suspicious to me - should perhaps be 0xFFFF?
Correct, good catch (pushed a fix to the same branch as before).
bq. 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).
bq. 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.
bq. 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).
{quote}
Remove Sparse from *SparseCellName - misleading, really it is just !Dense, so
leave out Dense from the name
{quote}
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.
bq. 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).
bq. Also, suggest renaming CompositeBound to one of BoundedComposite
Good idea.
bq. 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.
bq. 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".
> Push composites support in the storage engine
> ---------------------------------------------
>
> Key: CASSANDRA-5417
> URL: https://issues.apache.org/jira/browse/CASSANDRA-5417
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Sylvain Lebresne
> Assignee: Sylvain Lebresne
> Labels: performance
> Fix For: 2.1
>
>
> CompositeType happens to be very useful and is now widely used: CQL3 heavily
> rely on it, and super columns are now using it too internally. Besides,
> CompositeType has been advised as a replacement of super columns on the
> thrift side for a while, so it's safe to assume that it's generally used
> there too.
> CompositeType has initially been introduced as just another AbstractType.
> Meaning that the storage engine has no nothing whatsoever of composites
> being, well, composite. This has the following drawbacks:
> * Because internally a composite value is handled as just a ByteBuffer, we
> end up doing a lot of extra work. Typically, each time we compare 2 composite
> value, we end up "deserializing" the components (which, while it doesn't copy
> data per-se because we just slice the global ByteBuffer, still waste some cpu
> cycles and allocate a bunch of ByteBuffer objects). And since compare can be
> called *a lot*, this is likely not negligible.
> * This make CQL3 code uglier than necessary. Basically, CQL3 makes extensive
> use of composites, and since it gets backs ByteBuffer from the internal
> columns, it always have to check if it's actually a compositeType or not, and
> then split it and pick the different parts it needs. It's only an API
> problem, but having things exposed as composites directly would definitively
> make thinks cleaner. In particular, in most cases, CQL3 don't care whether it
> has a composite with only one component or a non-really-composite value, but
> we still always distinguishes both cases. Lastly, if we do expose composites
> more directly internally, it's not a lot more work to "internalize" better
> the different parts of the cell name that CQL3 uses (what's the clustering
> key, what's the actuall CQL3 column name, what's the collection element),
> making things cleaner. Last but not least, there is currently a bunch of
> places where methods take a ByteBuffer as argument and it's hard to know
> whether it expects a cell name or a CQL3 column name. This is pretty error
> prone.
> * It makes it hard (or impossible) to do a number of performance
> improvements. Consider CASSANDRA-4175, I'm not really sure how you can do it
> properly (in memory) if cell names are just ByteBuffer (since CQL3 column
> names are just one of the component in general). But we also miss
> oportunities of sharing prefixes. If we were able to share prefixes of
> composite names in memory we would 1) lower the memory footprint and 2)
> potentially speed-up comparison (of the prefixes) by checking reference
> equality first (also, doing prefix sharing on-disk, which is a separate
> concern btw, might be easier to do if we do prefix sharing in memory).
> So I suggest pushing CompositeType support inside the storage engine. What I
> mean by that concretely would be change the internal {{Column.name}} from
> ByteBuffer to some CellName type. A CellName would API-wise just be a list of
> ByteBuffer. But in practice, we'd have a specific CellName implementation for
> not-really-composite names, and the truly composite implementation will allow
> some prefix sharing. From an external API however, nothing would change, we
> would pack the composite as usual before sending it back to the client, but
> at least internally, comparison won't have to deserialize the components
> every time, and CQL3 code will be cleaner.
--
This message was sent by Atlassian JIRA
(v6.1#6144)
