diff --git a/src/ifcparse/parse.cpp b/src/ifcparse/parse.cpp index 38af6a04e4..5917ae89c3 100644 --- a/src/ifcparse/parse.cpp +++ b/src/ifcparse/parse.cpp @@ -2940,7 +2940,7 @@ void file::process_deletion_(const express::base& entity) { // entity being deleted are not deleted themselves. if (!references.empty()) { for (auto& related_instance : references) { - if (std::find(batch_deletion_ids_.begin(), batch_deletion_ids_.end(), related_instance.id()) != batch_deletion_ids_.end()) { + if (batch_deletion_ids_.get<1>().count((int)related_instance.id()) != 0) { continue; } @@ -3021,7 +3021,20 @@ void ifcopenshell::impl::in_memory_file_storage::process_deletion_inverse(const // Delete inverses into entity byref_excl_.erase(id); - byref_excl_.remove_source(id); + + // Delete the records the entity contributed through its own attributes. + // Walking the attributes mirrors build_inverses_, so every record with + // this source is covered without scanning the whole index for it. + const auto* decl = entity.declaration().as_entity(); + if (decl == nullptr) { + return; + } + std::function fn = [this, id, decl](const express::base& attr, int idx) { + if (attr.declaration().as_entity() != nullptr) { + byref_excl_.remove(attr.id(), id, (uint16_t)decl->index_in_schema(), idx); + } + }; + apply_individual_instance_visitor(entity).apply(fn); } namespace { diff --git a/src/ifcparse/storage.h b/src/ifcparse/storage.h index 1ff6b9de9c..2322bb449b 100644 --- a/src/ifcparse/storage.h +++ b/src/ifcparse/storage.h @@ -444,27 +444,6 @@ namespace ifcopenshell { return true; } - // Nothing indexes records by source, so this walks the whole index. - void remove_source(uint32_t source_id) { - sort(); - for (auto& record : base_) { - if (!is_dead(record) && record.source_id == source_id) { - kill(record); - } - } - for (auto it = delta_.begin(); it != delta_.end();) { - auto& records = it->second; - auto removed = std::remove_if(records.begin(), records.end(), [source_id](const inverse_record& record) { - return record.source_id == source_id; - }); - delta_size_ -= (size_t)std::distance(removed, records.end()); - records.erase(removed, records.end()); - it = records.empty() ? delta_.erase(it) : std::next(it); - } - compact_if_tombstones_dominate(); - invalidate_materialized(); - } - // Finalizes bulk loading. Subsequent add() calls go to the delta. void sort() const { if (!sorted_) { diff --git a/src/ifcparse/tests/test_ifcopenshell_parse.cpp b/src/ifcparse/tests/test_ifcopenshell_parse.cpp index 6e90ed270a..af93cc3a1b 100644 --- a/src/ifcparse/tests/test_ifcopenshell_parse.cpp +++ b/src/ifcparse/tests/test_ifcopenshell_parse.cpp @@ -165,3 +165,80 @@ TEST_CASE("Inverse lookups stay consistent across interleaved adds, removals and } CHECK(referencing_ids(target) == std::vector{(int)trimmed.id()}); } + +TEST_CASE("Deleting an instance unregisters the records its own attributes contributed", "[ifcparse]") { + ifcopenshell::file file(ifcopenshell::schema_by_name("IFC4")); + const auto* point_declaration = file.schema()->declaration_by_name("IfcCartesianPoint"); + const auto* polyline_declaration = file.schema()->declaration_by_name("IfcPolyline"); + const auto* trimmed_curve_declaration = file.schema()->declaration_by_name("IfcTrimmedCurve"); + + auto target = file.create(point_declaration); + auto second = file.create(point_declaration); + + // A reference registered before the first lookup lands in the base tier, + // one registered after it in the delta. + auto base_referencer = file.create(polyline_declaration); + base_referencer.set_attribute_value(0, std::vector{target, target}); + REQUIRE(file.instances_by_reference(target.id()).size() == 2); + auto delta_referencer = file.create(polyline_declaration); + delta_referencer.set_attribute_value(0, std::vector{target, second}); + REQUIRE(file.instances_by_reference(target.id()).size() == 3); + + // Duplicate references in one aggregate contribute two records; deleting + // the source must drop both. + file.remove_entity(base_referencer); + CHECK(file.instances_by_reference(target.id()).size() == 1); + + // Referencing the same instance through two attributes contributes a + // record per attribute; deleting the source must drop them all. + auto trimmed = file.create(trimmed_curve_declaration); + trimmed.set_attribute_value(1, std::vector{second}); + trimmed.set_attribute_value(2, std::vector{second}); + CHECK(file.instances_by_reference(second.id()).size() == 3); + file.remove_entity(trimmed); + CHECK(file.instances_by_reference(second.id()).size() == 1); + + // Deleting the target first prunes it out of the source's attribute, so + // deleting the source afterwards finds nothing left to unregister. + file.remove_entity(target); + file.remove_entity(delta_referencer); + CHECK(file.instances_by_reference(second.id()).empty()); + CHECK(file.get_total_inverses(second.id()) == 0); +} + +TEST_CASE("Batch deletion prunes surviving referencers and leaves no stale records", "[ifcparse]") { + ifcopenshell::file file(ifcopenshell::schema_by_name("IFC4")); + const auto* point_declaration = file.schema()->declaration_by_name("IfcCartesianPoint"); + const auto* polyline_declaration = file.schema()->declaration_by_name("IfcPolyline"); + + auto kept_point = file.create(point_declaration); + std::vector doomed_points; + for (int i = 0; i < 50; ++i) { + doomed_points.push_back(file.create(point_declaration)); + } + + // The survivor references every doomed point plus the kept one; a doomed + // referencer references the kept point. + auto survivor = file.create(polyline_declaration); + auto survivor_points = doomed_points; + survivor_points.push_back(kept_point); + survivor.set_attribute_value(0, survivor_points); + auto doomed_referencer = file.create(polyline_declaration); + doomed_referencer.set_attribute_value(0, std::vector{kept_point}); + REQUIRE(file.instances_by_reference(kept_point.id()).size() == 2); + + file.batch(); + for (auto& point : doomed_points) { + file.remove_entity(point); + } + file.remove_entity(doomed_referencer); + file.unbatch(); + + CHECK((std::vector)survivor.get_attribute_value(0) == std::vector{kept_point}); + CHECK(file.instances_by_reference(kept_point.id()).size() == 1); + CHECK(file.get_total_inverses(kept_point.id()) == 1); + for (auto& point : doomed_points) { + CHECK(file.instances_by_reference(point.id()).empty()); + } + CHECK(file.instances_by_reference(doomed_referencer.id()).empty()); +}