Skip to content

POC add expression support to QuerySet.update() #111

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

Closed
wants to merge 2 commits into from

Conversation

WaVEV
Copy link
Collaborator

@WaVEV WaVEV commented Aug 20, 2024

This is a quick approach to support expression in updates. I have to say that the update pipeline is not expressive as it's needed

@WaVEV WaVEV requested review from Jibola and timgraham August 20, 2024 03:18
if is_empty:
rows = 0
else:
base_pipeline = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This part is the only thing that need focus. Here I just create the $merge pipeline and the affected rows pipeline with a single transaction. I really don't know if this is the way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the right way to go about it. having a transaction ensures that state isn't changed between the lookup and the update.

Copy link
Collaborator Author

@WaVEV WaVEV Aug 20, 2024

Choose a reason for hiding this comment

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

I think we can use update many directly as Shane mentioned. I've re-read the docs and what the docs support are this three stages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeap, It works 🚀. So we are able to do the update without making two queries (one for count and the other for the update)

Copy link
Contributor

@Jibola Jibola left a comment

Choose a reason for hiding this comment

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

Overall, agree with the approach. I'm hoping there is another way to combine the count and merge after the merge has occurred, since we ultimately just want the number of documents that were changed.

if is_empty:
rows = 0
else:
base_pipeline = [
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the right way to go about it. having a transaction ensures that state isn't changed between the lookup and the update.

]
with self.connection.connection.start_session() as session, session.start_transaction():
result = next(self.collection.aggregate(count_pipeline), {"count": 0})
self.collection.aggregate(pipeline)
Copy link
Contributor

Choose a reason for hiding this comment

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

what has been the output when you merge:
[*base_pipeline, {$merge..}, {$count...}] Does this not work whatsoever?

@WaVEV
Copy link
Collaborator Author

WaVEV commented Aug 22, 2024

The final implementation is in #112

@WaVEV WaVEV closed this Aug 22, 2024
@timgraham timgraham changed the title POC support update with expressions. POC add expression support to QuerySet.update() Aug 22, 2024
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