feat(silo): allow updating of string columns

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

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.

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:

src/silo/database.test.cpp

@@ -54,7 +54,7 @@ std::shared_ptr<silo::Database> 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<silo::query_engine::IllegalQueryException>(
+         ::testing::HasSubstr("expected string literal")
+      )
+   );
+
+   // A phylogenetic-tree-backed column (primaryKey) cannot be updated.
+   EXPECT_THAT(
+      [&]() { database->updateColumn(table, "primaryKey", "'new_key'", "true"); },
+      ThrowsMessage<silo::query_engine::IllegalQueryException>(
+         ::testing::HasSubstr("phylogenetic tree")
+      )
+   );
+
+   // A lineage-indexed column (pango_lineage) cannot be updated.
    EXPECT_THAT(
-      [&]() { database->updateColumn(table, "division", "Basel", "true"); },
-      ThrowsMessage<silo::query_engine::IllegalQueryException>(::testing::HasSubstr("not supported")
+      [&]() { database->updateColumn(table, "pango_lineage", "'B.1'", "true"); },
+      ThrowsMessage<silo::query_engine::IllegalQueryException>(::testing::HasSubstr("lineage index")
       )
    );
 

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
          ));

src/silo/storage/column/indexed_string_column.cpp

@@ -5,6 +5,7 @@
 #include <fmt/format.h>
 
 #include "silo/common/bidirectional_string_map.h"
+#include "silo/storage/column/row_id.h"
 
 namespace silo::storage::column {
 
@@ -106,6 +107,41 @@ std::expected<void, std::string> IndexedStringColumn::appendChunk(const Buffer&
    return {};
 }
 
+void IndexedStringColumn::update(
+   const roaring::Roaring& row_ids,
+   const std::optional<std::string>& 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());
 }

src/silo/storage/column/indexed_string_column.h

@@ -91,6 +91,8 @@ class IndexedStringColumn {
 
    std::expected<void, std::string> appendChunk(const Buffer& buffer);
 
+   void update(const roaring::Roaring& row_ids, const std::optional<std::string>& value);
+
    [[nodiscard]] size_t numChunks() const { return value_ids.numChunks(); }
 
    [[nodiscard]] uint32_t chunkSize(uint16_t chunk_id) const {

src/silo/storage/column/indexed_string_column.test.cpp

@@ -2,6 +2,7 @@
 
 #include <expected>
 #include <initializer_list>
+#include <optional>
 #include <string>
 #include <string_view>
 
@@ -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<std::string>{}).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"