[
https://issues.apache.org/jira/browse/CASSANDRA-8099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14383633#comment-14383633
]
Benjamin Lerer commented on CASSANDRA-8099:
-------------------------------------------
As the patch is relatively large I have choose to split my review of the CQL
layer into chunks and give my comments for each chunk as soon as I have
finished reviewing it. I think it will make the things more manageable for
Sylvain and me.
For the first chunk I focused on the {{restrictions}}:
* I am not a big fan of big class hierachies but I wonder if it will not be
better to have two sub-classes for {{PrimaryKeyRestrictionSet}} one for the
partition key and one for the clustering columns rather than having a boolean
variable.
* In {{PrimaryKeyRestrictionSet}} the method {{addColumnFilterTo}} can be
simplified based on the fact that we know if the restrictions are on the
partition key components or on the clustering key columns.
* The {{AbstractPrimaryKeyRestrictions.toByteBuffers}} method can be moved down
as it is only used in {{PrimaryKeyRestrictionSet}}
* In {{MultiColumnRestriction}} the method {{isPartitionKey()}} is not used (in
case you have forgotten: {{MultiColumnRestriction}} only apply to clustering
key columns).
* I understand why you renamed {{?Restriction.Slice}} to
{{?Restriction.SliceRestriction}} but now the class names look a bit
inconsistent. May be we should rename the other classes too.
* In {{ColumnFilter}} the {{add(Expression expression)}} method is not used.
* In {{Operator}} the {{reverse}} method is not needed anymore and can be
removed.
* In {{StatementRestrictions}} I do not understand the use of {{useFiltering}}.
My understanding was that we should return an error message specifying that
{{ALLOW FILTERING}} is required and that this problem should have been handled
by {{checkNeedsFiltering}} in {{SelectStatement}}. Could you explain?
* In {{StatementRestrictions}} the {{nonPKRestrictedColumns}} method look wrong
to me as it can return some primary key columns.
> Refactor and modernize the storage engine
> -----------------------------------------
>
> Key: CASSANDRA-8099
> URL: https://issues.apache.org/jira/browse/CASSANDRA-8099
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Sylvain Lebresne
> Assignee: Sylvain Lebresne
> Fix For: 3.0
>
> Attachments: 8099-nit
>
>
> The current storage engine (which for this ticket I'll loosely define as "the
> code implementing the read/write path") is suffering from old age. One of the
> main problem is that the only structure it deals with is the cell, which
> completely ignores the more high level CQL structure that groups cell into
> (CQL) rows.
> This leads to many inefficiencies, like the fact that during a reads we have
> to group cells multiple times (to count on replica, then to count on the
> coordinator, then to produce the CQL resultset) because we forget about the
> grouping right away each time (so lots of useless cell names comparisons in
> particular). But outside inefficiencies, having to manually recreate the CQL
> structure every time we need it for something is hindering new features and
> makes the code more complex that it should be.
> Said storage engine also has tons of technical debt. To pick an example, the
> fact that during range queries we update {{SliceQueryFilter.count}} is pretty
> hacky and error prone. Or the overly complex ways {{AbstractQueryPager}} has
> to go into to simply "remove the last query result".
> So I want to bite the bullet and modernize this storage engine. I propose to
> do 2 main things:
> # Make the storage engine more aware of the CQL structure. In practice,
> instead of having partitions be a simple iterable map of cells, it should be
> an iterable list of row (each being itself composed of per-column cells,
> though obviously not exactly the same kind of cell we have today).
> # Make the engine more iterative. What I mean here is that in the read path,
> we end up reading all cells in memory (we put them in a ColumnFamily object),
> but there is really no reason to. If instead we were working with iterators
> all the way through, we could get to a point where we're basically
> transferring data from disk to the network, and we should be able to reduce
> GC substantially.
> Please note that such refactor should provide some performance improvements
> right off the bat but it's not it's primary goal either. It's primary goal is
> to simplify the storage engine and adds abstraction that are better suited to
> further optimizations.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
