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.

Sign up for GitHub

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

Jump to bottom

add support for bulk_update #148

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

add support for bulk_update #148

wants to merge 7 commits into from

Conversation

Mng-dev-ai
Copy link
Contributor

No description provided.

aminalaee
aminalaee reviewed Jan 11, 2022
View reviewed changes
Copy link
Member

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I had a look and could see a few points.

orm/models.py Outdated Show resolved Hide resolved
tests/test_columns.py Outdated Show resolved Hide resolved
docs/making_queries.md Outdated Show resolved Hide resolved
docs/making_queries.md Outdated Show resolved Hide resolved
@aminalaee
Copy link
Member

I have to admit I'm not sure how the API should look like. I'll probably take a look at a few examples in other projects. Maybe you can update the PR to show the current signature of the bulk update.

@Mng-dev-ai
Copy link
Contributor Author

https://github.com/collerek/ormar/blob/1ffb28d7b06f3f7989a6219c900e2fd441cbea42/ormar/queryset/queryset.py#L1064 I think that ormar do the same thing here.
Anyway take your time and ping me if you have any updates.

@aminalaee
Copy link
Member

It is also standard bulk update as Django: https://docs.djangoproject.com/en/4.0/ref/models/querysets/#bulk-update

aminalaee
aminalaee requested changes Feb 2, 2022
View reviewed changes
orm/models.py Outdated
]
expr = (
self.table.update()
.where(self.table.c.id == sqlalchemy.bindparam(self.pkname))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to get primary key column dynamically. The id is hard-coded.

aminalaee
aminalaee reviewed Feb 2, 2022
View reviewed changes
orm/models.py Outdated
@@ -20,6 +22,8 @@
"lte": "__le__",
}

MODEL = typing.TypeVar("MODEL", bound="Model")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What value does this bring? I mean we could call bulk_update with Model itself. right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? We use it as a type annotation for obj

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I understand. I mean you've done this:

def bulk_update(self, objects: typing.List[MODEL], ...):
    ....

What would be the difference if we did:

def bulk_update(self, objects: typing.List[Model], ...):
    ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case Model will be undefined because it has been defined after bulk_update

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "Model" ?

aminalaee
aminalaee reviewed Feb 2, 2022
View reviewed changes
orm/models.py
@@ -454,6 +467,43 @@ async def update(self, **kwargs) -> None:

await self.database.execute(expr)

async def bulk_update(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I should've noticed this earlier, apologies for that.
But maybe a general refactor would be useful here?
There's a lot of nested code here and it's not very readable. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree with you it needs to be more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aminalaee Any updates ?

@aminalaee
Copy link
Member

aminalaee commented Feb 22, 2022 •
edited

TBH I think this is really good and useful but as long as the databases project is not doing the bulk actions it's confusing and misleading to have bulk actions here.
But for the record, the databases doesn't do bulk insert/update and will insert these one by one so there won't be any difference.

We can keep it until then.

@Mng-dev-ai
Copy link
Contributor Author

Mng-dev-ai commented Feb 22, 2022 •
edited

So, maybe we need to add it to databases first.

@aminalaee
Copy link
Member

yes, I have an old PR which is doing it but It's not really the best way but it can give you some ideas. Feel free to take a look at that and start your own.

@Mng-dev-ai
Copy link
Contributor Author

Yeah sure, I will take a look at it.

@Mng-dev-ai
Copy link
Contributor Author

Mng-dev-ai commented Feb 22, 2022 •
edited

Check encode/databases#468 (comment)

tarsil added a commit to tarsil/saffier that referenced this pull request Feb 16, 2023
@tarsil
➕ bulk_update, tests and raw_sql
494ee68 
* The bulk_update is an adaptation of
  encode/orm#148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aminalaee aminalaee Awaiting requested review from aminalaee

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Mng-dev-ai @aminalaee