Fix bug in tdb matrix where we would read even when the array is empty and get nan values (#368)

Author: jparismorgan

src/include/detail/linalg/tdb_matrix.h

@@ -239,40 +239,35 @@ class tdbBlockedMatrix : public MatrixBase {
       throw std::runtime_error("Cell order and matrix order must match");
     }
 
-    // @todo Maybe throw an exception here instead of just an assert?
-    // Have to properly handle an exception since this is a constructor.
-    assert(cell_order == tile_order);
-
-    const size_t attr_idx = 0;
+    if (cell_order != tile_order) {
+      throw std::runtime_error("Cell order and tile order must match");
+    }
 
     auto domain_{schema_.domain()};
 
     auto row_domain{domain_.dimension(0)};
     auto col_domain{domain_.dimension(1)};
 
-    // If the user specifies a value then we use it, otherwise we use the
-    // non-empty domain. If non_empty_domain() is an empty vector it means that
-    // the array is empty.
+    // If non_empty_domain() is an empty vector it means that
+    // the array is empty. Else If the user specifies a value then we use it,
+    // otherwise we use the non-empty domain.
     auto non_empty_domain = array_->non_empty_domain<int>();
-    if (!last_row.has_value()) {
-      if (non_empty_domain.empty()) {
-        last_row_ = 0;
+    if (non_empty_domain.empty()) {
+      last_row_ = 0;
+      last_col_ = 0;
+    } else {
+      if (last_row.has_value()) {
+        last_row_ = *last_row;
       } else {
         last_row_ = non_empty_domain[0].second.second -
                     non_empty_domain[0].second.first + 1;
       }
-    } else {
-      last_row_ = *last_row;
-    }
-    if (!last_col.has_value()) {
-      if (non_empty_domain.empty()) {
-        last_col_ = 0;
+      if (last_col.has_value()) {
+        last_col_ = *last_col;
       } else {
         last_col_ = non_empty_domain[1].second.second -
                     non_empty_domain[1].second.first + 1;
       }
-    } else {
-      last_col_ = *last_col;
     }
 
     size_t dimension = last_row_ - first_row_;

src/include/test/test_utils.h

@@ -62,13 +62,14 @@ void fill_and_write_matrix(
     const std::string& uri,
     size_t rows,
     size_t cols,
-    size_t offset) {
+    size_t offset,
+    TemporalPolicy temporal_policy = {}) {
   tiledb::VFS vfs(ctx);
   if (vfs.is_dir(uri)) {
     vfs.remove_dir(uri);
   }
   std::iota(X.data(), X.data() + dimension(X) * num_vectors(X), offset);
-  write_matrix(ctx, X, uri);
+  write_matrix(ctx, X, uri, 0, true, temporal_policy);
 }
 
 /*
@@ -91,7 +92,8 @@ void fill_and_write_matrix(
     const std::string& ids_uri,
     size_t rows,
     size_t cols,
-    size_t offset) {
+    size_t offset,
+    TemporalPolicy temporal_policy = {}) {
   tiledb::VFS vfs(ctx);
   if (vfs.is_dir(uri)) {
     vfs.remove_dir(uri);
@@ -103,10 +105,10 @@ void fill_and_write_matrix(
   std::iota(X.ids().begin(), X.ids().end(), offset);
 
   // Write the vectors to their URI.
-  write_matrix(ctx, X, uri);
+  write_matrix(ctx, X, uri, 0, true, temporal_policy);
 
   // Write the IDs to their URI.
-  write_vector(ctx, X.ids(), ids_uri);
+  write_vector(ctx, X.ids(), ids_uri, 0, true, temporal_policy);
 }
 
 void validate_metadata(

src/include/test/unit_api_feature_vector_array.cc

@@ -664,7 +664,7 @@ TEST_CASE("api: temporal_policy", "[api]") {
     }
   }
 
-  // Read the data at timestamp 99 explicitly.
+  // Read the data at timestamp 99.
   {
     auto feature_vector_array = FeatureVectorArray(
         ctx, feature_vectors_uri, ids_uri, 0, TemporalPolicy(TimeTravel, 99));
@@ -684,4 +684,24 @@ TEST_CASE("api: temporal_policy", "[api]") {
       }
     }
   }
+
+  // Read the data at timestamp 50.
+  {
+    auto feature_vector_array = FeatureVectorArray(
+        ctx, feature_vectors_uri, ids_uri, 0, TemporalPolicy(TimeTravel, 50));
+    CHECK(extents(feature_vector_array)[0] == 0);
+    CHECK(extents(feature_vector_array)[1] == 0);
+    CHECK(feature_vector_array.num_vectors() == 0);
+    CHECK(feature_vector_array.num_ids() == 0);
+    CHECK(feature_vector_array.dimension() == 0);
+    auto data = MatrixView<FeatureType, stdx::layout_left>{
+        (FeatureType*)feature_vector_array.data(),
+        extents(feature_vector_array)[0],
+        extents(feature_vector_array)[1]};
+    auto ids = std::span<IdsType>(
+        (IdsType*)feature_vector_array.ids_data(),
+        feature_vector_array.num_vectors());
+    CHECK(ids.size() == 0);
+    CHECK(data.size() == 0);
+  }
 }

src/include/test/unit_tdb_io.cc

@@ -174,13 +174,13 @@ TEST_CASE("tdb_io: write empty matrix", "[tdb_io]") {
   empty_matrix.load();
   CHECK(num_vectors(empty_matrix) == 0);
   CHECK(empty_matrix.num_cols() == 0);
-  CHECK(empty_matrix.num_rows() == dimension);
+  CHECK(empty_matrix.num_rows() == 0);
 
   auto empty_preload_matrix =
       tdbColMajorPreLoadMatrix<float>(ctx, tmp_matrix_uri, dimension, 0, 0, {});
   CHECK(num_vectors(empty_preload_matrix) == 0);
   CHECK(empty_preload_matrix.num_cols() == 0);
-  CHECK(empty_preload_matrix.num_rows() == dimension);
+  CHECK(empty_preload_matrix.num_rows() == 0);
 }
 
 TEST_CASE("tdb_io: write empty vector", "[tdb_io]") {

src/include/test/unit_tdb_matrix.cc

@@ -336,3 +336,70 @@ TEST_CASE("tdb_matrix: empty matrix", "[tdb_matrix]") {
     CHECK(dimension(X) == 0);
   }
 }
+
+TEST_CASE("tdb_matrix: time travel", "[tdb_matrix]") {
+  tiledb::Context ctx;
+  std::string tmp_matrix_uri =
+      (std::filesystem::temp_directory_path() / "tmp_tdb_matrix").string();
+  int offset = 13;
+
+  size_t Mrows = 20;
+  size_t Ncols = 50;
+
+  tiledb::VFS vfs(ctx);
+  if (vfs.is_dir(tmp_matrix_uri)) {
+    vfs.remove_dir(tmp_matrix_uri);
+  }
+
+  auto X = ColMajorMatrix<int>(Mrows, Ncols);
+  std::iota(X.data(), X.data() + dimension(X) * num_vectors(X), offset);
+  write_matrix(ctx, X, tmp_matrix_uri, 0, true, TemporalPolicy{TimeTravel, 50});
+
+  {
+    // We can load the matrix at the creation timestamp.
+    auto Y = tdbPreLoadMatrix<int, stdx::layout_left>(
+        ctx, tmp_matrix_uri, 0, TemporalPolicy{TimeTravel, 50});
+    CHECK(num_vectors(Y) == num_vectors(X));
+    CHECK(dimension(Y) == dimension(X));
+    CHECK(std::equal(
+        X.data(), X.data() + dimension(X) * num_vectors(X), Y.data()));
+    for (size_t i = 0; i < Mrows; ++i) {
+      for (size_t j = 0; j < Ncols; ++j) {
+        CHECK(X(i, j) == Y(i, j));
+      }
+    }
+  }
+
+  {
+    // We can load the matrix at a later timestamp.
+    auto Y = tdbPreLoadMatrix<int, stdx::layout_left>(
+        ctx, tmp_matrix_uri, 0, TemporalPolicy{TimeTravel, 100});
+    CHECK(num_vectors(Y) == num_vectors(X));
+    CHECK(dimension(Y) == dimension(X));
+    CHECK(std::equal(
+        X.data(), X.data() + dimension(X) * num_vectors(X), Y.data()));
+    for (size_t i = 0; i < Mrows; ++i) {
+      for (size_t j = 0; j < Ncols; ++j) {
+        CHECK(X(i, j) == Y(i, j));
+      }
+    }
+  }
+
+  {
+    // We get no data if we load the matrix at an earlier timestamp.
+    auto Y = tdbPreLoadMatrix<int, stdx::layout_left>(
+        ctx, tmp_matrix_uri, 0, TemporalPolicy{TimeTravel, 5});
+    CHECK(num_vectors(Y) == 0);
+    CHECK(dimension(Y) == 0);
+  }
+
+  {
+    // We get no data if we load the matrix at an earlier timestamp, even if we
+    // specify we want to read 4 rows and 2 cols.
+    auto Y = tdbPreLoadMatrix<int, stdx::layout_left>(
+        ctx, tmp_matrix_uri, 4, 2, 0, TemporalPolicy{TimeTravel, 5});
+    CHECK(num_vectors(Y) == 0);
+    CHECK(dimension(Y) == 0);
+    CHECK(Y.size() == 0);
+  }
+}

src/include/test/unit_tdb_matrix_with_ids.cc

@@ -368,3 +368,90 @@ TEMPLATE_TEST_CASE(
     CHECK(X.ids()[i] == Z.ids()[i]);
   }
 }
+
+TEST_CASE("tdb_matrix_with_ids: time travel", "[tdb_matrix_with_ids]") {
+  tiledb::Context ctx;
+  std::string tmp_matrix_uri =
+      (std::filesystem::temp_directory_path() / "tmp_tdb_matrix").string();
+  std::string tmp_ids_uri =
+      (std::filesystem::temp_directory_path() / "tmp_ids_vector").string();
+
+  int offset = 13;
+
+  size_t Mrows = 40;
+  size_t Ncols = 20;
+
+  tiledb::VFS vfs(ctx);
+  if (vfs.is_dir(tmp_matrix_uri)) {
+    vfs.remove_dir(tmp_matrix_uri);
+  }
+  if (vfs.is_dir(tmp_ids_uri)) {
+    vfs.remove_dir(tmp_ids_uri);
+  }
+
+  auto X = ColMajorMatrixWithIds<float, uint64_t, size_t>(Mrows, Ncols);
+  fill_and_write_matrix(
+      ctx,
+      X,
+      tmp_matrix_uri,
+      tmp_ids_uri,
+      Mrows,
+      Ncols,
+      offset,
+      TemporalPolicy{TimeTravel, 50});
+
+  {
+    // We can load the matrix at the creation timestamp.
+    auto Y = tdbColMajorPreLoadMatrixWithIds<float, uint64_t, size_t>(
+        ctx, tmp_matrix_uri, tmp_ids_uri, 0, TemporalPolicy{TimeTravel, 50});
+    CHECK(num_vectors(Y) == num_vectors(X));
+    CHECK(dimension(Y) == dimension(X));
+    CHECK(std::equal(
+        X.data(), X.data() + dimension(X) * num_vectors(X), Y.data()));
+    for (size_t i = 0; i < Mrows; ++i) {
+      for (size_t j = 0; j < Ncols; ++j) {
+        CHECK(X(i, j) == Y(i, j));
+      }
+    }
+  }
+
+  {
+    // We can load the matrix at a later timestamp.
+    auto Y = tdbColMajorPreLoadMatrixWithIds<float, uint64_t, size_t>(
+        ctx, tmp_matrix_uri, tmp_ids_uri, 0, TemporalPolicy{TimeTravel, 100});
+    CHECK(num_vectors(Y) == num_vectors(X));
+    CHECK(dimension(Y) == dimension(X));
+    CHECK(std::equal(
+        X.data(), X.data() + dimension(X) * num_vectors(X), Y.data()));
+    for (size_t i = 0; i < Mrows; ++i) {
+      for (size_t j = 0; j < Ncols; ++j) {
+        CHECK(X(i, j) == Y(i, j));
+      }
+    }
+  }
+
+  {
+    // We get no data if we load the matrix at an earlier timestamp.
+    auto Y = tdbColMajorPreLoadMatrixWithIds<float, uint64_t, size_t>(
+        ctx, tmp_matrix_uri, tmp_ids_uri, 0, TemporalPolicy{TimeTravel, 5});
+    CHECK(num_vectors(Y) == 0);
+    CHECK(dimension(Y) == 0);
+    CHECK(Y.size() == 0);
+  }
+
+  {
+    // We get no data if we load the matrix at an earlier timestamp, even if we
+    // specify we want to read 4 rows and 2 cols.
+    auto Y = tdbColMajorPreLoadMatrixWithIds<float, uint64_t, size_t>(
+        ctx,
+        tmp_matrix_uri,
+        tmp_ids_uri,
+        4,
+        2,
+        0,
+        TemporalPolicy{TimeTravel, 5});
+    CHECK(num_vectors(Y) == 0);
+    CHECK(dimension(Y) == 0);
+    CHECK(Y.size() == 0);
+  }
+}