[GitHub] [cassandra] blambov commented on a diff in pull request #2625: CASSANDRA-18773 4.0 compaction speedup

["blambov (via GitHub)" <gi...@apache.org>]

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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org