Skip to content

Conversation

@twolfson
Copy link
Contributor

@twolfson twolfson commented Apr 15, 2022

Description

  • Updated section of bulk_create_with_history which doesn't have ids in response from database to consolidate n+1 lookups into 1 lookup
  • Added test to reproduce issue (old version made 7 queries, new makes 3 (INSERT normal, SELECT WHERE for ids, INSERT history))

Related Issue

This closes #974

Motivation and Context

For databases which lack ids in responses, there was a significant amount of additional queries being run. This resolves that issue by consolidating the number of queries

How Has This Been Tested?

Yes, please see changes in the test suite

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run the pre-commit run command to format and lint.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have added my name and/or github handle to AUTHORS.rst
  • I have added my change to CHANGES.rst
  • All new and existing tests passed.

@twolfson
Copy link
Contributor Author

Thanks for running the test build =D Kind of surprised it's failing so will take a look at reproducing by the end of the weekend 👍

@twolfson
Copy link
Contributor Author

I looked into the failing test build, the issue was some assertions which not exactly correct, and have now been fixed =)

@codecov
Copy link

codecov bot commented May 8, 2022

Codecov Report

Merging #975 (93790b7) into master (106266f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #975   +/-   ##
=======================================
  Coverage   97.64%   97.65%           
=======================================
  Files          23       23           
  Lines        1147     1151    +4     
  Branches      222      222           
=======================================
+ Hits         1120     1124    +4     
  Misses         12       12           
  Partials       15       15           
Impacted Files Coverage Δ
simple_history/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 106266f...93790b7. Read the comment docs.

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.

Second transaction in bulk_create_with_history results in n+1 query

2 participants