Merge pull request #447 from apache/fix_bit_packing

Fix bit packing

Author: AlexanderSaydakov

theta/include/bit_packing.hpp

@@ -329,7 +329,7 @@ static inline void pack_bits_13(const uint64_t* values, uint8_t* ptr) {
 
   *ptr++ = static_cast<uint8_t>(values[3] >> 4);
 
-  *ptr = static_cast<uint8_t>(values[3] >> 4);
+  *ptr = static_cast<uint8_t>(values[3] << 4);
   *ptr++ |= static_cast<uint8_t>(values[4] >> 9);
 
   *ptr++ = static_cast<uint8_t>(values[4] >> 1);
@@ -4227,7 +4227,7 @@ static inline void unpack_bits_33(uint64_t* values, const uint8_t* ptr) {
   values[6] |= *ptr >> 1;
 
   values[7] = static_cast<uint64_t>(*ptr++ & 1) << 32;
-  values[7] |= *ptr++ << 24;
+  values[7] |= static_cast<uint64_t>(*ptr++) << 24;
   values[7] |= *ptr++ << 16;
   values[7] |= *ptr++ << 8;
   values[7] |= *ptr;
@@ -4296,7 +4296,7 @@ static inline void unpack_bits_35(uint64_t* values, const uint8_t* ptr) {
   values[1] |= *ptr++ << 6;
   values[1] |= *ptr >> 2;
 
-  values[2] = static_cast<uint64_t>(*ptr++ & 2) << 33;
+  values[2] = static_cast<uint64_t>(*ptr++ & 3) << 33;
   values[2] |= static_cast<uint64_t>(*ptr++) << 25;
   values[2] |= *ptr++ << 17;
   values[2] |= *ptr++ << 9;

theta/test/bit_packing_test.cpp

@@ -29,51 +29,53 @@ namespace datasketches {
 static const uint64_t IGOLDEN64 = 0x9e3779b97f4a7c13ULL;
 
 TEST_CASE("pack unpack bits") {
-  for (uint8_t bits = 1; bits <= 63; ++bits) {
-    int n = 8;
-    const uint64_t mask = (1ULL << bits) - 1;
-    std::vector<uint64_t> input(n, 0);
-    const uint64_t igolden64 = IGOLDEN64;
-    uint64_t value = 0xaa55aa55aa55aa55ULL; // arbitrary starting value
-    for (int i = 0; i < n; ++i) {
-      input[i] = value & mask;
-      value += igolden64;
-    }
-    std::vector<uint8_t> bytes(n * sizeof(uint64_t), 0);
-    uint8_t offset = 0;
-    uint8_t* ptr = bytes.data();
-    for (int i = 0; i < n; ++i) {
-      offset = pack_bits(input[i], bits, ptr, offset);
-    }
+  uint64_t value = 0xaa55aa55aa55aa55ULL; // arbitrary starting value
+  for (int m = 0; m < 100; ++m) {
+    for (uint8_t bits = 1; bits <= 63; ++bits) {
+      int n = 8;
+      const uint64_t mask = (1ULL << bits) - 1;
+      std::vector<uint64_t> input(n, 0);
+      for (int i = 0; i < n; ++i) {
+        input[i] = value & mask;
+        value += IGOLDEN64;
+      }
+      std::vector<uint8_t> bytes(n * sizeof(uint64_t), 0);
+      uint8_t offset = 0;
+      uint8_t* ptr = bytes.data();
+      for (int i = 0; i < n; ++i) {
+        offset = pack_bits(input[i], bits, ptr, offset);
+      }
 
-    std::vector<uint64_t> output(n, 0);
-    offset = 0;
-    const uint8_t* cptr = bytes.data();
-    for (int i = 0; i < n; ++i) {
-      offset = unpack_bits(output[i], bits, cptr, offset);
-    }
-    for (int i = 0; i < n; ++i) {
-      REQUIRE(input[i] == output[i]);
+      std::vector<uint64_t> output(n, 0);
+      offset = 0;
+      const uint8_t* cptr = bytes.data();
+      for (int i = 0; i < n; ++i) {
+        offset = unpack_bits(output[i], bits, cptr, offset);
+      }
+      for (int i = 0; i < n; ++i) {
+        REQUIRE(input[i] == output[i]);
+      }
     }
   }
 }
 
 TEST_CASE("pack unpack blocks") {
-  for (uint8_t bits = 1; bits <= 63; ++bits) {
-    const uint64_t mask = (1ULL << bits) - 1;
-    std::vector<uint64_t> input(8, 0);
-    const uint64_t igolden64 = IGOLDEN64;
-    uint64_t value = 0xaa55aa55aa55aa55ULL; // arbitrary starting value
-    for (int i = 0; i < 8; ++i) {
-      input[i] = value & mask;
-      value += igolden64;
-    }
-    std::vector<uint8_t> bytes(8 * sizeof(uint64_t), 0);
-    pack_bits_block8(input.data(), bytes.data(), bits);
-    std::vector<uint64_t> output(8, 0);
-    unpack_bits_block8(output.data(), bytes.data(), bits);
-    for (int i = 0; i < 8; ++i) {
-      REQUIRE((input[i] & mask) == output[i]);
+  uint64_t value = 0xaa55aa55aa55aa55ULL; // arbitrary starting value
+  for (int n = 0; n < 100; ++n) {
+    for (uint8_t bits = 1; bits <= 63; ++bits) {
+      const uint64_t mask = (1ULL << bits) - 1;
+      std::vector<uint64_t> input(8, 0);
+      for (int i = 0; i < 8; ++i) {
+        input[i] = value & mask;
+        value += IGOLDEN64;
+      }
+      std::vector<uint8_t> bytes(bits, 0);
+      pack_bits_block8(input.data(), bytes.data(), bits);
+      std::vector<uint64_t> output(8, 0);
+      unpack_bits_block8(output.data(), bytes.data(), bits);
+      for (int i = 0; i < 8; ++i) {
+        REQUIRE(input[i] == output[i]);
+      }
     }
   }
 }