ifcparse: make deleting and creating instances work on RocksDB-backed files

file.remove() on a RocksDB-backed file segfaulted. Reducing it turned up
five gaps that each made editing such a file crash or silently do
nothing:

- process_deletion_inverse() decoded the v| inverse-record values as
  size_t while the serializer, register_inverse(), unregister_inverse()
  and instances_by_reference() use uint32_t, so std::find failed and
  vals.erase(end()) was undefined behaviour. It also took the DeleteRange
  end from an iterator that is invalid when the instance has no inverse
  records. Decode as uint32_t, remove every occurrence guarded on
  "found", and derive the range end from the prefix itself.
- attribute_value::size() ignored storage_model_, so every aggregate
  assignment on a RocksDB instance threw "Invalid variant index" from
  set_attribute_value(). Branch on the storage model like the sibling
  accessors and count the deserialized aggregate.
- rocks_db_file_storage::create() was a stub returning an empty handle,
  which anything creating an instance then dereferenced. Implement it
  after in_memory_file_storage::create().
- max_id_ is only initialised by the in-memory parse, so a RocksDB file
  would have handed out ids that overwrite existing instances.
  Implement the recalculate_id_counter() stub per backend and run it
  once before the first fresh_id() on RocksDB.
- byid_.erase() was a no-op: set_to_map_transformer::erase() and
  rocksdb_set_view::erase() were stubs. The deleted instance's attribute
  keys and cached handle survived, entity_names() still listed it and
  reopening the database threw. Erase deletes every key under the
  instance's prefix; the transformer forwards to it and takes an
  on-erase hook the storage uses to drop the cached handle.

root.remove_product on the first 200 products of a 61 MB model now
leaves the same surviving ids and inverse counts whether the file was
opened from SPF or converted to RocksDB.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HNrXDmR88wKPCYwGE21SyH

src/ifcparse/entity_instance_data.cpp

@@ -291,8 +291,38 @@ bool attribute_value::isNull() const
 
 unsigned int attribute_value::size() const
 {
-    // @todo
-    return array_.storage_ptr->apply_visitor(size_visitor{}, index_);
+    if (storage_model_ == 0) {
+        return array_.storage_ptr->apply_visitor(size_visitor{}, index_);
+    }
+#ifdef IFOPSH_WITH_ROCKSDB
+    else {
+        // Same answers as size_visitor: element count for aggregates, -1 otherwise.
+        switch (type()) {
+        case Argument_EMPTY_AGGREGATE:
+        case Argument_AGGREGATE_OF_EMPTY_AGGREGATE:
+            return 0;
+        case Argument_AGGREGATE_OF_INT:
+            return (unsigned int)dispatch_get_<std::vector<int64_t>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size();
+        case Argument_AGGREGATE_OF_DOUBLE:
+            return (unsigned int)dispatch_get_<std::vector<double>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size();
+        case Argument_AGGREGATE_OF_STRING:
+            return (unsigned int)dispatch_get_<std::vector<std::string>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size();
+        case Argument_AGGREGATE_OF_BINARY:
+            return (unsigned int)dispatch_get_<std::vector<boost::dynamic_bitset<>>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size();
+        case Argument_AGGREGATE_OF_ENTITY_INSTANCE:
+            return (unsigned int)((std::vector<express::base>)*this).size();
+        case Argument_AGGREGATE_OF_AGGREGATE_OF_INT:
+            return (unsigned int)dispatch_get_<std::vector<std::vector<int64_t>>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size();
+        case Argument_AGGREGATE_OF_AGGREGATE_OF_DOUBLE:
+            return (unsigned int)dispatch_get_<std::vector<std::vector<double>>>(array_, storage_model_, instance_name_, entity_or_type_, index_).size();
+        case Argument_AGGREGATE_OF_AGGREGATE_OF_ENTITY_INSTANCE:
+            return (unsigned int)((std::vector<std::vector<express::base>>)*this).size();
+        default:
+            return (unsigned int)-1;
+        }
+    }
+#endif
+    throw std::logic_error("RocksDB storage is unavailable");
 }
 
 ifcopenshell::argument_type attribute_value::type() const

src/ifcparse/file.cpp

@@ -155,7 +155,15 @@ ifcopenshell::impl::rocks_db_file_storage::rocks_db_file_storage(const std::stri
     : db(init_db(filepath, readonly))
     , file(ffile)
     , instance_ids_(db.get(), "i|")
-    , instance_by_name_(&instance_ids_, [this](size_t v) { return assert_existance(v, entityinstance_ref); })
+    , instance_by_name_(
+        &instance_ids_,
+        [this](size_t v) { return assert_existance(v, entityinstance_ref); },
+        [this](size_t v) {
+            // The instance's keys are gone from the database; drop the
+            // cached handle so lookups don't keep resolving it.
+            std::lock_guard<std::mutex> lock(instance_cache_mutex_);
+            instance_cache_.erase((uint32_t)v);
+        })
     , bytype_(db.get(), "t|")
     , byguid_internal_(db.get(), "g|"),
       byguid_(&byguid_internal_, [this](size_t v) { return assert_existance(v, entityinstance_ref); }, [](const express::base& v) { return v.identity(); })
@@ -207,22 +215,21 @@ void ifcopenshell::impl::rocks_db_file_storage::process_deletion_inverse(const e
     auto id = inst.id();
 
     {
-        // compute next prefix that does not start with v|{id}|
+        // Delete every record referencing inst: all keys under v|{id}|. The
+        // prefix with its last byte incremented is the exclusive upper bound
+        // ('}' follows '|'), so no iterator is needed to find the range end.
         auto prefix = "v|" + std::to_string(id) + "|";
-        auto it = std::unique_ptr<rocksdb::Iterator>(db->NewIterator(rocksdb::ReadOptions()));
-        it->Seek(prefix);
-        while (it->Valid()) {
-            it->Next();
-            if (!it->key().starts_with(prefix)) {
-                break;
-            }
-        }
+        auto upper_bound = prefix;
+        upper_bound.back() = '}';
 
         rocksdb::WriteBatch batch;
-        batch.DeleteRange(prefix, it->key());
+        batch.DeleteRange(prefix, upper_bound);
         db->Write(wopts, &batch);
     }
 
+    // Delete the records inst contributed through its own attributes: drop
+    // its id from the value lists of every instance it references. The values
+    // are uint32_t ids, as written by the serializer and register_inverse().
     // This is based on traversal which needs instances to still be contained in the map.
     // another option would be to keep byid intact for the remainder of this loop
     auto entity_attributes = traverse(inst, 1);
@@ -233,26 +240,24 @@ void ifcopenshell::impl::rocks_db_file_storage::process_deletion_inverse(const e
         const unsigned int name = entity_attribute.id();
         // Do not update inverses for simple types (which have id()==0 in IfcOpenShell).
         if (name != 0) {
-            // Find instances entity -> other
-            // and update inverses from entity into other
+            auto prefix = "v|" + std::to_string(name) + "|";
+            auto it = std::unique_ptr<rocksdb::Iterator>(db->NewIterator(rocksdb::ReadOptions()));
+            it->Seek(prefix);
+            while (it->Valid() && it->key().starts_with(prefix)) {
+                std::string s = it->value().ToString();
 
-            {
-                auto prefix = "v|" + std::to_string(name) + "|";
-                auto it = std::unique_ptr<rocksdb::Iterator>(db->NewIterator(rocksdb::ReadOptions()));
-                it->Seek(prefix);
-                while (it->Valid() && it->key().starts_with(prefix)) {
-                    std::string s = it->value().ToString();
-
-                    // Iterator are snapshotted? So don't get invalidated?
-                    std::vector<size_t> vals(s.size() / sizeof(size_t));
-                    memcpy(vals.data(), s.data(), s.size());
-                    vals.erase(std::find(vals.begin(), vals.end(), (size_t)id));
-                    s.resize(vals.size() * sizeof(size_t));
+                // Iterator are snapshotted? So don't get invalidated?
+                std::vector<uint32_t> vals(s.size() / sizeof(uint32_t));
+                memcpy(vals.data(), s.data(), s.size());
+                auto removed = std::remove(vals.begin(), vals.end(), (uint32_t)id);
+                if (removed != vals.end()) {
+                    vals.erase(removed, vals.end());
+                    s.resize(vals.size() * sizeof(uint32_t));
                     memcpy(s.data(), vals.data(), s.size());
                     db->Put(wopts, it->key(), s);
-
-                    it->Next();
                 }
+
+                it->Next();
             }
         }
     }
@@ -314,19 +319,47 @@ ifcopenshell::filetype ifcopenshell::guess_file_type(const std::string& fn) {
 }
 
 express::base ifcopenshell::impl::rocks_db_file_storage::create(const ifcopenshell::declaration* decl, int id) {
+#ifndef IFOPSH_WITH_ROCKSDB
     (void)decl;
     (void)id;
-    return express::base{};
-    /*
-    if (decl->as_entity() || decl->as_type_declaration()) {
-        auto* inst = file->schema()->instantiate(decl, rocks_db_attribute_storage{});
-		// @todo maybe this needs to be set to file? In order to have a context (ie. rocksdb::db*) to write to?
-        inst->file_ = nullptr;
-        return file->add_entity(inst);
+    throw exception("RocksDB support not compiled in");
+#else
+    // Mirrors in_memory_file_storage::create(). The instance's attributes
+    // live in the database (written by set_attribute_value(), read back on
+    // access), so the cache only has to keep the identity of the handle
+    // stable: assert_existance() can reload it from the type record that
+    // add_type_ref() writes.
+    uint32_t instance_name;
+    if (decl->as_entity() != nullptr) {
+        if (id == -1) {
+            if (!id_counter_recalculated_) {
+                file->recalculate_id_counter();
+                id_counter_recalculated_ = true;
+            }
+            instance_name = file->fresh_id();
+        } else {
+            instance_name = id;
+        }
+    } else if (decl->as_type_declaration() != nullptr) {
+        instance_name = 0;
     } else {
         throw std::runtime_error("Requires and entity or type declaration");
     }
-    */
+    auto data = ifcopenshell::make_pointer_type<instance_data>(file, decl, instance_name, rocks_db_attribute_storage{});
+    {
+        std::lock_guard<std::mutex> lock(instance_cache_mutex_);
+        if (instance_name) {
+            instance_cache_.insert({instance_name, data});
+        } else {
+            type_instance_cache_.insert({data->identity(), data});
+        }
+    }
+
+    express::base inst(data);
+    add_type_ref(inst);
+
+    return inst;
+#endif
 }
 
 express::base ifcopenshell::impl::in_memory_file_storage::create(const ifcopenshell::declaration* decl, int id) {

src/ifcparse/parse.cpp

@@ -2636,16 +2636,32 @@ template void ifcopenshell::impl::in_memory_file_storage::read_from_stream(file_
 #endif
 
 void file::recalculate_id_counter() {
-    /*
-    // @todo
-    entity_by_id::key_type k = 0;
-    for (auto& p : byid_) {
-        if (p.first > k) {
-            k = p.first;
+    unsigned int k = 0;
+    std::visit([&k](auto& x) {
+        if constexpr (std::is_same_v<std::decay_t<decltype(x)>, impl::in_memory_file_storage>) {
+            for (const auto& p : x.byid_) {
+                k = std::max(k, (unsigned int)p.first);
+            }
         }
-    }
-    max_id_ = (unsigned int)k;
-    */
+#ifdef IFOPSH_WITH_ROCKSDB
+        else if constexpr (std::is_same_v<std::decay_t<decltype(x)>, impl::rocks_db_file_storage>) {
+            // Keys sort as text, so the largest id can't be found by seeking;
+            // scan the i|<id>|_ type records, one per entity instance.
+            const std::string prefix = "i|";
+            auto it = std::unique_ptr<rocksdb::Iterator>(x.db->NewIterator(rocksdb::ReadOptions()));
+            for (it->Seek(prefix); it->Valid() && it->key().starts_with(prefix); it->Next()) {
+                const auto key = it->key().ToString();
+                if (key.size() > 2 && key.compare(key.size() - 2, 2, "|_") == 0) {
+                    k = std::max(k, (unsigned int)std::stoul(key.substr(2, key.size() - 4)));
+                }
+            }
+        }
+#endif
+        else {
+            throw std::runtime_error("Storage not initialized");
+        }
+    }, storage_);
+    max_id_ = k;
 }
 
 class traversal_recorder {

src/ifcparse/rocksdb_set_view.h

@@ -23,6 +23,7 @@
 #ifdef IFOPSH_WITH_ROCKSDB
 #include <rocksdb/db.h>
 #include <rocksdb/options.h>
+#include <rocksdb/write_batch.h>
 #endif
 
 #include <memory>
@@ -196,6 +197,27 @@ public:
         return iterator();
     }
 
+    // Removes the element: every key under prefix + key + "|". The prefix
+    // with its last byte incremented is the exclusive upper bound ('}'
+    // follows '|'). Returns 1 if the element existed, 0 otherwise.
+    size_t erase(const key_type& key) {
+#ifdef IFOPSH_WITH_ROCKSDB
+        if (find(key) == end()) {
+            return 0;
+        }
+        const std::string lower_bound = prefix_ + key_to_string(key) + "|";
+        std::string upper_bound = lower_bound;
+        upper_bound.back() = '}';
+        rocksdb::WriteBatch batch;
+        batch.DeleteRange(lower_bound, upper_bound);
+        db_->Write(rocksdb::WriteOptions{}, &batch);
+        return 1;
+#else
+        static_cast<void>(key);
+        return 0;
+#endif
+    }
+
     // Read-only find: returns an iterator to the element with the given key if it exists.
     iterator find(const key_type& key) const {
 #ifdef IFOPSH_WITH_ROCKSDB
@@ -216,11 +238,6 @@ public:
         return end();
     }
 
-    size_t erase(const key_type& key) {
-        static_cast<void>(key);
-        // @todo
-        return 0;
-    }
 };
 
 #endif

src/ifcparse/set_to_map_transformer.h

@@ -36,11 +36,17 @@ public:
 private:
     BaseSet* base_map_;
     Transform transform_;
+    std::function<void(const key_type&)> on_erase_;
 
 public:
     set_to_map_transformer(BaseSet* base_set, Transform transform)
         : base_map_(base_set), transform_(transform) {}
 
+    // on_erase runs after erase(key), whether or not the base set still
+    // held the key, so state derived from the set can be dropped.
+    set_to_map_transformer(BaseSet* base_set, Transform transform, std::function<void(const key_type&)> on_erase)
+        : base_map_(base_set), transform_(transform), on_erase_(std::move(on_erase)) {}
+
     class iterator {
     public:
         using base_iterator = typename BaseSet::iterator;
@@ -107,8 +113,10 @@ public:
     }
 
     size_t erase(const key_type& key) {
-        static_cast<void>(key);
-        // @todo
-        return 0;
+        const size_t erased = base_map_->erase(key);
+        if (on_erase_) {
+            on_erase_(key);
+        }
+        return erased;
     }
 };

src/ifcparse/storage.h

@@ -678,6 +678,9 @@ namespace ifcopenshell {
             typedef std::map<uint32_t, shared_pointer_type> entity_by_iden_cache;
             entity_by_iden_cache instance_cache_, type_instance_cache_;
             std::mutex instance_cache_mutex_;
+            // Opening a database doesn't visit every instance, so the file's
+            // id counter is recalculated on the first create().
+            bool id_counter_recalculated_ = false;
 
             // @todo all these size_ts should probably be uint32_t for consistency with in-mem storage