From 3933c5648c016c752c11ab90d7ea3cbfab06d5b6 Mon Sep 17 00:00:00 2001
From: Eduardo Ramirez <sunjeets@netflix.com>
Date: Fri, 30 Aug 2024 08:39:06 -0700
Subject: [PATCH] Refactor set and map for copyBucketsFrom

---
 .../list/HollowListTypeDataElements.java      | 14 +++++++++++
 .../HollowListTypeDataElementsJoiner.java     | 22 ++++--------------
 .../HollowListTypeDataElementsSplitter.java   | 18 +++------------
 .../engine/map/HollowMapTypeDataElements.java | 18 +++++++++++++++
 .../map/HollowMapTypeDataElementsJoiner.java  | 22 +++---------------
 .../HollowMapTypeDataElementsSplitter.java    | 23 +++----------------
 .../engine/set/HollowSetTypeDataElements.java |  2 --
 ...ollowSetTypeDataElementsSplitJoinTest.java |  4 +++-
 8 files changed, 49 insertions(+), 74 deletions(-)

diff --git a/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElements.java b/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElements.java
index bba58719a..006dc888d 100644
--- a/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElements.java
+++ b/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElements.java
@@ -113,4 +113,18 @@ public long getStartElement(int ordinal) {
     public long getEndElement(int ordinal) {
         return listPointerData.getElementValue((long)ordinal * bitsPerListPointer, bitsPerListPointer);
     }
+
+    public void copyElementsFrom(long startElement, HollowListTypeDataElements src, long srcStartElement, long srcEndElement) {
+        if (bitsPerElement == src.bitsPerElement) {
+            // fast path can bulk copy elements
+            long numElements = srcEndElement - srcStartElement;
+            elementData.copyBits(src.elementData, srcStartElement * bitsPerElement, startElement * bitsPerElement, numElements * bitsPerElement);
+        } else {
+            for (long element=srcStartElement;element<srcEndElement;element++) {
+                long elementVal = src.elementData.getElementValue(element * src.bitsPerElement, src.bitsPerElement);
+                elementData.setElementValue(startElement * bitsPerElement, bitsPerElement, elementVal);
+                startElement++;
+            }
+        }
+    }
 }
diff --git a/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElementsJoiner.java b/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElementsJoiner.java
index e9a97f89d..470229ae8 100644
--- a/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElementsJoiner.java
+++ b/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElementsJoiner.java
@@ -56,27 +56,15 @@ public void copyRecords() {
             int fromIndex = ordinal & fromMask;
             int fromOrdinal = ordinal >> fromOrdinalShift;
 
-            if (fromOrdinal <= from[fromIndex].maxOrdinal) {
+            if (fromOrdinal <= from[fromIndex].maxOrdinal) { // else lopsided shard for e.g. when consumers skip type shards with no additions
                 HollowListTypeDataElements source = from[fromIndex];
                 long startElement = source.getStartElement(fromOrdinal);
                 long endElement = source.getEndElement(fromOrdinal);
 
-                if (source.bitsPerElement == to.bitsPerElement) {
-                    // fastpath can bulk copy elements. emptyBucketValue is same since bitsPerElement is same
-                    long numElements = endElement - startElement;
-                    int bitsPerElement = source.bitsPerElement;
-                    to.elementData.copyBits(source.elementData, startElement * bitsPerElement, elementCounter * bitsPerElement, numElements * bitsPerElement);
-                    elementCounter += numElements;
-                } else {
-                    for (long element = startElement; element < endElement; element++) {
-                        int elementOrdinal = (int) source.elementData.getElementValue(element * source.bitsPerElement, source.bitsPerElement);
-                        to.elementData.setElementValue(elementCounter * to.bitsPerElement, to.bitsPerElement, elementOrdinal);
-                        elementCounter++;
-                    }
-                }
-            } // else: lopsided shard for e.g. when consumers skip type shards with no additions.
-              //       nothing is written to elementData and the cached value of elementCounter is written to listPointerData.
-
+                long numElements = endElement - startElement;
+                to.copyElementsFrom(elementCounter, source, startElement, endElement);
+                elementCounter += numElements;
+            }
             to.listPointerData.setElementValue((long) to.bitsPerListPointer * ordinal, to.bitsPerListPointer, elementCounter);
         }
     }
diff --git a/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElementsSplitter.java b/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElementsSplitter.java
index b26aec70c..6562ac574 100644
--- a/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElementsSplitter.java
+++ b/hollow/src/main/java/com/netflix/hollow/core/read/engine/list/HollowListTypeDataElementsSplitter.java
@@ -71,21 +71,9 @@ public void copyRecords() {
             long endElement = from.getEndElement(ordinal);
             HollowListTypeDataElements target = to[toIndex];
 
-            if (from.bitsPerElement == target.bitsPerElement) {
-                // fast path can bulk copy elements. emptyBucketValue is same since bitsPerElement is same
-                long numElements = endElement - startElement;
-                int bitsPerElement = from.bitsPerElement;
-                target.elementData.copyBits(from.elementData, startElement * bitsPerElement, elementCounter[toIndex] * bitsPerElement, numElements * bitsPerElement);
-                elementCounter[toIndex] += numElements;
-            } else {
-                // slow path(but more compact) not exercised until populateSats above supports split shard specific bitsPerElement
-                // (which would make sense to add once HollowListTypeWriteState's gatherStatistics supports assigning bitsPerElement at a shard level)
-                for (long element=startElement;element<endElement;element++) {
-                    int elementOrdinal = (int)from.elementData.getElementValue(element * from.bitsPerElement, from.bitsPerElement);
-                    target.elementData.setElementValue(elementCounter[toIndex] * target.bitsPerElement, target.bitsPerElement, elementOrdinal);
-                    elementCounter[toIndex]++;
-                }
-            }
+            long numElements = endElement - startElement;
+            target.copyElementsFrom(elementCounter[toIndex], from, startElement, endElement);
+            elementCounter[toIndex] += numElements;
 
             target.listPointerData.setElementValue((long) target.bitsPerListPointer * toOrdinal, target.bitsPerListPointer, elementCounter[toIndex]);
         }
diff --git a/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElements.java b/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElements.java
index 1586fb3e5..72562f255 100644
--- a/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElements.java
+++ b/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElements.java
@@ -134,4 +134,22 @@ int getBucketValueByAbsoluteIndex(long absoluteBucketIndex) {
         return (int)entryData.getElementValue((absoluteBucketIndex * bitsPerMapEntry) + bitsPerKeyElement, bitsPerValueElement);
     }
 
+    public void copyBucketsFrom(long startBucket, HollowMapTypeDataElements src, long srcStartBucket, long srcEndBucket) {
+        if (bitsPerKeyElement == src.bitsPerKeyElement && bitsPerValueElement == src.bitsPerValueElement) {
+            // fast path can bulk copy buckets. emptyBucketKeyValue is same since bitsPerKeyElement is the same
+            long numBuckets = srcEndBucket - srcStartBucket;
+            entryData.copyBits(src.entryData, srcStartBucket * bitsPerMapEntry, startBucket * bitsPerMapEntry, numBuckets * bitsPerMapEntry);
+        } else {
+            for (long bucket=srcStartBucket;bucket<srcEndBucket;bucket++) {
+                long bucketKey = src.entryData.getElementValue(bucket * src.bitsPerMapEntry, src.bitsPerKeyElement);
+                long bucketValue = src.entryData.getElementValue(bucket * src.bitsPerMapEntry + src.bitsPerKeyElement, src.bitsPerValueElement);
+                if(bucketKey == src.emptyBucketKeyValue)
+                    bucketKey = emptyBucketKeyValue;
+                long targetBucketOffset = startBucket * bitsPerMapEntry;
+                entryData.setElementValue(targetBucketOffset, bitsPerKeyElement, bucketKey);
+                entryData.setElementValue(targetBucketOffset + bitsPerKeyElement, bitsPerValueElement, bucketValue);
+                startBucket++;
+            }
+        }
+    }
 }
diff --git a/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElementsJoiner.java b/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElementsJoiner.java
index e9999b7be..5338c93a1 100644
--- a/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElementsJoiner.java
+++ b/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElementsJoiner.java
@@ -77,29 +77,13 @@ public void copyRecords() {
             if (fromOrdinal <= from[fromIndex].maxOrdinal) { // else lopsided shards resulting from skipping type shards with no additions, mapSize remains 0
                 long startBucket = source.getStartBucket(fromOrdinal);
                 long endBucket = source.getEndBucket(fromOrdinal);
-                long numBuckets = endBucket - startBucket;
 
-                if (to.bitsPerKeyElement == source.bitsPerKeyElement && to.bitsPerValueElement == source.bitsPerValueElement) {
-                    // emptyBucketKeyValue will also be uniform
-                    long bitsPerMapEntry = to.bitsPerMapEntry;
-                    to.entryData.copyBits(source.entryData, startBucket * bitsPerMapEntry, bucketCounter * bitsPerMapEntry, numBuckets * bitsPerMapEntry);
-                    bucketCounter += numBuckets;
-                } else {
-                    for (long bucket=startBucket;bucket<endBucket;bucket++) {
-                        long bucketKey = source.entryData.getElementValue(bucket * source.bitsPerMapEntry, source.bitsPerKeyElement);
-                        long bucketValue = source.entryData.getElementValue(bucket * source.bitsPerMapEntry + source.bitsPerKeyElement, source.bitsPerValueElement);
-                        if (bucketKey == source.emptyBucketKeyValue)
-                            bucketKey = to.emptyBucketKeyValue; // since empty bucket key value can be non-uniform across shards
-                        long targetBucketOffset = bucketCounter * to.bitsPerMapEntry;
-                        to.entryData.setElementValue(targetBucketOffset, to.bitsPerKeyElement, bucketKey);
-                        to.entryData.setElementValue(targetBucketOffset + to.bitsPerKeyElement, to.bitsPerValueElement, bucketValue);
-                        bucketCounter ++;
-                    }
-                }
+                long numBuckets = endBucket - startBucket;
+                to.copyBucketsFrom(bucketCounter, source, startBucket, endBucket);
+                bucketCounter += numBuckets;
 
                 mapSize = source.mapPointerAndSizeData.getElementValue((long) (fromOrdinal * source.bitsPerFixedLengthMapPortion) + source.bitsPerMapPointer, source.bitsPerMapSizeValue);
             }
-
             to.mapPointerAndSizeData.setElementValue( (long) ordinal * to.bitsPerFixedLengthMapPortion, to.bitsPerMapPointer, bucketCounter);
             to.mapPointerAndSizeData.setElementValue((long) (ordinal * to.bitsPerFixedLengthMapPortion) + to.bitsPerMapPointer, to.bitsPerMapSizeValue, mapSize);
         }
diff --git a/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElementsSplitter.java b/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElementsSplitter.java
index 5bc8c7628..65693340a 100644
--- a/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElementsSplitter.java
+++ b/hollow/src/main/java/com/netflix/hollow/core/read/engine/map/HollowMapTypeDataElementsSplitter.java
@@ -78,26 +78,9 @@ public void copyRecords() {
             long startBucket = from.getStartBucket(ordinal);
             long endBucket = from.getEndBucket(ordinal);
 
-            if (target.bitsPerKeyElement == from.bitsPerKeyElement && target.bitsPerValueElement == from.bitsPerValueElement) {
-                // fast path can bulk copy buckets. emptyBucketKeyValue is same since bitsPerKeyElement is the same
-                long numBuckets = endBucket - startBucket;
-                long bitsPerMapEntry = from.bitsPerMapEntry;
-                target.entryData.copyBits(from.entryData, startBucket * bitsPerMapEntry, bucketCounter[toIndex] * bitsPerMapEntry, numBuckets * bitsPerMapEntry);
-                bucketCounter[toIndex] += numBuckets;
-            } else {
-                // slow path(but more compact) not exercised until populateSats above supports split shard specific bitsPerKeyElement and bitsPerValueElement
-                // (which would make sense to add once HollowMapTypeWriteState's gatherStatistics supports assigning bitsPerKeyElement and bitsPerValueElement at a shard level)
-                for (long bucket=startBucket;bucket<endBucket;bucket++) {
-                    long bucketKey = from.entryData.getElementValue(bucket * from.bitsPerMapEntry, from.bitsPerKeyElement);
-                    long bucketValue = from.entryData.getElementValue(bucket * from.bitsPerMapEntry + from.bitsPerKeyElement, from.bitsPerValueElement);
-                    if(bucketKey == from.emptyBucketKeyValue)
-                        bucketKey = target.emptyBucketKeyValue;
-                    long targetBucketOffset = (bucketCounter[toIndex] * target.bitsPerMapEntry);
-                    target.entryData.setElementValue(targetBucketOffset, target.bitsPerKeyElement, bucketKey);
-                    target.entryData.setElementValue(targetBucketOffset + target.bitsPerKeyElement, target.bitsPerValueElement, bucketValue);
-                    bucketCounter[toIndex]++;
-                }
-            }
+            long numBuckets = endBucket - startBucket;
+            target.copyBucketsFrom(bucketCounter[toIndex], from, startBucket, endBucket);
+            bucketCounter[toIndex] += numBuckets;
 
             target.mapPointerAndSizeData.setElementValue((long) toOrdinal * target.bitsPerFixedLengthMapPortion, target.bitsPerMapPointer, bucketCounter[toIndex]);
             long mapSize = from.mapPointerAndSizeData.getElementValue((long) (ordinal * from.bitsPerFixedLengthMapPortion) + from.bitsPerMapPointer, from.bitsPerMapSizeValue);
diff --git a/hollow/src/main/java/com/netflix/hollow/core/read/engine/set/HollowSetTypeDataElements.java b/hollow/src/main/java/com/netflix/hollow/core/read/engine/set/HollowSetTypeDataElements.java
index f2a267932..fb2fbf553 100644
--- a/hollow/src/main/java/com/netflix/hollow/core/read/engine/set/HollowSetTypeDataElements.java
+++ b/hollow/src/main/java/com/netflix/hollow/core/read/engine/set/HollowSetTypeDataElements.java
@@ -132,12 +132,10 @@ public void copyBucketsFrom(long startBucket, HollowSetTypeDataElements src, lon
             long numBuckets = srcEndBucket - srcStartBucket;
             elementData.copyBits(src.elementData, srcStartBucket * bitsPerElement, startBucket * bitsPerElement, numBuckets * bitsPerElement);
         } else {
-            // one bucket at a time
             for (long bucket=srcStartBucket;bucket<srcEndBucket;bucket++) {
                 long bucketVal = src.elementData.getElementValue(bucket * src.bitsPerElement, src.bitsPerElement);
                 if(bucketVal == src.emptyBucketValue)
                     bucketVal = emptyBucketValue;
-                // SNAP: TODO: empty bucket should be encoded with all 1s, not copied as-is - in all collection spliitters/joiners
                 elementData.setElementValue(startBucket * bitsPerElement, bitsPerElement, bucketVal);
                 startBucket++;
             }
diff --git a/hollow/src/test/java/com/netflix/hollow/core/read/engine/set/HollowSetTypeDataElementsSplitJoinTest.java b/hollow/src/test/java/com/netflix/hollow/core/read/engine/set/HollowSetTypeDataElementsSplitJoinTest.java
index e8b801c17..e32eb753a 100644
--- a/hollow/src/test/java/com/netflix/hollow/core/read/engine/set/HollowSetTypeDataElementsSplitJoinTest.java
+++ b/hollow/src/test/java/com/netflix/hollow/core/read/engine/set/HollowSetTypeDataElementsSplitJoinTest.java
@@ -17,7 +17,9 @@ public class HollowSetTypeDataElementsSplitJoinTest extends AbstractHollowSetTyp
     @Test
     public void testSplittingAndJoiningWithSnapshotBlob() throws Exception {
 
-        // SNAP: TODO: cleanup this whole method
+        // SNAP: TODO: duplicate this test
+
+        // SNAP: TODO: cleanup this whole method.
         String blobPath = "/Users/ssingh/workspace/blob-cache/vms-daintree/prod/"; // null; // dir where snapshot blob exists for e.g. "/tmp/";
         long v = 20230611133921525l; // 0l; // snapshot version for e.g. 20230915162636001l;
         String[] setTypesWithOneShard = {"SetOfContractRestriction", "SetOfContractAsset", "SetOfDashStreamBoxInfo",