Fix update bugs

[ray6080]

No description provided.

[benjaminwinger]

This doesn't seem to work properly for me. I checked out this branch and modified the InsertManyNodesTest to also check the inserted values, and it's still failing:

diff --git a/test/storage/node_insertion_deletion_test.cpp b/test/storage/node_insertion_deletion_test.cpp
index e5cb932b9..7d9fa4e97 100644
--- a/test/storage/node_insertion_deletion_test.cpp
+++ b/test/storage/node_insertion_deletion_test.cpp
@@ -87,12 +87,18 @@ TEST_F(NodeInsertionDeletionTests, InsertManyNodesTest) {
     auto preparedStatement = conn->prepare("CREATE (:person {ID:$id});");
     for (int64_t i = 0; i < BufferPoolConstants::PAGE_4KB_SIZE; i++) {
         auto result =
-            conn->execute(preparedStatement.get(), std::make_pair(std::string("id"), 10001 + i));
+            conn->execute(preparedStatement.get(), std::make_pair(std::string("id"), 10000 + i));
         ASSERT_TRUE(result->isSuccess()) << result->toString();
     }
-    auto result = conn->query("MATCH (a:person) WHERE a.ID >= 10001 RETURN COUNT(*);");
+    auto result = conn->query("MATCH (a:person) WHERE a.ID >= 10000 RETURN COUNT(*);");
     ASSERT_TRUE(result->hasNext());
     auto tuple = result->getNext();
     ASSERT_EQ(tuple->getValue(0)->getValue<int64_t>(), BufferPoolConstants::PAGE_4KB_SIZE);
     ASSERT_FALSE(result->hasNext());
+    result = conn->query("MATCH (a:person) RETURN a.ID;");
+    int64_t i = 0;
+    while(result->hasNext()) {
+        auto tuple = result->getNext();
+        EXPECT_EQ(i++, tuple->getValue(0)->getValue<int64_t>());
+    }
 }

[benjaminwinger]

Looking forwards to implementing constant compression, I'm not sure this is the best way to handle this, as for constant compression we would have the same maximum values per page, but the values read would be non-zero, so we would actually need to scan them.

Then again, if the constant value is stored in the metadata, rather than in the page, then we would need a special way of scanning it anyway, but the simple implementation I would think (which would also work for variable sized values) would be to store it in the page once and copy it numValues times to the output.

[ray6080]

This doesn't seem to work properly for me. I checked out this branch and modified the InsertManyNodesTest to also check the inserted values, and it's still failing:

diff --git a/test/storage/node_insertion_deletion_test.cpp b/test/storage/node_insertion_deletion_test.cpp
index e5cb932b9..7d9fa4e97 100644
--- a/test/storage/node_insertion_deletion_test.cpp
+++ b/test/storage/node_insertion_deletion_test.cpp
@@ -87,12 +87,18 @@ TEST_F(NodeInsertionDeletionTests, InsertManyNodesTest) {
     auto preparedStatement = conn->prepare("CREATE (:person {ID:$id});");
     for (int64_t i = 0; i < BufferPoolConstants::PAGE_4KB_SIZE; i++) {
         auto result =
-            conn->execute(preparedStatement.get(), std::make_pair(std::string("id"), 10001 + i));
+            conn->execute(preparedStatement.get(), std::make_pair(std::string("id"), 10000 + i));
         ASSERT_TRUE(result->isSuccess()) << result->toString();
     }
-    auto result = conn->query("MATCH (a:person) WHERE a.ID >= 10001 RETURN COUNT(*);");
+    auto result = conn->query("MATCH (a:person) WHERE a.ID >= 10000 RETURN COUNT(*);");
     ASSERT_TRUE(result->hasNext());
     auto tuple = result->getNext();
     ASSERT_EQ(tuple->getValue(0)->getValue<int64_t>(), BufferPoolConstants::PAGE_4KB_SIZE);
     ASSERT_FALSE(result->hasNext());
+    result = conn->query("MATCH (a:person) RETURN a.ID;");
+    int64_t i = 0;
+    while(result->hasNext()) {
+        auto tuple = result->getNext();
+        EXPECT_EQ(i++, tuple->getValue(0)->getValue<int64_t>());
+    }
 }

Is your ground truth correct here? Why compare to i++? It should be 10000 + i++ here.

[benjaminwinger]

Is your ground truth correct here? Why compare to i++? It should be 10000 + i++ here.

The datasets starts with 0-9999 from a csv. I guess it could only query the ones which are added, but a lot of the time the erroneous values end up with smaller IDs, so it would skip the values and maybe the output would be less helpful.

[ray6080]

Is your ground truth correct here? Why compare to i++? It should be 10000 + i++ here.

The datasets starts with 0-9999 from a csv. I guess it could only query the ones which are added, but a lot of the time the erroneous values end up with smaller IDs, so it would skip the values and maybe the output would be less helpful.

Yeah, because the scan runs in parallel, so the output needs to be ordered to be easily checked.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f05d348) 89.55% compared to head (dd413f0) 89.53%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2098      +/-   ##
==========================================
- Coverage   89.55%   89.53%   -0.03%     
==========================================
  Files         981      981              
  Lines       35901    35906       +5     
==========================================
- Hits        32151    32148       -3     
- Misses       3750     3758       +8     

Files	Coverage Δ
src/include/storage/store/column_chunk.h	92.15% <100.00%> (+0.15%)	⬆️
src/include/storage/store/compression.h	96.22% <100.00%> (ø)
src/include/storage/store/node_column.h	100.00% <ø> (ø)
src/storage/local_table.cpp	94.17% <100.00%> (ø)
src/storage/store/compression.cpp	73.26% <100.00%> (ø)
src/storage/store/string_node_column.cpp	98.96% <100.00%> (ø)
src/storage/store/var_list_node_column.cpp	100.00% <100.00%> (ø)
src/storage/store/node_column.cpp	80.00% <94.11%> (+0.24%)	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.