Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

serbanPricop
Copy link

  • 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

I'm helping!

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc.)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Related issue(s)

Other Information

serbanPricop and others added 5 commits October 14, 2024 17:45
…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
- 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
- Change `save` method signature to use keyword-only argument
@Dresdn
Copy link
Contributor

Dresdn commented Oct 14, 2024

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:

  1. Optimizing saving an Entity with multiple EAV value updates

Looking at test_setting_attributes(), the p.save() current does 4 database operations, 2 per value: 1 get_all_attributes() and another Attribute.save_value(). This would cut it down to 2 operations, regardless the number of attribute updates for an entity!

  1. Optimizing saving n Entities with y Attributes using Django's bulk manager methods

Being able to bulk_save() Value objects would be great when trying to bulk save Entity objects.

Comment on lines +121 to +123
if not commit:
attributes_value = self.save_bulk(values, attributes)
Value.objects.bulk_create(attributes_value)
Copy link
Contributor

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.

Comment on lines +210 to +214
def save_bulk(
self,
eav_values,
attributes,
):
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants