[AI-generated, unverified] ifcparse: make deleting and creating instances work on RocksDB-backed files (#9508)

* 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

* ifcparse: map argument_type to its stored type, size() as size_t

Review: argument_type enumerates the members of type_variant_parameter_pack
in order, so express that once as argument_storage_type_t<A> (pinned by
static_asserts) and let attribute_value::size() on RocksDB go through a
single aggregate_size_<A>() helper instead of spelling each vector type
out in the switch. size() now returns size_t; its only caller already
took size_t.

Also build the RocksDB DeleteRange upper bounds as prefix + ('|' + 1)
rather than a literal '}', which read as the {id} placeholder notation.

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

* ifcparse: fixed-width hex ids in RocksDB keys, batched instance erasure

Review: recalculate_id_counter() scanned every i|<id>|_ record because
ids were written as decimal text, which doesn't sort numerically. Make
every numeric key segment fixed-width 16-digit lowercase hex, produced
and parsed by key_to_string()/key_from_string() in rocksdb_map_adapter.h,
with named builders (rocksdb_key::attribute, header_attribute,
type_record, inverse, inverse_prefix, type_list, upper_bound) that the
storage, entity_instance_data.cpp, read_schema() and the serializer use
instead of assembling "i|" + std::to_string(id) + ... by hand. Keys now
sort by id, an instance's records are contiguous in id order, and the
largest id is the last key under i|, which recalculate_id_counter()
seeks to. The two dormant to_string_fixed_width() helpers are gone.

This changes the on-disk layout; databases converted before this commit
have to be re-converted.

Also review: instance_cache_ eviction went through an on-erase hook on
set_to_map_transformer, a std::function call under a mutex per deleted
instance. Drop the hook; file::remove_entity() and unbatch() call
file::erase_instances_(ids), which on RocksDB is
rocks_db_file_storage::erase_instances(): one WriteBatch of DeleteRanges
and one lock for the whole batch.

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

* ifcopenshell-python: test_rocks queries the fixed-width hex keys

The test looked up raw keys in the decimal layout ("i|139|5",
"t|<identity>|0"); numeric key segments are fixed-width hex now.

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

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

src/ifcopenshell-python/test/test_streaming_rocksdb_and_simpletyperefs.py

@@ -111,9 +111,11 @@ def test_rocks():
         assert f[139].RelatingPropertyDefinition.is_a("IfcPropertySetDefinitionSet")
         assert {x.id() for x in f[139].RelatingPropertyDefinition[0]} == {136, 138}
 
-        b = f.key_value_store_query("i|139|5")[2:]
+        # Numeric key segments are fixed-width hex: i|<id>|<attribute>,
+        # t|<identity>|<attribute>. See rocksdb_map_adapter.h.
+        b = f.key_value_store_query(f"i|{139:016x}|{5:016x}")[2:]
         iden = struct.unpack("Q", b)[0]
-        b = f.key_value_store_query(f"t|{iden}|0")[1:]
+        b = f.key_value_store_query(f"t|{iden:016x}|{0:016x}")[1:]
         assert set(struct.unpack("Q", b[i : i + 8])[0] for i in range(1, len(b), 9)) == {136, 138}
 
         g = ifcopenshell.open(fn)

src/ifcparse/entity_instance_data.cpp

@@ -65,9 +65,7 @@ namespace {
             {
                 std::string str;
                 array_.db_ptr->db->Get(rocksdb::ReadOptions{},
-                    (is_header ? "h|" : (entity_or_type->as_entity() ? "i|" : "t|")) +
-                    (is_header ? entity_or_type->name() : std::to_string(instance_name_)) + "|" +
-                    std::to_string(index_), &str);
+                    (is_header ? rocksdb_key::header_attribute(entity_or_type->name(), index_) : rocksdb_key::attribute(entity_or_type->as_entity() != nullptr, instance_name_, index_)), &str);
                 ::impl::deserialize(array_.db_ptr, str, val);
             } else {
                 static_assert(
@@ -97,9 +95,7 @@ namespace {
             std::string str;
             const bool is_header = entity_or_type->schema() == &Header_section_schema::get_schema();
             array_.db_ptr->db->Get(rocksdb::ReadOptions{},
-                (is_header ? "h|" : (entity_or_type->as_entity() ? "i|" : "t|")) +
-                (is_header ? entity_or_type->name() : std::to_string(instance_name_)) + "|" +
-                std::to_string(index_), &str);
+                (is_header ? rocksdb_key::header_attribute(entity_or_type->name(), index_) : rocksdb_key::attribute(entity_or_type->as_entity() != nullptr, instance_name_, index_)), &str);
             if constexpr (std::is_same_v<T, blank>) {
                 if (str.size() == 0) {
                     return true;
@@ -125,9 +121,7 @@ namespace {
             std::string str;
             const bool is_header = entity_or_type->schema() == &Header_section_schema::get_schema();
             if (!array_.db_ptr->db->Get(rocksdb::ReadOptions{},
-                (is_header ? "h|" : (entity_or_type->as_entity() ? "i|" : "t|")) +
-                (is_header ? entity_or_type->name() : std::to_string(instance_name_)) + "|" +
-                std::to_string(index_), &str).ok()) {
+                (is_header ? rocksdb_key::header_attribute(entity_or_type->name(), index_) : rocksdb_key::attribute(entity_or_type->as_entity() != nullptr, instance_name_, index_)), &str).ok()) {
                 return type_encoder::encode_type<blank>() - 'A';
             }
             return (size_t) str[0] - 'A';
@@ -135,6 +129,12 @@ namespace {
 #endif
         throw std::logic_error("RocksDB storage is unavailable");
     }
+
+    template<argument_type A>
+    inline size_t aggregate_size_(attribute_value::pointer_type array_, uint8_t storage_model_, size_t instance_name_, const ifcopenshell::declaration* entity_or_type, uint8_t index_)
+    {
+        return dispatch_get_<argument_storage_type_t<A>>(array_, storage_model_, instance_name_, entity_or_type, index_).size();
+    }
 }
 
 attribute_value::operator int64_t() const
@@ -173,9 +173,7 @@ attribute_value::operator std::string() const
             std::string str;
             const bool is_header = entity_or_type_->schema() == &Header_section_schema::get_schema();
             array_.db_ptr->db->Get(rocksdb::ReadOptions{},
-                (is_header ? "h|" : (entity_or_type_->as_entity() ? "i|" : "t|")) +
-                (is_header ? entity_or_type_->name() : std::to_string(instance_name_)) + "|" +
-                std::to_string(index_), &str);
+                (is_header ? rocksdb_key::header_attribute(entity_or_type_->name(), index_) : rocksdb_key::attribute(entity_or_type_->as_entity() != nullptr, instance_name_, index_)), &str);
             size_t v;
             memcpy(&v, str.data() + 1, sizeof(size_t));
             auto decl = array_.db_ptr->file->schema()->declarations()[v]->as_enumeration_type();
@@ -197,9 +195,7 @@ attribute_value::operator enumeration_reference() const
         std::string str;
         const bool is_header = entity_or_type_->schema() == &Header_section_schema::get_schema();
         array_.db_ptr->db->Get(rocksdb::ReadOptions{},
-            (is_header ? "h|" : (entity_or_type_->as_entity() ? "i|" : "t|")) +
-            (is_header ? entity_or_type_->name() : std::to_string(instance_name_)) + "|" +
-            std::to_string(index_), &str);
+            (is_header ? rocksdb_key::header_attribute(entity_or_type_->name(), index_) : rocksdb_key::attribute(entity_or_type_->as_entity() != nullptr, instance_name_, index_)), &str);
         size_t v;
         memcpy(&v, str.data() + 1, sizeof(size_t));
         auto decl = array_.db_ptr->file->schema()->declarations()[v]->as_enumeration_type();
@@ -225,9 +221,7 @@ attribute_value::operator express::base () const
         std::string str;
         const bool is_header = entity_or_type_->schema() == &Header_section_schema::get_schema();
         array_.db_ptr->db->Get(rocksdb::ReadOptions{},
-            (is_header ? "h|" : (entity_or_type_->as_entity() ? "i|" : "t|")) +
-            (is_header ? entity_or_type_->name() : std::to_string(instance_name_)) + "|" +
-            std::to_string(index_), &str);
+            (is_header ? rocksdb_key::header_attribute(entity_or_type_->name(), index_) : rocksdb_key::attribute(entity_or_type_->as_entity() != nullptr, instance_name_, index_)), &str);
         size_t v;
         memcpy(&v, str.data() + 2, sizeof(size_t));
         if (str.size() > 1 && str[1] == 'i') {
@@ -289,10 +283,40 @@ bool attribute_value::isNull() const
     return dispatch_has_<blank>(array_, storage_model_, instance_name_, entity_or_type_, index_);
 }
 
-unsigned int attribute_value::size() const
+size_t attribute_value::size() const
 {
-    // @todo
-    return array_.storage_ptr->apply_visitor(size_visitor{}, index_);
+    if (storage_model_ == 0) {
+        return (size_t)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 aggregate_size_<Argument_AGGREGATE_OF_INT>(array_, storage_model_, instance_name_, entity_or_type_, index_);
+        case Argument_AGGREGATE_OF_DOUBLE:
+            return aggregate_size_<Argument_AGGREGATE_OF_DOUBLE>(array_, storage_model_, instance_name_, entity_or_type_, index_);
+        case Argument_AGGREGATE_OF_STRING:
+            return aggregate_size_<Argument_AGGREGATE_OF_STRING>(array_, storage_model_, instance_name_, entity_or_type_, index_);
+        case Argument_AGGREGATE_OF_BINARY:
+            return aggregate_size_<Argument_AGGREGATE_OF_BINARY>(array_, storage_model_, instance_name_, entity_or_type_, index_);
+        case Argument_AGGREGATE_OF_ENTITY_INSTANCE:
+            return aggregate_size_<Argument_AGGREGATE_OF_ENTITY_INSTANCE>(array_, storage_model_, instance_name_, entity_or_type_, index_);
+        case Argument_AGGREGATE_OF_AGGREGATE_OF_INT:
+            return aggregate_size_<Argument_AGGREGATE_OF_AGGREGATE_OF_INT>(array_, storage_model_, instance_name_, entity_or_type_, index_);
+        case Argument_AGGREGATE_OF_AGGREGATE_OF_DOUBLE:
+            return aggregate_size_<Argument_AGGREGATE_OF_AGGREGATE_OF_DOUBLE>(array_, storage_model_, instance_name_, entity_or_type_, index_);
+        case Argument_AGGREGATE_OF_AGGREGATE_OF_ENTITY_INSTANCE:
+            return aggregate_size_<Argument_AGGREGATE_OF_AGGREGATE_OF_ENTITY_INSTANCE>(array_, storage_model_, instance_name_, entity_or_type_, index_);
+        default:
+            return (size_t)-1;
+        }
+    }
+#endif
+    throw std::logic_error("RocksDB storage is unavailable");
 }
 
 ifcopenshell::argument_type attribute_value::type() const
@@ -512,9 +536,7 @@ bool rocks_db_attribute_storage::has(void* storage, const ifcopenshell::declarat
     std::string v;
     auto success = rdb_storage->db->Get(
         rocksdb::ReadOptions{},
-        (is_header ? "h|" : (decl->as_entity() ? "i|" : "t|")) +
-        (is_header ? decl->name() : std::to_string(identity)) + "|" +
-        std::to_string(index), &v);
+        (is_header ? rocksdb_key::header_attribute(decl->name(), index) : rocksdb_key::attribute(decl->as_entity() != nullptr, identity, index)), &v);
     if constexpr (std::is_same_v<std::decay_t<T>, blank>) {
         if (!success.ok()) {
             return true;
@@ -532,9 +554,7 @@ void rocks_db_attribute_storage::set(void* storage, const ifcopenshell::declarat
     ::impl::serialize(v, value);
     rdb_storage->db->Put(
         rdb_storage->wopts,
-        (is_header ? "h|" : (decl->as_entity() ? "i|" : "t|")) +
-        (is_header ? decl->name() : std::to_string(identity)) + "|" +
-        std::to_string(index), v);
+        (is_header ? rocksdb_key::header_attribute(decl->name(), index) : rocksdb_key::attribute(decl->as_entity() != nullptr, identity, index)), v);
 }
 
 template IFC_PARSE_API void rocks_db_attribute_storage::set<blank>(void* storage, const ifcopenshell::declaration* decl, std::size_t identity, size_t index, const blank& value);

src/ifcparse/file.cpp

@@ -55,7 +55,7 @@ express::base ifcopenshell::impl::rocks_db_file_storage::assert_existance(size_t
 
     std::string v;
 
-    rocksdb::Status s = db->Get(rocksdb::ReadOptions{}, (r == entityinstance_ref ? "i|" : "t|") + std::to_string(number) + "|_", &v);
+    rocksdb::Status s = db->Get(rocksdb::ReadOptions{}, rocksdb_key::type_record(r == entityinstance_ref, number), &v);
     if (s.ok()) {
         size_t s;
         memcpy(&s, v.data(), sizeof(size_t));
@@ -207,22 +207,20 @@ 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}|
-        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;
-            }
-        }
+        // Delete every record referencing inst: all keys under v|<id>|. The
+        // exclusive upper bound is the same prefix with its separator
+        // incremented, so no iterator is needed to find the range end.
+        const auto prefix = rocksdb_key::inverse_prefix(id);
+        const auto upper_bound = rocksdb_key::upper_bound(prefix);
 
         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,32 +231,51 @@ 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 = rocksdb_key::inverse_prefix(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();
             }
         }
     }
 #endif
 }
 
+void ifcopenshell::impl::rocks_db_file_storage::erase_instances(const std::vector<uint32_t>& ids)
+{
+#ifndef IFOPSH_WITH_ROCKSDB
+    (void)ids;
+#endif
+#ifdef IFOPSH_WITH_ROCKSDB
+    // One write for every instance's keys, one lock for their cached handles.
+    rocksdb::WriteBatch batch;
+    for (auto id : ids) {
+        const auto prefix = rocksdb_key::instance(true, id);
+        batch.DeleteRange(prefix, rocksdb_key::upper_bound(prefix));
+    }
+    db->Write(wopts, &batch);
+
+    std::lock_guard<std::mutex> lock(instance_cache_mutex_);
+    for (auto id : ids) {
+        instance_cache_.erase(id);
+    }
+#endif
+}
+
 express::base ifcopenshell::impl::in_memory_file_storage::instance_by_id(int id)
 {
     auto it = byid_.find(id);
@@ -314,19 +331,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/file.h

@@ -234,6 +234,7 @@ public:
     batch_deletion_ids_t batch_deletion_ids_;
     bool batch_mode_ = false;
     void process_deletion_(const express::base& entity);
+    void erase_instances_(const std::vector<uint32_t>& ids);
 
   public:
 #ifdef USE_MMAP

src/ifcparse/instance_data.h

@@ -32,6 +32,7 @@
 #undef Handle
 
 #include <rocksdb/db.h>
+#include <tuple>
 
 #pragma pop_macro("Handle")
 
@@ -226,6 +227,24 @@ struct pack_to_variant_array<parameter_pack<Args...>> {
 
 using in_memory_attribute_storage = pack_to_variant_array<type_variant_parameter_pack>::type;
 
+// argument_type enumerates the members of type_variant_parameter_pack in
+// order, so a member maps back to the type stored for it.
+template <typename Pack>
+struct pack_element;
+
+template <typename... Args>
+struct pack_element<parameter_pack<Args...>> {
+    template <size_t I>
+    using type = std::tuple_element_t<I, std::tuple<Args...>>;
+};
+
+template <argument_type A>
+using argument_storage_type_t = typename pack_element<type_variant_parameter_pack>::template type<A>;
+
+static_assert(std::is_same_v<argument_storage_type_t<Argument_INT>, int64_t>, "argument_type must enumerate type_variant_parameter_pack in order");
+static_assert(std::is_same_v<argument_storage_type_t<Argument_AGGREGATE_OF_INT>, std::vector<int64_t>>, "argument_type must enumerate type_variant_parameter_pack in order");
+static_assert(std::is_same_v<argument_storage_type_t<Argument_AGGREGATE_OF_AGGREGATE_OF_ENTITY_INSTANCE>, std::vector<std::vector<express::base>>>, "argument_type must enumerate type_variant_parameter_pack in order");
+
 template <typename Pack>
 struct type_encoder_impl;
 
@@ -427,7 +446,7 @@ public:
     operator enumeration_reference() const;
 
     bool isNull() const;
-    unsigned int size() const;
+    size_t size() const;
 
     ifcopenshell::argument_type type() const;
 

src/ifcparse/parse.cpp

@@ -1099,16 +1099,6 @@ void ifcopenshell::impl::in_memory_file_storage::unregister_inverse(unsigned id_
     }
 }
 
-namespace {
-    template <typename T>
-    std::string to_string_fixed_width(const T& t, size_t) {
-        // @todo currently inactive
-        std::ostringstream oss;
-        oss << /*std::setfill('0') << std::setw(w) <<*/ t;
-        return oss.str();
-    }
-}
-
 void ifcopenshell::impl::rocks_db_file_storage::register_inverse(unsigned id_from, const ifcopenshell::entity* from_entity, int inst_id, int attribute_index) {
 #ifndef IFOPSH_WITH_ROCKSDB
     (void)id_from;
@@ -1122,7 +1112,7 @@ void ifcopenshell::impl::rocks_db_file_storage::register_inverse(unsigned id_fro
     s.resize(sizeof(uint32_t));
     memcpy(s.data(), &v, sizeof(uint32_t));
 
-    auto key = "v|" + to_string_fixed_width(inst_id, 10) + "|" + to_string_fixed_width(from_entity->index_in_schema(), 4) + "|" + to_string_fixed_width(attribute_index, 2);
+    auto key = rocksdb_key::inverse(inst_id, from_entity->index_in_schema(), attribute_index);
 
     db->Merge(wopts, key, s);
     /*
@@ -1147,7 +1137,7 @@ void ifcopenshell::impl::rocks_db_file_storage::unregister_inverse(unsigned id_f
 #ifdef IFOPSH_WITH_ROCKSDB
     static std::string s;
     auto inst_id = inst.id();
-    auto key = "v|" + to_string_fixed_width(inst_id, 10) + "|" + to_string_fixed_width(from_entity->index_in_schema(), 4) + "|" + to_string_fixed_width(attribute_index, 2);
+    auto key = rocksdb_key::inverse(inst_id, from_entity->index_in_schema(), attribute_index);
     if (db->Get(rocksdb::ReadOptions{}, key, &s).ok()) {
         std::vector<uint32_t> vals(s.size() / sizeof(uint32_t));
         memcpy(vals.data(), s.data(), s.size());
@@ -1178,12 +1168,12 @@ void ifcopenshell::impl::rocks_db_file_storage::add_type_ref(const express::base
         memcpy(s.data(), &v, sizeof(size_t));
 
         // no merges yet, because the python client doesn't support them
-        db->Merge(wopts, "t|" + std::to_string(new_entity.declaration().index_in_schema()), s);
+        db->Merge(wopts, rocksdb_key::type_list(new_entity.declaration().index_in_schema()), s);
 
         /*{
             std::string current;
             // @todo this uses the same key-namespace as typedecl instances, not a direct conflict, but also not very clear
-            auto key = "t|" + std::to_string(new_entity.declaration().index_in_schema());
+            auto key = rocksdb_key::type_list(new_entity.declaration().index_in_schema());
             db->Get(rocksdb::ReadOptions{}, key, &current);
             auto new_val = current + s;
             db->Put(wopts, key, new_val);
@@ -1193,7 +1183,7 @@ void ifcopenshell::impl::rocks_db_file_storage::add_type_ref(const express::base
     // not only mapping also register type
     v = new_entity.declaration().index_in_schema();
     memcpy(s.data(), &v, sizeof(size_t));
-    db->Put(wopts, (new_entity.declaration().as_entity() ? "i|" : "t|") + std::to_string(new_entity.id() ? new_entity.id() : new_entity.identity()) + "|_", s);
+    db->Put(wopts, rocksdb_key::type_record(new_entity.declaration().as_entity() != nullptr, new_entity.id() ? new_entity.id() : new_entity.identity()), s);
 #endif
 }
 
@@ -1205,7 +1195,7 @@ void ifcopenshell::impl::rocks_db_file_storage::remove_type_ref(const express::b
 #ifdef IFOPSH_WITH_ROCKSDB
     if (new_entity.declaration().as_entity()) {
         std::string s;
-        auto key = "t|" + std::to_string(new_entity.declaration().index_in_schema());
+        auto key = rocksdb_key::type_list(new_entity.declaration().index_in_schema());
         if (db->Get(rocksdb::ReadOptions{}, key, &s).ok()) {
             std::vector<size_t> vals(s.size() / sizeof(size_t));
             memcpy(vals.data(), s.data(), s.size());
@@ -1216,7 +1206,7 @@ void ifcopenshell::impl::rocks_db_file_storage::remove_type_ref(const express::b
         }
     }
 
-    db->Delete(wopts, (new_entity.declaration().as_entity() ? "i|" : "t|") + std::to_string(new_entity.id() ? new_entity.id() : new_entity.identity()) + "|_");
+    db->Delete(wopts, rocksdb_key::type_record(new_entity.declaration().as_entity() != nullptr, new_entity.id() ? new_entity.id() : new_entity.identity()));
 #endif
 }
 
@@ -2636,16 +2626,30 @@ 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>) {
+            // Ids are fixed-width hex in the keys, so the largest id owns the
+            // last key under the entity prefix.
+            const std::string prefix = "i|";
+            auto it = std::unique_ptr<rocksdb::Iterator>(x.db->NewIterator(rocksdb::ReadOptions()));
+            it->SeekForPrev(rocksdb_key::upper_bound(prefix));
+            if (it->Valid() && it->key().starts_with(prefix)) {
+                k = (unsigned int)parse_hex_key(it->key().ToString().substr(prefix.size(), 16));
+            }
+        }
+#endif
+        else {
+            throw std::runtime_error("Storage not initialized");
+        }
+    }, storage_);
+    max_id_ = k;
 }
 
 class traversal_recorder {
@@ -2940,10 +2944,24 @@ void file::remove_entity(const express::base& entity) {
         batch_deletion_ids_.push_back(id);
     } else {
         process_deletion_(entity);
-        byid_.erase(entity.id());
+        erase_instances_({(uint32_t)id});
     }
 }
 
+void file::erase_instances_(const std::vector<uint32_t>& ids) {
+    std::visit([this, &ids](auto& x) {
+        if constexpr (std::is_same_v<std::decay_t<decltype(x)>, impl::in_memory_file_storage>) {
+            for (auto id : ids) {
+                byid_.erase(id);
+            }
+        } else if constexpr (std::is_same_v<std::decay_t<decltype(x)>, impl::rocks_db_file_storage>) {
+            x.erase_instances(ids);
+        } else {
+            throw std::runtime_error("Storage not initialized");
+        }
+    }, storage_);
+}
+
 void file::process_deletion_(const express::base& entity) {
 
     auto references = instances_by_reference(entity.id());
@@ -3127,7 +3145,7 @@ std::vector<express::base> file::instances_by_reference(int t) {
 #ifdef IFOPSH_WITH_ROCKSDB
         else if constexpr (std::is_same_v<std::decay_t<decltype(x)>, impl::rocks_db_file_storage>) {
             // @todo no lower/upper_bounds() implemented yet
-            auto prefix = "v|" + std::to_string(t) + "|";
+            auto prefix = rocksdb_key::inverse_prefix(t);
             auto it = std::unique_ptr<rocksdb::Iterator>(x.db->NewIterator(rocksdb::ReadOptions()));
             it->Seek(prefix);
             while (it->Valid() && it->key().starts_with(prefix)) {
@@ -3285,7 +3303,7 @@ std::vector<int> file::get_inverse_indices_by_id(int instance_id) {
         } else if constexpr (std::is_same_v<std::decay_t<decltype(x)>, impl::rocks_db_file_storage>) {
 #ifdef IFOPSH_WITH_ROCKSDB
             // @todo no lower/upper_bounds() implemented yet
-            auto prefix = "v|" + std::to_string(instance_id) + "|";
+            auto prefix = rocksdb_key::inverse_prefix(instance_id);
             auto it = std::unique_ptr<rocksdb::Iterator>(x.db->NewIterator(rocksdb::ReadOptions()));
             it->Seek(prefix);
             while (it->Valid() && it->key().starts_with(prefix)) {
@@ -3358,7 +3376,7 @@ std::vector<express::entity> file::get_inverse(int instance_id, const ifcopenshe
             visit_subtypes(type->as_entity(), [this, attribute_index, instance_id, &return_value, &x](const ifcopenshell::declaration* ent) {
                 if (attribute_index == -1) {
                     // @todo no lower/upper_bounds() implemented yet
-                    auto prefix = "v|" + std::to_string(instance_id) + "|" + std::to_string(ent->index_in_schema()) + "|";
+                    auto prefix = rocksdb_key::inverse_prefix(instance_id) + key_to_string(ent->index_in_schema()) + "|";
                     auto it = std::unique_ptr<rocksdb::Iterator>(x.db->NewIterator(rocksdb::ReadOptions()));
                     it->Seek(prefix);
                     while (it->Valid() && it->key().starts_with(prefix)) {
@@ -3507,9 +3525,7 @@ void ifcopenshell::file::unbatch() {
         process_deletion_(instance_by_id(id));
     }
     // keep in memory until all deletions are processed
-    for (auto& id : batch_deletion_ids_) {
-        byid_.erase(id);
-    }
+    erase_instances_(std::vector<uint32_t>(batch_deletion_ids_.begin(), batch_deletion_ids_.end()));
     batch_mode_ = false;
     batch_deletion_ids_.clear();
 }
@@ -3613,7 +3629,7 @@ bool ifcopenshell::impl::rocks_db_file_storage::read_schema(const ifcopenshell::
 #endif
 #ifdef IFOPSH_WITH_ROCKSDB
     std::string value;
-    auto key = "h|file_schema|0";
+    const auto key = rocksdb_key::header_attribute("file_schema", 0);
     db->Get(rocksdb::ReadOptions{}, key, &value);
     std::vector<std::string> strings;
     if (::impl::deserialize(this, value, strings) && strings.size() == 1) {

src/ifcparse/rocksdb_map_adapter.h

@@ -105,12 +105,64 @@ struct DefaultCodec<std::string> {
     }
 };
 
+// Numeric key segments are fixed-width lowercase hex, so keys sort as
+// numbers: an instance's records are laid out in id order and the largest
+// id under a prefix is its last key.
+inline std::string hex_key(uint64_t value) {
+    static constexpr char digits[] = "0123456789abcdef";
+    std::string s(16, '0');
+    for (int i = 15; i >= 0; --i) {
+        s[(size_t)i] = digits[value & 0xf];
+        value >>= 4;
+    }
+    return s;
+}
+
+inline uint64_t parse_hex_key(const std::string& s) {
+    return std::stoull(s, nullptr, 16);
+}
+
 template <typename KeyT>
 std::string key_to_string(const KeyT& key) {
     if constexpr (std::is_same_v<KeyT, std::string>) {
         return key;
     } else {
-        return std::to_string(key);
+        static_assert(std::is_integral_v<KeyT>, "key_to_string expects a string or an integral key");
+        return hex_key((uint64_t)key);
+    }
+}
+
+// The keys the file storage and the serializer agree on:
+//   i|<id>|<attribute>   t|<identity>|<attribute>   h|<name>|<attribute>
+//   i|<id>|_             t|<identity>|_             type record
+//   v|<referenced id>|<entity index>|<attribute>    inverse record
+//   t|<declaration index>                           instances of a type
+namespace rocksdb_key {
+    // The exclusive end of everything under prefix.
+    inline std::string upper_bound(std::string prefix) {
+        prefix.back() = (char)(prefix.back() + 1);
+        return prefix;
+    }
+    inline std::string instance(bool is_entity, uint64_t id) {
+        return (is_entity ? "i|" : "t|") + key_to_string(id) + "|";
+    }
+    inline std::string attribute(bool is_entity, uint64_t id, uint64_t index) {
+        return instance(is_entity, id) + key_to_string(index);
+    }
+    inline std::string header_attribute(const std::string& name, uint64_t index) {
+        return "h|" + name + "|" + key_to_string(index);
+    }
+    inline std::string type_record(bool is_entity, uint64_t id) {
+        return instance(is_entity, id) + "_";
+    }
+    inline std::string inverse_prefix(uint64_t referenced_id) {
+        return "v|" + key_to_string(referenced_id) + "|";
+    }
+    inline std::string inverse(uint64_t referenced_id, uint64_t entity_index, uint64_t attribute_index) {
+        return inverse_prefix(referenced_id) + key_to_string(entity_index) + "|" + key_to_string(attribute_index);
+    }
+    inline std::string type_list(uint64_t declaration_index) {
+        return "t|" + key_to_string(declaration_index);
     }
 }
 
@@ -121,7 +173,7 @@ KeyT key_from_string(const std::string& key_string) {
     if constexpr (std::is_same_v<KeyT, std::string>) {
         return key_string;
     } else if constexpr (std::is_integral_v<KeyT>) {
-        return static_cast<KeyT>(std::stoll(key_string));
+        return static_cast<KeyT>(parse_hex_key(key_string));
     } else {
         static_assert(sizeof(KeyT) == 0, "key_from_string not implemented for this type");
     }
@@ -132,7 +184,7 @@ std::string tuple_to_string_impl(const Tuple& tuple_value, std::index_sequence<I
     static_cast<void>(indices);
     std::ostringstream oss;
     // Unpack the tuple; add a pipe before each element except the first.
-    ((oss << (Is == 0 ? "" : "|") << std::to_string(std::get<Is>(tuple_value))), ...);
+    ((oss << (Is == 0 ? "" : "|") << key_to_string(std::get<Is>(tuple_value))), ...);
     return oss.str();
 }
 
@@ -145,7 +197,7 @@ std::string key_to_string(const std::tuple<Ts...>& key) {
 template<typename T>
 T convert_string(const std::string& token) {
     if constexpr (std::is_integral_v<T>) {
-        return static_cast<T>(std::stoll(token));
+        return static_cast<T>(parse_hex_key(token));
     } else if constexpr (std::is_floating_point_v<T>) {
         return static_cast<T>(std::stod(token));
     } else {

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,26 @@ public:
         return iterator();
     }
 
+    // Removes the element: every key under prefix + key + "|". The exclusive
+    // upper bound is the same prefix with its separator incremented. 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) + "|";
+        const std::string upper_bound = prefix_ + key_to_string(key) + std::string(1, '|' + 1);
+        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 +237,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

@@ -107,8 +107,6 @@ public:
     }
 
     size_t erase(const key_type& key) {
-        static_cast<void>(key);
-        // @todo
-        return 0;
+        return base_map_->erase(key);
     }
 };

src/ifcparse/storage.h

@@ -678,6 +678,13 @@ 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;
+
+            // Deletes every key of the given instances and drops their cached
+            // handles, in one write and under one lock.
+            void erase_instances(const std::vector<uint32_t>& ids);
 
             // @todo all these size_ts should probably be uint32_t for consistency with in-mem storage
 

src/serializers/rocks_db_serializer.cpp

@@ -98,16 +98,6 @@ namespace {
 	}
 }
 
-namespace {
-	template <typename T>
-	std::string to_string_fixed_width(const T& t, size_t w) {
-		// @todo currently inactive
-		std::ostringstream oss;
-		oss << /*std::setfill('0') << std::setw(w) <<*/ t;
-		return oss.str();
-	}
-}
-
 void RocksDbSerializer::write_streaming_() {
 	ifcopenshell::impl::rocks_db_file_storage storage(rocksdb_filename_, nullptr);
 
@@ -170,15 +160,13 @@ void RocksDbSerializer::write_streaming_() {
 				// @nb cast to int in order not be interpreted as a char when appending to string
 				int index = p.first.index_;
 
-				auto key = (is_header ? "h|" : (decl->as_entity() ? "i|" : "t|")) +
-					(is_header ? decl->name() : std::to_string(p.first.name_)) + "|" +
-					std::to_string(index);
+				auto key = (is_header ? rocksdb_key::header_attribute(decl->name(), index) : rocksdb_key::attribute(decl->as_entity() != nullptr, p.first.name_, index));
 
 				if (storage.db->Get(storage.ropts, key, &tmp) == rocksdb::Status::OK() && tmp.size() == (sizeof(size_t) + 2) && tmp[0] == ifcopenshell::type_encoder::encode_type<express::base>() && tmp[1] == 't')
 				{
 					size_t iden;
 					memcpy(&iden, tmp.data() + 2, sizeof(size_t));
-					key = "t|" + std::to_string(iden) + "|0";
+					key = rocksdb_key::attribute(false, iden, 0);
 					type_identities_wrote_as_refs.insert(iden);
 				}
 
@@ -215,7 +203,7 @@ void RocksDbSerializer::write_streaming_() {
 
 				auto write_inverse = [&](const ifcopenshell::reference_or_simple_type& v) {
 					if (auto* ref = std::get_if<ifcopenshell::instance_reference>(&v)) {
-						auto key = "v|" + to_string_fixed_width(*ref, 10) + "|" + to_string_fixed_width(decl->index_in_schema(), 4) + "|" + to_string_fixed_width(index, 2);
+						auto key = rocksdb_key::inverse(*ref, decl->index_in_schema(), index);
 						static std::string s;
 						uint32_t vv = name;
 						s.resize(sizeof(uint32_t));
@@ -247,7 +235,7 @@ void RocksDbSerializer::write_streaming_() {
 
 				storage.db->Put(
 					storage.wopts,
-					(inst.declaration().as_entity() ? "i|" : "t|") + std::to_string(inst.identity()) + "|_", s);
+					rocksdb_key::type_record(inst.declaration().as_entity() != nullptr, inst.identity()), s);
 
 				if (type_identities_wrote_as_refs.find(inst.identity()) != type_identities_wrote_as_refs.end()) {
 					// already written as reference, skip
@@ -275,13 +263,13 @@ void RocksDbSerializer::write_streaming_() {
 				memcpy(s.data(), &v, sizeof(size_t));
 				storage.db->Put(
 					storage.wopts,
-					(decl->as_entity() ? "i|" : "t|") + std::to_string(name) + "|_", s);
+					rocksdb_key::type_record(decl->as_entity() != nullptr, name), s);
 
 				{
 					size_t v = name;
 					std::string s(sizeof(size_t), ' ');
 					memcpy(s.data(), &v, sizeof(size_t));
-					storage.db->Merge(storage.wopts, "t|" + std::to_string(decl->index_in_schema()), s);
+					storage.db->Merge(storage.wopts, rocksdb_key::type_list(decl->index_in_schema()), s);
 				}
 
 				// GlobalId as numeric ref to instance name, so that the guid map in