diff --git a/documentation/python_bindings.md b/documentation/python_bindings.md index 3670ab1c8..fc0fe7ab9 100644 --- a/documentation/python_bindings.md +++ b/documentation/python_bindings.md @@ -118,11 +118,11 @@ print(len(matching)) # number of matching rows **`get_amino_acid_reference_sequence(table_name, sequence_name)`** → `str`. **`print_all_data(table_name)`** — prints all rows of a table to stdout (debugging aid). -### Updating scalar columns +### Updating columns **`update_column(table_name, column_name, value, filter_expression="")`** -Assigns a single scalar `value` to `column_name` for every row matched by `filter_expression` (a SaneQL filter, like `get_filtered_bitmap`). An empty or `None` filter defaults to `true`, updating all rows. +Assigns a single `value` to `column_name` for every row matched by `filter_expression` (a SaneQL filter, like `get_filtered_bitmap`). An empty or `None` filter defaults to `true`, updating all rows. `value` is a **SaneQL literal** — parsed by the same parser as queries — and must match the column's type: @@ -132,9 +132,10 @@ Assigns a single scalar `value` to `column_name` for every row matched by `filte | float | `"3.14"`, `"0"` | | bool | `"true"`, `"false"` | | date | `"'2021-03-15'::date"` | +| string | `"'Basel'"`  (a quoted string literal) | | any of them | `"null"`  (clears the rows) | -Only scalar value columns (int, float, date, bool) can be updated. String and sequence columns are rejected. +Scalar value columns (int, float, date, bool) and string columns (plain, indexed, and zstd-compressed) can be updated. Two kinds of string column are rejected because their auxiliary indexes have no in-place update support: columns backed by a **phylogenetic tree** (e.g. the primary key when it is a phylo field) and columns backed by a **lineage index**. Sequence columns are also rejected. ```python db = Database("path/to/silo-dir") @@ -148,6 +149,9 @@ db.update_column("default", "age", "100", filter_expression="age = 4") # Assign a date literal db.update_column("default", "date", "'2000-01-01'::date") +# Reassign a string column (note the quotes: the value is a SaneQL string literal) +db.update_column("default", "division", "'Basel'", filter_expression="division = 'Bern'") + # Clear a boolean column to null for a subset of rows db.update_column("default", "test_boolean_column", "null", filter_expression="region = 'Asia'") ``` @@ -177,9 +181,9 @@ The bindings translate C++ exceptions into Python ones: ```python try: - db.update_column("default", "country", "'x'") # country is a string column + db.update_column("default", "pango_lineage", "'B.1'") # backed by a lineage index except ValueError as e: - print(e) # "Updating columns of type '...' is not supported; only INT32, FLOAT, DATE32 and BOOL ..." + print(e) # "Column 'pango_lineage' is backed by a lineage index and cannot be updated" ``` ## Complete Example diff --git a/python/silodb/database.pyx b/python/silodb/database.pyx index 0380e5a36..15c9d6344 100644 --- a/python/silodb/database.pyx +++ b/python/silodb/database.pyx @@ -375,10 +375,12 @@ cdef class PyDatabase: ``get_filtered_bitmap``). Every matched row is assigned the same ``value``, a SaneQL literal (parsed by the same parser as queries) matching the column's type, e.g. ``"3"`` for an int column, ``"3.14"`` for a float column, ``"true"``/``"false"`` for a bool - column, ``"'2021-03-15'::date"`` for a date column. The literal ``"null"`` clears the - matched rows. + column, ``"'2021-03-15'::date"`` for a date column, and a quoted string literal such as + ``"'Basel'"`` for a string column. The literal ``"null"`` clears the matched rows. - Only scalar value columns (int, float, date and bool) can be updated. + Scalar value columns (int, float, date and bool) and string columns (plain, indexed and + zstd-compressed) can be updated. String columns backed by a phylogenetic tree or a lineage + index cannot be updated, because their auxiliary indexes have no in-place update support. Not thread-safe: this mutates the in-memory database in place with no internal locking. diff --git a/python/tests/test_database.py b/python/tests/test_database.py index 2972df696..7dddc58e6 100644 --- a/python/tests/test_database.py +++ b/python/tests/test_database.py @@ -788,10 +788,12 @@ def test_query_preserves_data_after_checkpoint(self, empty_database, main_refere class TestUpdateColumn: """Test the update_column binding. - Scalar value columns (int/float/date/bool) cannot be created through the Python table-creation - API (its extra columns are always strings), so these tests exercise the binding layer itself: - argument validation, marshalling, and translation of C++ errors into Python exceptions. The - successful scalar-update behavior is covered by the C++ unit tests (database.test.cpp). + Extra columns created through the Python table-creation API are plain (non-indexed, non-phylo) + string columns, so these tests cover both the binding layer itself (argument validation, + marshalling, translation of C++ errors into Python exceptions) and the end-to-end update of a + string column. Scalar value columns (int/float/date/bool) and indexed string columns cannot be + created through this API; their update behavior is covered by TestUpdateColumnOnLoadedDatabase + and the C++ unit tests. """ def _database_with_string_column(self, main_reference_sequence): @@ -808,6 +810,10 @@ def _database_with_string_column(self, main_reference_sequence): db.append_data_from_file("sequences", INPUT_FILE) return db + @staticmethod + def _count(database, filter_expression): + return len(database.get_filtered_bitmap("sequences", filter_expression)) + def test_update_column_empty_table_name_raises(self, empty_database): """Test that an empty table name raises ValueError before reaching C++.""" with pytest.raises(ValueError, match="table_name cannot be empty"): @@ -824,11 +830,42 @@ def test_update_column_unknown_column_raises(self, main_reference_sequence): with pytest.raises(ValueError, match="does not contain a column"): db.update_column("sequences", "does_not_exist", "1") - def test_update_column_string_column_not_supported(self, main_reference_sequence): - """Test that updating a non-scalar (string) column raises the 'not supported' ValueError.""" + def test_update_string_column_all_rows(self, main_reference_sequence): + """A quoted string literal is assigned to every matched row of a string column.""" + db = self._database_with_string_column(main_reference_sequence) + total = self._count(db, "true") + assert self._count(db, "country = 'Switzerland'") == total + + db.update_column("sequences", "country", "'Germany'") + + assert self._count(db, "country = 'Switzerland'") == 0 + assert self._count(db, "country = 'Germany'") == total + + def test_update_string_column_with_filter(self, main_reference_sequence): + """Only rows matching the filter are reassigned; a brand-new value is interned.""" + db = self._database_with_string_column(main_reference_sequence) + total = self._count(db, "true") + assert total > 1 + + db.update_column("sequences", "country", "'France'", "primary_key = 'key_29'") + + assert self._count(db, "country = 'France'") == 1 + assert self._count(db, "country = 'Switzerland'") == total - 1 + + def test_update_string_column_clears_rows_to_null(self, main_reference_sequence): + """The literal 'null' clears the matched rows of a string column.""" + db = self._database_with_string_column(main_reference_sequence) + assert self._count(db, "country = null") == 0 + + db.update_column("sequences", "country", "null", "primary_key = 'key_29'") + + assert self._count(db, "country = null") == 1 + + def test_update_string_column_type_mismatch_raises(self, main_reference_sequence): + """A non-string literal is rejected for a string column.""" db = self._database_with_string_column(main_reference_sequence) - with pytest.raises(ValueError, match="not supported"): - db.update_column("sequences", "country", "'x'") + with pytest.raises(ValueError, match="expected string literal"): + db.update_column("sequences", "country", "5") class TestUpdateColumnOnLoadedDatabase: diff --git a/src/silo/database.test.cpp b/src/silo/database.test.cpp index c2d8665cd..ededc4fac 100644 --- a/src/silo/database.test.cpp +++ b/src/silo/database.test.cpp @@ -54,7 +54,7 @@ std::shared_ptr buildTestDatabase() { database->createTable( silo::schema::TableName::getDefault(), silo::initialize::Initializer::createSchemaFromConfigFiles( - std::move(database_config), + database_config, reference_genomes, lineage_trees, phylo_tree_file, @@ -172,6 +172,25 @@ TEST(DatabaseTest, updateColumnAssignsScalarValueToMatchingRows) { // Date values are SaneQL date literals. database->updateColumn(table, "date", "'2000-01-01'::date", "true"); ASSERT_EQ(countWhere(*database, "date = '2000-01-01'::date"), 5); + + // Indexed string columns can be reassigned; the inverted index stays consistent. `value` is a + // SaneQL string literal, so it is quoted. + ASSERT_EQ(countWhere(*database, "division = 'Bern'"), 2); + database->updateColumn(table, "division", "'Zurich'", "division = 'Bern'"); + ASSERT_EQ(countWhere(*database, "division = 'Bern'"), 0); + ASSERT_EQ(countWhere(*database, "division = 'Zurich'"), 2); + + // A value not previously present in the dictionary is interned on update. + database->updateColumn(table, "division", "'Geneva'", "primaryKey = 'key1'"); + ASSERT_EQ(countWhere(*database, "division = 'Geneva'"), 1); + + // A SaneQL `null` literal clears an indexed string back to null, and a concrete value can be set + // again afterwards. + database->updateColumn(table, "division", "null", "primaryKey = 'key1'"); + ASSERT_EQ(countWhere(*database, "division = null"), 1); + database->updateColumn(table, "division", "'Basel'", "primaryKey = 'key1'"); + ASSERT_EQ(countWhere(*database, "division = null"), 0); + ASSERT_EQ(countWhere(*database, "division = 'Basel'"), 1); } TEST(DatabaseTest, updateColumnRejectsInvalidRequests) { @@ -186,10 +205,26 @@ TEST(DatabaseTest, updateColumnRejectsInvalidRequests) { ) ); - // String columns cannot be updated by this scalar path. + // A string literal must be quoted; an int literal is not a valid string value. + EXPECT_THAT( + [&]() { database->updateColumn(table, "division", "5", "true"); }, + ThrowsMessage( + ::testing::HasSubstr("expected string literal") + ) + ); + + // A phylogenetic-tree-backed column (primaryKey) cannot be updated. + EXPECT_THAT( + [&]() { database->updateColumn(table, "primaryKey", "'new_key'", "true"); }, + ThrowsMessage( + ::testing::HasSubstr("phylogenetic tree") + ) + ); + + // A lineage-indexed column (pango_lineage) cannot be updated. EXPECT_THAT( - [&]() { database->updateColumn(table, "division", "Basel", "true"); }, - ThrowsMessage(::testing::HasSubstr("not supported") + [&]() { database->updateColumn(table, "pango_lineage", "'B.1'", "true"); }, + ThrowsMessage(::testing::HasSubstr("lineage index") ) ); diff --git a/src/silo/query_engine/scalar_column_update.cpp b/src/silo/query_engine/scalar_column_update.cpp index f500e7908..b6b5ba0af 100644 --- a/src/silo/query_engine/scalar_column_update.cpp +++ b/src/silo/query_engine/scalar_column_update.cpp @@ -49,10 +49,41 @@ void assignScalarLiteralToColumn( row_ids, is_null ? std::nullopt : std::optional{ast::extractBoolLiteral(*literal)} ); return; + case schema::ColumnType::STRING: { + auto& string_column = columns.string_columns.at(column.name); + if (string_column.metadata->phylo_tree.has_value()) { + throw IllegalQueryException(fmt::format( + "Column '{}' is backed by a phylogenetic tree and cannot be updated, because it " + "would break the tree's row bindings", + column.name + )); + } + string_column.update( + row_ids, is_null ? std::nullopt : std::optional{ast::extractStringLiteral(*literal)} + ); + return; + } + case schema::ColumnType::INDEXED_STRING: { + auto& indexed_string_column = columns.indexed_string_columns.at(column.name); + if (indexed_string_column.getLineageIndex().has_value()) { + throw IllegalQueryException(fmt::format( + "Column '{}' is backed by a lineage index and cannot be updated", column.name + )); + } + indexed_string_column.update( + row_ids, is_null ? std::nullopt : std::optional{ast::extractStringLiteral(*literal)} + ); + return; + } + case schema::ColumnType::ZSTD_COMPRESSED_STRING: + columns.zstd_compressed_string_columns.at(column.name) + .update( + row_ids, is_null ? std::nullopt : std::optional{ast::extractStringLiteral(*literal)} + ); + return; default: throw IllegalQueryException(fmt::format( - "Updating columns of type '{}' is not supported; only INT32, FLOAT, DATE32 and BOOL " - "columns can be updated (column '{}')", + "Updating columns of type '{}' is not supported (column '{}')", schema::columnTypeToString(column.type), column.name )); diff --git a/src/silo/storage/column/indexed_string_column.cpp b/src/silo/storage/column/indexed_string_column.cpp index e8bceced1..09deecf78 100644 --- a/src/silo/storage/column/indexed_string_column.cpp +++ b/src/silo/storage/column/indexed_string_column.cpp @@ -5,6 +5,7 @@ #include #include "silo/common/bidirectional_string_map.h" +#include "silo/storage/column/row_id.h" namespace silo::storage::column { @@ -106,6 +107,41 @@ std::expected IndexedStringColumn::appendChunk(const Buffer& return {}; } +void IndexedStringColumn::update( + const roaring::Roaring& row_ids, + const std::optional& value +) { + SILO_ASSERT(!lineage_index.has_value()); + + // Null rows carry the empty-string placeholder id as their stored value (see `appendChunk`), so + // the target id is that placeholder for a null update and the interned value id otherwise. + const Idx new_value_id = value.has_value() ? metadata->dictionary.getOrCreateId(*value) + : metadata->dictionary.getOrCreateId(""); + // `try_emplace` keeps the id present in the index even for the null placeholder, matching how + // `appendChunk` treats null rows (present as a value id, absent from any match bitmap). + indexed_values.try_emplace(new_value_id); + + for (const uint32_t global_row_id : row_ids) { + const RowId row_id = RowId::fromGlobal(global_row_id); + // Detach the row from its previous value's match bitmap. Previously-null rows are not members + // of any match bitmap, so only non-null rows need detaching. + if (!null_bitmap.contains(global_row_id)) { + const Idx old_value_id = value_ids.at(row_id); + indexed_values.at(old_value_id).remove(global_row_id); + } + value_ids.setValue(row_id, new_value_id); + if (value.has_value()) { + indexed_values.at(new_value_id).add(global_row_id); + } + } + + if (value.has_value()) { + null_bitmap -= row_ids; + } else { + null_bitmap |= row_ids; + } +} + bool IndexedStringColumn::isNull(RowId row_id) const { return null_bitmap.contains(row_id.toGlobal()); } diff --git a/src/silo/storage/column/indexed_string_column.h b/src/silo/storage/column/indexed_string_column.h index 8c535506e..82c1bc1c7 100644 --- a/src/silo/storage/column/indexed_string_column.h +++ b/src/silo/storage/column/indexed_string_column.h @@ -91,6 +91,8 @@ class IndexedStringColumn { std::expected appendChunk(const Buffer& buffer); + void update(const roaring::Roaring& row_ids, const std::optional& value); + [[nodiscard]] size_t numChunks() const { return value_ids.numChunks(); } [[nodiscard]] uint32_t chunkSize(uint16_t chunk_id) const { diff --git a/src/silo/storage/column/indexed_string_column.test.cpp b/src/silo/storage/column/indexed_string_column.test.cpp index a89a58b2d..37939cb77 100644 --- a/src/silo/storage/column/indexed_string_column.test.cpp +++ b/src/silo/storage/column/indexed_string_column.test.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -103,6 +104,36 @@ TEST(IndexedStringColumn, valuesSpanningMultipleAppendedChunks) { ASSERT_EQ(*under_test.filter("value 3").value(), roaring::Roaring({RowId::chunkStart(1) + 1})); } +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST(IndexedStringColumn, updateKeepsInvertedIndexConsistent) { + IndexedStringColumnMetadata column_metadata("some_column"); + IndexedStringColumn under_test{&column_metadata}; + + ASSERT_TRUE( + appendIndexedValues(under_test, {"value 1", "value 2", "value 2", "value 3", "value 1"}) + .has_value() + ); + + // Reassign the two "value 2" rows to a value not yet in the dictionary; the old value's bitmap + // empties and the new value is interned in both the dictionary and the inverted index. + under_test.update(roaring::Roaring({1, 2}), "value 4"); + ASSERT_EQ(*under_test.filter("value 4").value(), roaring::Roaring({1, 2})); + ASSERT_EQ(*under_test.filter("value 2").value(), roaring::Roaring()); + EXPECT_EQ(under_test.getValueString(RowId(0, 1)), "value 4"); + EXPECT_EQ(under_test.getValueString(RowId(0, 2)), "value 4"); + + // A nullopt update detaches rows from their value bitmap and marks them null. + under_test.update(roaring::Roaring({0}), std::nullopt); + ASSERT_EQ(*under_test.filter("value 1").value(), roaring::Roaring({4})); + ASSERT_EQ(*under_test.filter(std::optional{}).value(), roaring::Roaring({0})); + EXPECT_TRUE(under_test.isNull(RowId(0, 0))); + + // A previously-null row can be reassigned to a concrete value again. + under_test.update(roaring::Roaring({0}), "value 1"); + ASSERT_EQ(*under_test.filter("value 1").value(), roaring::Roaring({0, 4})); + EXPECT_FALSE(under_test.isNull(RowId(0, 0))); +} + TEST(IndexedStringColumn, addingLineageAndThenSublineageFiltersCorrectly) { auto lineage_definition = LineageTreeAndIdMap::fromLineageDefinitionFilePath( "testBaseData/exampleDataset/lineage_definition.yaml" diff --git a/src/silo/storage/column/string_column.cpp b/src/silo/storage/column/string_column.cpp index 855d098b6..c52953fa5 100644 --- a/src/silo/storage/column/string_column.cpp +++ b/src/silo/storage/column/string_column.cpp @@ -139,6 +139,51 @@ std::expected StringColumn::appendChunk(const Buffer& buffer) return {}; } +void StringColumn::update( + const roaring::Roaring& row_ids, + const std::optional& value +) { + // Chunks are immutable once appended, so every chunk containing an updated row is rebuilt from + // scratch rather than mutated in place. First collect the chunk ids that are actually touched so + // untouched chunks are left alone. + std::unordered_set touched_chunk_ids; + for (const uint32_t global_row_id : row_ids) { + touched_chunk_ids.insert(RowId::fromGlobal(global_row_id).chunk_id); + } + + for (const uint16_t chunk_id : touched_chunk_ids) { + const uint32_t chunk_row_count = chunkSize(chunk_id); + StringColumnChunk rebuilt_chunk; + for (uint32_t row_in_chunk = 0; row_in_chunk < chunk_row_count; ++row_in_chunk) { + const RowId row_id{ + .chunk_id = chunk_id, .row_in_chunk = static_cast(row_in_chunk) + }; + const uint32_t global_row_id = row_id.toGlobal(); + if (row_ids.contains(global_row_id)) { + // Updated row: take the new value (or the null placeholder when clearing). + if (value.has_value()) { + rebuilt_chunk.insert(*value); + } else { + rebuilt_chunk.insertNull(); + } + } else if (null_bitmap.contains(global_row_id)) { + // Untouched null row: reads from the old chunk are meaningless, keep it null. + rebuilt_chunk.insertNull(); + } else { + // Untouched non-null row: copy its current value over from the old chunk. + rebuilt_chunk.insert(getValueString(row_id)); + } + } + chunks.at(chunk_id) = std::move(rebuilt_chunk); + } + + if (value.has_value()) { + null_bitmap -= row_ids; + } else { + null_bitmap |= row_ids; + } +} + bool StringColumn::isNull(RowId row_id) const { return null_bitmap.contains(row_id.toGlobal()); } diff --git a/src/silo/storage/column/string_column.h b/src/silo/storage/column/string_column.h index 2b40a163d..17303e527 100644 --- a/src/silo/storage/column/string_column.h +++ b/src/silo/storage/column/string_column.h @@ -66,7 +66,7 @@ class StringColumnMetadata : public ColumnMetadata { /// One immutable slice of a StringColumn, produced by a single `appendChunk` call. The German /// string suffix ids stored in `fixed_string_data` reference offsets within this same chunk's /// `variable_string_data`, so the two registries are kept paired. A chunk is never mutated once it -/// has been appended; deleting rows rewrites whole chunks rather than shifting data across them. +/// has been appended; deleting or updating rows rewrites whole chunks rather than mutating them. class StringColumnChunk { vector::GermanStringRegistry fixed_string_data; @@ -124,6 +124,8 @@ class StringColumn { std::expected appendChunk(const Buffer& buffer); + void update(const roaring::Roaring& row_ids, const std::optional& value); + [[nodiscard]] bool isNull(RowId row_id) const; [[nodiscard]] SiloString getValue(RowId row_id) const; diff --git a/src/silo/storage/column/string_column.test.cpp b/src/silo/storage/column/string_column.test.cpp index 51f022f3c..cd2465eb4 100644 --- a/src/silo/storage/column/string_column.test.cpp +++ b/src/silo/storage/column/string_column.test.cpp @@ -2,9 +2,12 @@ #include #include +#include #include #include +#include + #include #include #include @@ -225,6 +228,42 @@ TEST(StringColumn, compareAcrossColumns) { EXPECT_EQ(column_1.getValueString(RowId(0, 4)), column_2.getValueString(RowId(0, 4))); } +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST(StringColumn, updateRebuildsTouchedChunks) { + StringColumnMetadata metadata{"string_column"}; + StringColumn under_test(&metadata); + + SILO_ASSERT(appendStringValues(under_test, {"short a", "value to overwrite"}).has_value()); + SILO_ASSERT(appendStringValues(under_test, {"short b", "short c"}).has_value()); + + // Overwrite a short value with a long one (moving it into the variable-data registry) and a long + // value with a short one, targeting rows in both chunks at once. Untouched rows in a rebuilt + // chunk must keep their original values. + roaring::Roaring to_long; + to_long.add(RowId(0, 0).toGlobal()); + to_long.add(RowId(1, 1).toGlobal()); + under_test.update(to_long, "a replacement value that is definitely long"); + + roaring::Roaring to_short; + to_short.add(RowId(0, 1).toGlobal()); + under_test.update(to_short, "tiny"); + + EXPECT_EQ(under_test.getValueString(RowId(0, 0)), "a replacement value that is definitely long"); + EXPECT_EQ(under_test.getValueString(RowId(0, 1)), "tiny"); + EXPECT_EQ(under_test.getValueString(RowId(1, 0)), "short b"); // untouched + EXPECT_EQ(under_test.getValueString(RowId(1, 1)), "a replacement value that is definitely long"); + + // A nullopt update marks rows null; a concrete value clears the null flag again. + roaring::Roaring to_null; + to_null.add(RowId(1, 0).toGlobal()); + under_test.update(to_null, std::nullopt); + EXPECT_TRUE(under_test.isNull(RowId(1, 0))); + + under_test.update(to_null, "revived"); + EXPECT_FALSE(under_test.isNull(RowId(1, 0))); + EXPECT_EQ(under_test.getValueString(RowId(1, 0)), "revived"); +} + // NOLINTNEXTLINE(readability-function-cognitive-complexity) TEST(StringColumn, valuesSpanningMultipleAppendedChunks) { StringColumnMetadata metadata{"string_column"}; diff --git a/src/silo/storage/column/zstd_compressed_string_column.cpp b/src/silo/storage/column/zstd_compressed_string_column.cpp index 1df7aaefb..4d32f762c 100644 --- a/src/silo/storage/column/zstd_compressed_string_column.cpp +++ b/src/silo/storage/column/zstd_compressed_string_column.cpp @@ -1,5 +1,7 @@ #include "silo/storage/column/zstd_compressed_string_column.h" +#include "silo/storage/column/row_id.h" + namespace silo::storage::column { ZstdCompressedStringColumnMetadata::ZstdCompressedStringColumnMetadata( @@ -33,6 +35,24 @@ std::expected ZstdCompressedStringColumn::appendChunk(const B return {}; } +void ZstdCompressedStringColumn::update( + const roaring::Roaring& row_ids, + const std::optional& value +) { + // An empty stored buffer denotes null (see `appendChunk`/`getDecompressed`); a concrete value is + // compressed once and the same bytes are written to every matched row. + std::string stored_value; + if (value.has_value()) { + stored_value = metadata->compressor.compress(value->data(), value->size()); + null_bitmap -= row_ids; + } else { + null_bitmap |= row_ids; + } + for (const uint32_t global_row_id : row_ids) { + values.setValue(RowId::fromGlobal(global_row_id), stored_value); + } +} + bool ZstdCompressedStringColumn::isNull(RowId row_id) const { return null_bitmap.contains(row_id.toGlobal()); } diff --git a/src/silo/storage/column/zstd_compressed_string_column.h b/src/silo/storage/column/zstd_compressed_string_column.h index 91918d45e..719a4ef62 100644 --- a/src/silo/storage/column/zstd_compressed_string_column.h +++ b/src/silo/storage/column/zstd_compressed_string_column.h @@ -55,6 +55,8 @@ class ZstdCompressedStringColumn { [[nodiscard]] std::expected appendChunk(const Buffer& buffer); + void update(const roaring::Roaring& row_ids, const std::optional& value); + [[nodiscard]] bool isNull(RowId row_id) const; [[nodiscard]] size_t numChunks() const { return values.numChunks(); } diff --git a/src/silo/storage/column/zstd_compressed_string_column.test.cpp b/src/silo/storage/column/zstd_compressed_string_column.test.cpp index 84e18f465..6861b77eb 100644 --- a/src/silo/storage/column/zstd_compressed_string_column.test.cpp +++ b/src/silo/storage/column/zstd_compressed_string_column.test.cpp @@ -1,6 +1,9 @@ #include "silo/storage/column/zstd_compressed_string_column.h" +#include + #include +#include #include "silo/storage/column/row_id.h" @@ -48,6 +51,34 @@ void appendChunk( } } // namespace +// NOLINTNEXTLINE(readability-function-cognitive-complexity) +TEST(ZstdCompressedStringColumn, updateOverwritesCompressedValuesAcrossChunks) { + silo::storage::column::ZstdCompressedStringColumnMetadata column_metadata{"test_column", "ACGT"}; + silo::storage::column::ZstdCompressedStringColumn under_test(&column_metadata); + + appendChunk(under_test, {"2020-01-01", "2023-01-05"}); + appendChunk(under_test, {std::nullopt, "2021-12-03"}); + + // Overwrite a concrete row and a previously-null row spanning both chunks with the same value. + roaring::Roaring to_update; + to_update.add(RowId(0, 1).toGlobal()); + to_update.add(RowId(1, 0).toGlobal()); + under_test.update(to_update, "2099-09-09"); + + EXPECT_EQ(under_test.getDecompressed(RowId(0, 0)), "2020-01-01"); // untouched + EXPECT_EQ(under_test.getDecompressed(RowId(0, 1)), "2099-09-09"); + EXPECT_EQ(under_test.getDecompressed(RowId(1, 0)), "2099-09-09"); + EXPECT_FALSE(under_test.isNull(RowId(1, 0))); + EXPECT_EQ(under_test.getDecompressed(RowId(1, 1)), "2021-12-03"); // untouched + + // A nullopt update marks the row null again. + roaring::Roaring to_null; + to_null.add(RowId(0, 1).toGlobal()); + under_test.update(to_null, std::nullopt); + EXPECT_TRUE(under_test.isNull(RowId(0, 1))); + EXPECT_EQ(under_test.getDecompressed(RowId(0, 1)), std::nullopt); +} + // Each appendChunk starts a fresh, immutable chunk whose global row ids begin at a fresh // 2^16-aligned offset (chunk k starts at k << 16); the null bitmap stores those aligned row ids. // NOLINTNEXTLINE(readability-function-cognitive-complexity)