Periodic delete #446
Periodic delete #446
Conversation
There was a problem hiding this comment.
Good job, the query module logic is sound and everything else is fine as well. 👍
I’ve only had a few comments about avoiding duplication, and please add a test where you delete nodes with a specific label (or repurpose test_periodic_delete_on_just_nodes for that
There was a problem hiding this comment.
Make batch size-related behavior consistent btwn ValidateBatchSize and ValidateDeletionConfig, and try to extract that into its own function
There was a problem hiding this comment.
Nitpick: I recommend using positional arguments to avoid repeating, e.g. use
fmt::format("MATCH ({0}) WHERE ID({0}) = __{0}_id", node_name)
instead of
fmt::format("MATCH ({}) WHERE ID({}) = __{}_id", node_name, node_name, node_name) (and ditto elsewhere).
There was a problem hiding this comment.
I'm not sure what you mean on the first comment
There was a problem hiding this comment.
About the first comment: ValidateBatchSize and ValidateDeletionConfig have different checks & error messages for the batch_size parameter. I advise to make them consistent, and then to extract that into its own function to avoid repeated code.
|
@antepusic @antoniofilipovic Pls review again! |
There was a problem hiding this comment.
Made the comment in https://github.com/memgraph/mage/pull/446/files#r1521938923 clearer, just make sure to address it
|
Description
Periodic delete as a workaround for large number of deltas during deletion
Pull request type
######################################
Reviewer checklist (the reviewer checks this part)
Module/Algorithm
######################################
@kgolubic "Added query module for periodic delete of graph entities"
memgraph/documentation#501