Fix undefined behaviour of char bit shifting when combining classic indices (bingmann#6)

Previously when combining multiple classic indices into a single classic
index, the contents of source indices are read in as `char`. During the
interleaving process, depending on the current position of the destination
index, both left and right shifts on the next char could be performed.

However, there are a few undefined behaviours that could affect the results
depending on the platform:
1. The signedness of a `char` is an undefined behaviour. Hence when
bit shifting, the usual arithmetic conversion performed on the char is
undefined. The char could be promoted to either signed int or unsigned
int.
2. If the char is treated as signed int, the bit shifting (both left and
right) is also undefined in pre-c++20 standards. The behaviour is platform
dependent.

This change fixes the issue by declare the contents read from source
indices as `unsigned char`.

Author: Zhicheng-Liu

cobs/construction/classic_index.cpp

@@ -247,7 +247,7 @@ void classic_combine_streams(
 
     // read many blocks from each file, interleave them into new block, and
     // write it out
-    std::vector<std::vector<char> > in_blocks(streams.size());
+    std::vector<std::vector<unsigned char> > in_blocks(streams.size());
     for (size_t i = 0; i < streams.size(); ++i) {
         in_blocks[i].resize(row_bytes[i] * batch_size);
     }
@@ -266,7 +266,7 @@ void classic_combine_streams(
         // read data from streams
         for (size_t i = 0; i < streams.size(); ++i) {
             streams[i].read(
-                in_blocks[i].data(), row_bytes[i] * this_batch);
+                    (char*)(in_blocks[i].data()), row_bytes[i] * this_batch);
             LOG << "stream[" << i << "] read " << streams[i].gcount();
             die_unequal(row_bytes[i] * this_batch,
                         static_cast<size_t>(streams[i].gcount()));

tests/classic_index_construction.cpp

@@ -5,11 +5,14 @@
  *
  * All rights reserved. Published under the MIT License in the LICENSE file.
  ******************************************************************************/
+#include <fstream>
+#include <algorithm>
 
 #include <fstream>
 #include <algorithm>
 
 #include "test_util.hpp"
+#include <cobs/file/classic_index_header.hpp>
 #include <cobs/query/classic_index/mmap_search_file.hpp>
 #include <cobs/util/calc_signature_size.hpp>
 #include <cobs/util/file.hpp>
@@ -24,6 +27,20 @@ static fs::path index_dir = base_dir / "index";
 static fs::path index_file = base_dir / "index.cobs_classic";
 static fs::path tmp_path = base_dir / "tmp";
 
+// Compare two classic indices. Return true if the bloom filters of both files are the same.
+bool compare_classic_indices(const std::string& filename1, const std::string& filename2)
+{
+    std::ifstream ifs1, ifs2;
+
+    cobs::deserialize_header<cobs::ClassicIndexHeader>(ifs1, filename1);
+    cobs::deserialize_header<cobs::ClassicIndexHeader>(ifs2, filename2);
+
+    std::istreambuf_iterator<char> begin1(ifs1);
+    std::istreambuf_iterator<char> begin2(ifs2);
+  
+    return std::equal(begin1,std::istreambuf_iterator<char>(),begin2); //Second argument is end-of-range iterator
+}
+
 // Compare two files. Return true if the contents of both files are the same.
 bool compare_files(const std::string& filename1, const std::string& filename2)
 {
@@ -220,4 +237,61 @@ TEST_F(classic_index_construction, combined_index_same_as_classic_constructed) {
     ASSERT_TRUE(compare_files(combined_index.string(), classic_constructed_index));
 }
 
+TEST_F(classic_index_construction, same_documents_combined_into_same_index) {
+    // This test starts with 18 copies of the same randomly generated document.
+    // These documents are split in 4 groups: g1 with 1 copy, g2 with 2 copies,
+    // g7 with 7 copies and g8 with 8 copies.
+    // A classic index is constructed through `cobs::classic_construct` for these
+    // 4 groups: c1, c2, c7 and c8.
+    // We then combine these 4 classic indices in this way: c1 + c8 = c1c8 and
+    // c2 + c7 = c2c7.
+    // The point of this test is to verify that c1c8 and c2c7 are the same.
+    using cobs::pad_index;
+    fs::create_directories(index_dir);
+    fs::create_directories(index_dir/pad_index(0));
+    fs::create_directories(index_dir/pad_index(1));
+    fs::create_directories(index_dir/pad_index(18));
+    fs::create_directories(index_dir/pad_index(27));
+
+    // prepare 4 groups of copies of a randomly generated document
+    std::string random_doc_src_string = cobs::random_sequence(1000, 1);
+    auto random_doc_one_copy = generate_documents_one(random_doc_src_string, 1);
+    auto random_doc_two_copies = std::vector<cobs::KMerBuffer<31> >(2, random_doc_one_copy[0]);
+    auto random_doc_seven_copies = std::vector<cobs::KMerBuffer<31> >(7, random_doc_one_copy[0]);
+    auto random_doc_eight_copies = std::vector<cobs::KMerBuffer<31> >(8, random_doc_one_copy[0]);
+    generate_test_case(random_doc_one_copy, "random_", input_dir/pad_index(1));
+    generate_test_case(random_doc_two_copies, "random_", input_dir/pad_index(2));
+    generate_test_case(random_doc_seven_copies, "random_", input_dir/pad_index(7));
+    generate_test_case(random_doc_eight_copies, "random_", input_dir/pad_index(8));
+
+    cobs::ClassicIndexParameters index_params;
+    index_params.false_positive_rate = 0.001; // in order to use large signature size
+    index_params.mem_bytes = 80;
+    index_params.num_threads = 1;
+    index_params.continue_ = true;
+
+    // generate a classic index for each document groups
+    cobs::classic_construct(cobs::DocumentList(input_dir/pad_index(1)),
+            index_dir/pad_index(18)/(pad_index(1) + ".cobs_classic"),
+            tmp_path, index_params);
+    cobs::classic_construct(cobs::DocumentList(input_dir/pad_index(8)),
+            index_dir/pad_index(18)/(pad_index(8) + ".cobs_classic"),
+            tmp_path, index_params);
+    cobs::classic_construct(cobs::DocumentList(input_dir/pad_index(2)),
+            index_dir/pad_index(27)/(pad_index(2) + ".cobs_classic"),
+            tmp_path, index_params);
+    cobs::classic_construct(cobs::DocumentList(input_dir/pad_index(7)),
+            index_dir/pad_index(27)/(pad_index(7) + ".cobs_classic"),
+            tmp_path, index_params);
+
+    // generate a combined index fro both classic constructed index
+    fs::path c1c8, c2c7;
+    cobs::classic_combine(index_dir/pad_index(18), index_dir/pad_index(0), c1c8,
+                          80, 1, false);
+    cobs::classic_combine(index_dir/pad_index(27), index_dir/pad_index(1), c2c7,
+                          80, 1, false);
+
+    ASSERT_TRUE(compare_classic_indices(c1c8.string(), c2c7.string()));
+}
+
 /******************************************************************************/