Add check of hashed data when writing new data

[peytondmurray]

This PR adds a check when writing data to a file. This check verifies that chunks of data that already exist in the hashtable correspond to arrays that match the data we are trying to write.

This should incur a performance overhead when writing datasets that reuse chunks, but ensures we won't be corrupting hashtables in the future. Closes #294.

Note: Currently I'm seeing a test failure locally with test_delete_string_dataset on master. The failure happens when trying to open the file after running h5repack on the hdf5 file, with OSError: Unable to open file (bad object header version number). I'm opening this PR to see if CI sees it.

[ArvidJB]

The same check should also go into write_dataset_chunks.

Can you add a test where the hash function is mocked to be one of the previous data_version hash functions? The check should get triggered.

[peytondmurray]

Okay, this is really strange. The error in the test that I mentioned above is happening because h5repack segfaults when it is called in the subprocess for that test. Here's the file itself - all it has is a few strings in an array.

file.hdf5.zip

When I run h5repack -v 2 <test_data.hdf5> <repacked.hdf5.tmp> I can see that the segfault arises after handling prev_version:

Making new file ...
-----------------------------------------------------------------
 Type     Filter (Compression)        Timing read/write    Name
-----------------------------------------------------------------
 group                                                    /
 group                                                    /_version_data
 group                                                    /_version_data/foo
  attr                         1.470000e-06/1.889999e-06   largest_index
 dset                         0.000000e+00/5.443900e-05   /_version_data/foo/hash_table
 dset                         5.605000e-05/5.415900e-05   /_version_data/foo/raw_data
  attr                         1.119999e-06/1.580000e-06   chunks
 group                                                    /_version_data/versions
  attr                         9.500000e-07/1.270000e-06   data_version
  attr                         3.010000e-06/2.629999e-06   current_version
 group                                                    /_version_data/versions/__first_version__
  attr                         2.229999e-06/4.030000e-06   timestamp
 group                                                    /_version_data/versions/r1
  attr                         1.290000e-06/1.439999e-06   committed
  attr                         2.519999e-06/4.039999e-06   timestamp
  attr                         1.630000e-06/2.030001e-06   prev_version
Segmentation fault (core dumped)

[peytondmurray]

Looks like there must be a problem locally with h5repack that is responsible for the failing test I'm seeing, and anyway it isn't a failing test related to this change.

---

Changes

• Removed a little dead code
• Changed the logic slightly to make it more readable by removing the usage of setdefault. This let me get the clarity I needed to figure out how to carry out the check of the hashed data.
• Added check of the hashed data
• Moved some code inside the branch statement that only needed to be run on one branch

[ArvidJB]

Looking pretty good!

• Can you add a test which uses the old hash functions from data_version 2, 3?
• Add a test with NaN values, since I think assert_array_equal will not handle those correctly by default?
• Add a test with strings to check we correctly compare encoded bytes vs. strings

[peytondmurray]

Okay, I think we have our bases covered with these tests.

[ArvidJB]

Looks good to me!