Fixed #28821 -- Made QuerySet.bulk_create() work for multi-table inheritance when possible.#13763
Fixed #28821 -- Made QuerySet.bulk_create() work for multi-table inheritance when possible.#13763jdufresne wants to merge 1 commit intodjango:masterfrom jdufresne:multi-table-bulk-insert
Conversation
charettes
left a comment
charettes
left a comment
There was a problem hiding this comment.
Thanks for the patch
I think the docs should mention that the current implementation of bulk_create requires one extra query per parent as one could assume that the implementation is smart enough to rely on INSERT INTO child ... FROM SELECT id AS parent_ptr_id FROM (INSERT INTO parent ... RETURNING id) which can be achieved in a single query.
|
Just to make it clear I think we should definitely move along with this patch even if it doesn't support single query MTI A follow up ticket that adds support for single query multi-table |
|
Thanks for taking a look, Simon. I added "An additional SQL query is executed per concrete model in the inheritance hierarchy." to the |
charettes
left a comment
charettes
left a comment
There was a problem hiding this comment.
I left a comment regarding Model.from_db but outside out that it might be worth adding a test for direct multi table inheritance e.g.
class A(models.Model):
pass
class B(models.Model):
pass
class C(A, B):
pass
Thanks! This suggested test exposed a bug. I'm looking into resolving it. |
|
I have implemented the suggested change and included the suggested test. The precise test is slightly different than the suggested one due to avoid conflicts in PR fields which triggers a system check failure: |
django/db/models/query.py
Outdated
There was a problem hiding this comment.
I think we should use local_concrete_fields 🤔 Also, the current solutions doesn't work with recursive parents, e.g.
Pizzeria.objects.bulk_create([
Pizzeria(name='Vegan Pizza House', rank=33),
])crashes when we add the Ranked model:
class Place(models.Model):
name = models.CharField(max_length=100)
class Meta:
abstract = True
class Ranked(models.Model):
rank = models.IntegerField(null=True)
class Restaurant(Ranked, Place):
pass
class Pizzeria(Restaurant):
pass
There was a problem hiding this comment.
Hi, I'm going to make an attempt at finishing this PR, but I'm not super familiar with Django innards. Do you have any suggestions towards resolving this problem?
There was a problem hiding this comment.
@dralley The suggestion is already in the comment: "I think we should use local_concrete_fields", but it's only a suggestion. I didn't check this.
There was a problem hiding this comment.
Oh, I thought it was referring to two separate issues.
Alright then, I'll make a new PR and we can proceed from there
…ritance when possible.
|
I'd be glad to test this (or attempt to test it) on our codebase once it's ready. We have a lot of multi-table inheritance. |
|
@jdufresne Do you plan to pick this work back up? |
|
@dralley I am no longer working on this. Please feel free to pick up the remaining work if you're motivated to do so. I will close this now to avoid any future confusion. |
|
ping |
|
@mbrav I haven't hada lot of time. I do hope to pick it back up eventually. |
5 participants
ticket-28821