-
-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(entity): optimize entity save method and add bulk save functionality #663
base: master
Are you sure you want to change the base?
feat(entity): optimize entity save method and add bulk save functionality #663
Conversation
…lity - Add `commit` parameter to `Entity.save()` method - Implement `save_bulk()` method for efficient bulk creation of EAV values - Modify `save()` method to collect values and use `save_bulk()` when commit is True - Improve performance by reducing database queries during save operations
for more information, see https://pre-commit.ci
- Add `commit` parameter to `save` method - Implement `save_bulk` method for preparing bulk EAV Value objects - Modify `save` method to support bulk creation when `commit=False` - Update attribute value handling in `save` method
for more information, see https://pre-commit.ci
- Change `save` method signature to use keyword-only argument
Thanks for the PR @serbanPricop! Looking at the use cases for this, I see two use cases I'll keep in mind when adding some comments on the PR. Please do correct me so I can be sure I align with your thoughts! The use cases for this PR I see are:
Looking at
Being able to |
if not commit: | ||
attributes_value = self.save_bulk(values, attributes) | ||
Value.objects.bulk_create(attributes_value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If commit=False
, I would expect nothing to be saved to the database, which is line with Django behavior. Instead, what do you think about returning a list[Value]
back? This would work for any number of attributes that are updated during an eav.save()
call.
def save_bulk( | ||
self, | ||
eav_values, | ||
attributes, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line with the other comment, should bulk_save()
take a list[Entity]
? Or is the use case for this something different than the two scenarios I identified earlier?
Thinking of how Django's bulks_create()
works, you pass in a list of objects created in memory that then get written with an optimized database query. In this scenario, I would think that when wanting to update a number of entities, you would do something in your app like:
for patient in patients:
patient.eav.age = 10
patient.eav.height = 2.3
eav.bulk_save(patients, 10)
Patient.objects.bulk_create(patients)
Note I did a generic eav.bulk_save()
as I'm not sure this should be on the Entity manager as inserted into a specific Entity instance versus being on a Model Manager. Maybe that's a future thing, having a manager for a model that has EAV methods on it, but for now, I think a utility function would work just fine too.
Also, thinking about it, we should use the same language as Django with "bulk_update" and their params to not have confusion around things like expecting signals to be called like a typical save()
What do you think?
commit
parameter toEntity.save()
methodsave_bulk()
method for efficient bulk creation of EAV valuessave()
method to collect values and usesave_bulk()
when commit is TrueI'm helping!
Checklist
CHANGELOG.md
Pull Request type
Please check the type of change your PR introduces:
Related issue(s)
Other Information