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

Optimize multibuy database queries #469

Draft
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

patrickreiffenstein
Copy link
Contributor

@patrickreiffenstein patrickreiffenstein commented Jun 12, 2024

Fixes #357.

@krestenlaust krestenlaust changed the title Duplicate query removal when buying Optimize multibuy database queries Jun 13, 2024
Copy link
Member

@krestenlaust krestenlaust left a comment

Choose a reason for hiding this comment

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

I'll have to run it locally to check things over, and verify that we have tests in place

@@ -128,15 +128,15 @@ def execute(self):
self.member = Member.objects.select_for_update().get(id=self.member.id)
self.member.fulfill(transaction)

# Collect all the sales of the order
Sales = []
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Sales = []
sales = []

for i in range(item.count):
s = Sale(member=self.member, product=item.product, room=self.room, price=item.product.price)
s.save()
Sales.append(s)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Sales.append(s)
sales.append(s)

@krestenlaust krestenlaust self-requested a review June 13, 2024 07:51
@krestenlaust
Copy link
Member

krestenlaust commented Aug 5, 2024

I'll try to review this before summer ends. Probably after the current update has been finished

@JakobTopholt
Copy link

@Mast3rwaf1z Vil du ikke være en king og review den her så Kresten kan få lidt fritid?

@krestenlaust
Copy link
Member

♥️ jeg vil dog stadig gerne merge den

@Mast3rwaf1z
Copy link
Member

@Mast3rwaf1z Vil du ikke være en king og review den her så Kresten kan få lidt fritid?

Tager lige et kig senere, var lige til forelæsning

Copy link
Member

@Mast3rwaf1z Mast3rwaf1z left a comment

Choose a reason for hiding this comment

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

Some small things, i think improves readability of the code, and removed some redundant constructor calls?

also i believe by using _ in a for loop prevents memory being allocated to the variable. really insignificant but hey, its good to have imo

s.save()
Sales.append(s)
# Save all the sales
Sale.objects.bulk_create(Sales)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Sale.objects.bulk_create(Sales)
Sale.objects.bulk_create(sales)

Comment on lines +719 to +722
if str(unique_id) not in unique_product_dict:
unique_product_dict[str(unique_id)] = 1
else:
unique_product_dict[str(unique_id)] += 1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if str(unique_id) not in unique_product_dict:
unique_product_dict[str(unique_id)] = 1
else:
unique_product_dict[str(unique_id)] += 1
if unique_id not in unique_product_dict:
unique_product_dict[unique_id] = 1
else:
unique_product_dict[unique_id] += 1

what's the point in converting from an int object to string all the time?

Copy link
Member

Choose a reason for hiding this comment

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

you can also optimize it further by just using dict comprehension:

unique_product_dict = {i:bought_ids.count(i) for i in bought_ids}

Comment on lines +725 to 730
for i in unique_product_dict:
product = Product.objects.get(
Q(pk=i),
Q(active=True),
Q(deactivate_date__gte=time_now) | Q(deactivate_date__isnull=True),
Q(rooms__id=room.id) | Q(rooms=None),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for i in unique_product_dict:
product = Product.objects.get(
Q(pk=i),
Q(active=True),
Q(deactivate_date__gte=time_now) | Q(deactivate_date__isnull=True),
Q(rooms__id=room.id) | Q(rooms=None),
for key, value in unique_product_dict.items():
product = Product.objects.get(
Q(pk=key),
Q(active=True),
Q(deactivate_date__gte=time_now) | Q(deactivate_date__isnull=True),
Q(rooms__id=room.id) | Q(rooms=None),

product = Product.objects.get(
Q(pk=i),
Q(active=True),
Q(deactivate_date__gte=time_now) | Q(deactivate_date__isnull=True),
Q(rooms__id=room.id) | Q(rooms=None),
)
products.append(product)
products.extend([product for x in range(unique_product_dict[str(i)])])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
products.extend([product for x in range(unique_product_dict[str(i)])])
products.extend([product for _ in range(value)])

@krestenlaust krestenlaust marked this pull request as draft September 24, 2024 21:50
@krestenlaust krestenlaust removed their request for review September 24, 2024 21:50
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.

Poor performance on high amount of products bought (even with multibuy)
4 participants