[
https://issues.apache.org/jira/browse/CASSANDRA-9888?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14643963#comment-14643963
]
Branimir Lambov commented on CASSANDRA-9888:
--------------------------------------------
The code looks good. Several nits:
[BTree.build|https://github.com/apache/cassandra/compare/trunk...belliottsmith:9888#diff-4b911b7d0959c6219175e2349968f3cdR104]:
Any reason for this change? AFAICS it only makes a negative size error a tad
harder to identify.
[BTree.transformAndFilter|https://github.com/apache/cassandra/compare/trunk...belliottsmith:9888#diff-4b911b7d0959c6219175e2349968f3cdR624]:
Extracting a boolean for isLeaf would make this a bit more readable.
[BTreeBackedRow.Builder.isSorted|https://github.com/apache/cassandra/compare/trunk...belliottsmith:9888#diff-b81748ca244377652fa8f06b79b78567R474]:
Is this true on purpose? It defies the purpose of the only use of the method I
can find, I'd rather remove it altogether.
[BTree.Builder.build|https://github.com/apache/cassandra/compare/trunk...belliottsmith:9888#diff-4b911b7d0959c6219175e2349968f3cdR957]:
It appears that {{sortedAndFiltered()}} does not really mean what it says, as
duplicates could be part of the given data at least as used by
{{BTreeBackedRow.Builder}}. Since we only apply resolver at the end, and do
that selectively, it also seems to me that specifying it initially is not a
very good idea. I think a sequence of
{{builder().assumeSorted().permitDuplicates()...resolveDuplicates(resolver).build()}}
or even {{builder().nonincremental()...resolveDuplicates(resolver).build()}} /
{{builder().nonincremental()...assumeSortedAndFiltered().build()}} would carry
a clearer meaning.
[ComplexColumnData.filter|https://github.com/apache/cassandra/compare/trunk...belliottsmith:9888#diff-bc19f192ef82fbca9abd27526054bb0fR151]:
This is probably the correct indentation according to the rules, but it breaks
up the statement confusingly. I'd move {{(cell) ->}} as well as {{? cell :
null}} to separate lines and indent each of the three conjuncts a little
further.
Nice to have: [BTree.iterator(...,
true)|https://github.com/apache/cassandra/compare/trunk...belliottsmith:9888#diff-bc19f192ef82fbca9abd27526054bb0fR102]
made me look the method up to understand what the boolean means. Could we have
an enumeration for direction, so that this is obvious? The enumeration could
include some constants and/or methods to make implementing directed logic
easier.
> BTreeBackedRow and ComplexColumnData
> ------------------------------------
>
> Key: CASSANDRA-9888
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9888
> Project: Cassandra
> Issue Type: Improvement
> Components: Core
> Reporter: Benedict
> Assignee: Benedict
> Fix For: 3.0 beta 1
>
>
> I found ArrayBackedRow a little hard to follow, especially around building,
> so I've converted it to BTreeBackedRow, along with ComplexColumnData. Both
> now rely on BTree.Builder, which introduces a little extra functionality to
> permit these classes to be implemented more declaratively. Transformations of
> these classes are also now uniform and more declarative, also depending on
> some new functionality in BTree that permits applying a
> transformation/filtration to an existing btree (this could be optimised at a
> later date, but should suffice for now).
> The result is IMO both clearer and should scale more gracefully to larger
> numbers of columns and complex cells.
> This hasn't taken all of the possible improvements of the back of this change
> to their natural conclusion, as we are somewhat time pressed and I would
> prefer to get the ball rolling with this first round.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
