blambov commented on code in PR #2625:
URL: https://github.com/apache/cassandra/pull/2625#discussion_r1327458815
##########
src/java/org/apache/cassandra/db/partitions/UnfilteredPartitionIterators.java:
##########
@@ -45,6 +45,7 @@ private UnfilteredPartitionIterators() {}
public interface MergeListener
{
+ public default boolean preserveOrder() { return true; }
Review Comment:
This needs a short javadoc/explanation.
`NOOP` below should be returning `false` for this.
##########
src/java/org/apache/cassandra/db/compaction/CompactionIterator.java:
##########
@@ -154,6 +154,12 @@ private UnfilteredPartitionIterators.MergeListener
listener()
{
return new UnfilteredPartitionIterators.MergeListener()
{
+ @Override
+ public boolean preserveOrder()
+ {
+ return type == OperationType.COMPACTION &&
controller.cfs.indexManager.hasIndexes();
Review Comment:
Make sure to update this to match the `return null` path in `listener` for
the other versions -- perhaps extract this to a method that both sites call?
##########
src/java/org/apache/cassandra/db/partitions/UnfilteredPartitionIterators.java:
##########
@@ -120,9 +121,16 @@ public void reduce(int idx, UnfilteredRowIterator current)
partitionKey = current.partitionKey();
isReverseOrder = current.isReverseOrder();
- // Note that because the MergeListener cares about it, we want
to preserve the index of the iterator.
- // Non-present iterator will thus be set to empty in
getReduced.
- toMerge.set(idx, current);
+ if (listener != null && listener.preserveOrder())
Review Comment:
As nothing stops `preserveOrder` from changing between this call and the one
in `getReduced` and `onKeyChange`, I would store the decision in a boolean and
use that in the latter calls.
Better still, could we call `preserveOrder` just once at construction?
##########
src/java/org/apache/cassandra/db/partitions/UnfilteredPartitionIterators.java:
##########
@@ -132,17 +140,20 @@ protected UnfilteredRowIterator getReduced()
? null
:
listener.getRowMergeListener(partitionKey, toMerge);
- // Make a single empty iterator object to merge, we don't need
toMerge.size() copiess
- UnfilteredRowIterator empty = null;
-
- // Replace nulls by empty iterators
- for (int i = 0; i < toMerge.size(); i++)
+ if (listener != null && listener.preserveOrder())
Review Comment:
We could also skip the empty iterators if `rowListener` turns out to be null.
As this is a bit more involved (getting this listener and setting up the
array needs to happen on the first `reduce` call), feel free to ignore this
comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
