[#84 ] Add delete_atom method in the InMemoryDB adapter #89
Conversation
| if node: | ||
| atom_type = node['named_type'] | ||
|
|
||
| named_type_exists = any( |
There was a problem hiding this comment.
In order to perform this single check, you are iterating on all nodes. I understand why you did it but you need to be very careful to not introduce this kind of inefficiency in the code as it may pass unnoticed. When it seems we are forced to introduce such type of performance penalty we need to discuss. In this case, for example, it's acceptable just to let go the unused types. The cost of having then is irrelevant and we can always have some type of periodical (or explicitly user-requested) cleanup tasks to clean up this kind of thing out of the AtomDB.
I've create a card to make sure we don't forget this (#90).
So you can remove this check and the code below which actually delete the atom type.
There was a problem hiding this comment.
Yes, I understand and agree with you. We can implement a routine task to cleanup these atom_types in the future. I was aware of the performance and execution on all nodes and all links, but since we are on ramOnly I thought the atoms couldn't be too big. Actually thought I'd talk to you before implementing type removal. I imagine you would mention this point
There was a problem hiding this comment.
OK. Understood. An additional comment, though: ramOnly is the adapter used by most of users in the MeTTa interpreter. It will be used to run the majority of the user's experiments. It's not just a buffer to store temporary info before moving it to a remote DAS server. So we must keep strict focus to make all operations as optimized as we can.
| ) | ||
|
|
||
| if not named_type_exists: | ||
| self._delete_atom_type(atom_type) |
There was a problem hiding this comment.
Remove this (see comments above)
|
|
||
| link_handle = atom['_id'] | ||
|
|
||
| named_type_exists = any( |
There was a problem hiding this comment.
The same comments I made for node type deletion applies here. But here it's even worst as you are iterating on all links (which are usually in larger number). Please delete this check.
| ) | ||
|
|
||
| if not named_type_exists: | ||
| self._delete_atom_type(atom['named_type']) |
There was a problem hiding this comment.
Remove this (see comments above)
| if len(pattern) > 0: | ||
| pattern.remove((link_document['_id'], tuple(targets_hash))) | ||
| if len(pattern) == 0: | ||
| self.db.patterns.pop(pattern_key, None) |
There was a problem hiding this comment.
This is wrong. The pattern key is still searchable even if there's no atom that matches it. We should leave the empty list.
There was a problem hiding this comment.
Sorry, I couldn't understand your point. After removing an item from a pattern_key I do another check and if this list is empty I completely remove pattern_key
There was a problem hiding this comment.
pattern holds a list of (handle, targets) that matched the pattern identified as pattern_key. In line 159 you check if this list became empty after removing the entry related to the link you are deleting (which you did in line 158). If this list is empty, you remove pattern_key out of self.db.patterns in line 160. This is wrong, you should keep the empty list there.
There was a problem hiding this comment.
Why do we need to keep this pattern empty? We talked about this case a few days ago and agreed that if the list is empty the patterns should be deleted
There was a problem hiding this comment.
I'll take back my comment regarding the correctness of the code. It's correct, indeed, as any search for non-existent keys (either here in ramOnly and in redis-mongo) will return an empty list anyway. However it's still unnecessary. Perhaps I misunderstood the context when we discussed this before. Anyway I see no point in worrying about removing empty lists (in this and in all the other similar methods).
If you think we need to remove them, please provide details on why you think we should do it. If it's just to keep the tables clean of empty values (which would return [] if queried from non-existent keys anyway) I vote for just adding another "cleanup" task in the card #90
| template_composite_type = self.db.templates.get(link_document['composite_type_hash'], []) | ||
| if len(template_composite_type) > 0: | ||
| template_composite_type.remove((link_document['_id'], tuple(targets_hash))) | ||
| if len(template_composite_type) == 0: |
There was a problem hiding this comment.
This is wrong. The template key is still searchable even if there's no atom that matches it. We should leave the empty list.
| if len(template_named_type) > 0: | ||
| template_named_type.remove((link_document['_id'], tuple(targets_hash))) | ||
| if len(template_named_type) == 0: | ||
| self.db.templates.pop(link_document['named_type_hash'], None) |
There was a problem hiding this comment.
This is wrong. The template key is still searchable even if there's no atom that matches it. We should leave the empty list.
| if len(handles) > 0: | ||
| handles.remove(link_handle) | ||
| if len(handles) == 0: | ||
| self.db.incoming_set.pop(atom_handle, None) |
There was a problem hiding this comment.
This is wrong. The key is still searchable even if there's no atom pointing to it. We should leave the empty list.
Resolves #84