Inner join visible orders #4879

sauloperez · 2020-03-02T15:51:30Z

What? Why?

Improves (but doesn't close yet) #4782.

This replaces the underlying SQL to get the visible orders, which has quite far-reaching consequences as this bit of logic controls which orders a user can see in reports. Instead of using LEFT JOIN we use INNER JOIN. This means we won't load all line_items, variants and products of the DB but only the ones that match the line_items that belong to the specified orders.

This has an immediate effect on the time it takes to get the data from the DB. On staging data, goes from this complex query plan which reports 2.46s of execution time to this other query plan. This time the execution drops to 1.04s with a reduction (very likely but I haven't checked) on RAM usage.

What should we test?

We should test the distributor totals by supplier report but also at least two other reports. I'd say the ones enterprises use the most but I can only guess. My bet is:

Packing
Order cycle management

Release notes

Build reports reducing the pressure on the DB by refactoring a permissions SQL query.
Changelog Category: Changed

sauloperez · 2020-03-02T15:57:39Z

spec/controllers/spree/admin/reports/distributor_totals_by_supplier_spec.rb

@@ -0,0 +1,249 @@
+require 'spec_helper'
+
+describe Spree::Admin::ReportsController, type: :controller do


I tried to mimic the report I generated with sample data in staging. All of it without shortcuts so I managed to control all the data involved, which enabled me to understands all the bits and pieces of the report generation. I don't plan to keep after I finally close the issue. Meanwhile, it provides the lab conditions necessary to iterate quickly.

luisramos0

Nice, I think this is ok and will improve things 👍
Inner joins are generally faster than left joins 👍

I dont think your explanation is correct though:
"This means we won't load all line_items, variants and products of the DB but only the ones that match the line_items that belong to the specified orders."
The only entries that will not be loaded now are orders without line items AND orders with line items without variants AND orders with line items with variants without products. This is why I think this will not create any trouble.

luisramos0 · 2020-03-03T10:03:47Z

app/controllers/spree/admin/reports_controller.rb

+        if create_csv
+          @csv_report = csv_report(header, table)
+          send_data @csv_report, filename: csv_file_name
+        end


from "rapper style" code to "daily language" code ❤️

Note however that my whole point was to introduce an ivar (which I absolutely dislike) so I could test the csv output. I wanted to avoid any test mocks if possible.

ah, I see, much better anyway!

luisramos0 · 2020-03-03T10:06:10Z

lib/open_food_network/orders_and_fulfillments_report.rb

@@ -82,8 +82,7 @@ def scale_factor(product)
    end

    def order_permissions
-      return @order_permissions unless @order_permissions.nil?
-      @order_permissions = ::Permissions::Order.new(@user)
+      @order_permissions ||= ::Permissions::Order.new(@user)


and from "dumb style" to "eloquent style" ❤️

sauloperez · 2020-03-03T18:46:37Z

Yeah, that's what meant with the inner joins, and it is also my hope 😅

I see no reason why a LEFT might be needed and its the root cause of the awful performance.

sauloperez · 2020-03-05T17:46:43Z

This apparently harmless change

     def order_permissions
-      return @order_permissions unless @order_permissions.nil?
-      @order_permissions = ::Permissions::Order.new(@user)
+      @order_permissions ||= ::Permissions::Order.new(@user)
     end

broke spec/features/consumer/shopping/embedded_groups_spec.rb:14 (broken build). It seems totally unrelated but... you know, embedded things... I removed the commit and this is ready for a 2nd approval.

luisramos0 · 2020-03-05T18:16:59Z

have you tried rerunning the build, embedded has a flaky spec.

sauloperez · 2020-03-05T19:03:56Z

have you tried rerunning the build, embedded has a flaky spec.

yes, like 3 times and it still failed. But I've seen that same spec failing in a build from one of your branches so it might as well be just an exceptionally flaky spec 🤷‍♂️ I just don't want to delay this PR any further.

sauloperez · 2020-03-10T08:24:30Z

This would benefit from a second approval but while we wait the problem is still there. So given the reduced scope of this change, I assume there is little risk and move it to test ready.

RachL · 2020-03-10T13:03:01Z

@sigmundpetersen why did you changed it back?

sigmundpetersen · 2020-03-10T13:15:02Z

Yeah sorry, there are 2 PRs connected to the issue and I don't think the other one is Test ready. Maybe we should disconnect this one and test it seperately? And let the other remain in Code Review.

RachL · 2020-03-10T13:23:56Z

ok, thanks :) done!

RachL · 2020-03-10T17:34:16Z

This is looking good so far on staging.

I have checked with several cases the OC reports and the packing report.

I did the other with simple producer profile.

The results were fine.

@sauloperez I didn't test the performance part, because I guess this will be looked at in production, correct?

sauloperez · 2020-03-10T21:53:54Z

@sauloperez I didn't test the performance part, because I guess this will be looked at in production, correct?

Exactly. The affected SQL query is taking less than half the time in "lab" conditions so I'm hoping this is going to have a noticeable and positive impact. We'll see.

mkllnk

I just discovered that I had a comment on this pull request that didn't get submitted somehow. Anyway, I'm reverting it now following #4782.

mkllnk · 2020-03-13T00:17:47Z

app/services/permissions/order.rb

@@ -10,7 +10,7 @@ def initialize(user)
    # Find orders that the user can see
    def visible_orders
      Spree::Order.
-        with_line_items_variants_and_products_outer.
+        with_line_items_variants_and_products.


This means that empty orders won't show any more, right? I'm not sure about the use case but I think it was possible to see empty carts this way.

Are we missing test coverage for some cases there?

I think it was possible to see empty carts this way

LEFT JOINS would allow that but I think we somehow filter them in a later step

sauloperez commented Mar 2, 2020

View reviewed changes

sauloperez mentioned this pull request Mar 2, 2020

Order Cycle Distributor Totals by Supplier results in Snail #4782

Closed

sigmundpetersen assigned sauloperez Mar 2, 2020

luisramos0 approved these changes Mar 3, 2020

View reviewed changes

sigmundpetersen requested a review from Matt-Yorkley March 4, 2020 07:48

sauloperez added 3 commits March 5, 2020 17:45

Add controller tests to cover totals by supplier

461b1b2

Replace LEFT JOIN with INNER JOIN

53a6377

I see no reason why a LEFT might be needed and its the root cause of the awful performance.

Rewrite INNER JOIN in ActiveRecord's DSL

0042ab2

sauloperez force-pushed the inner-join-visible-orders branch from d81cbd4 to 0042ab2 Compare March 5, 2020 17:01

RachL self-assigned this Mar 10, 2020

RachL added the pr-staged-fr staging.coopcircuits.fr label Mar 10, 2020

RachL removed the pr-staged-fr staging.coopcircuits.fr label Mar 10, 2020

sauloperez merged commit 802ac64 into openfoodfoundation:master Mar 10, 2020

sauloperez deleted the inner-join-visible-orders branch March 10, 2020 21:54

mkllnk reviewed Mar 19, 2020

View reviewed changes

mkllnk mentioned this pull request Mar 19, 2020

Revert "Inner join visible orders" #5016

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inner join visible orders #4879

Inner join visible orders #4879

sauloperez commented Mar 2, 2020 •

edited

sauloperez Mar 2, 2020

luisramos0 left a comment

luisramos0 Mar 3, 2020

sauloperez Mar 3, 2020

luisramos0 Mar 3, 2020

luisramos0 Mar 3, 2020

sauloperez commented Mar 3, 2020

sauloperez commented Mar 5, 2020 •

edited

luisramos0 commented Mar 5, 2020

sauloperez commented Mar 5, 2020 •

edited

sauloperez commented Mar 10, 2020 •

edited

RachL commented Mar 10, 2020

sigmundpetersen commented Mar 10, 2020

RachL commented Mar 10, 2020

RachL commented Mar 10, 2020

sauloperez commented Mar 10, 2020

mkllnk left a comment

mkllnk Mar 13, 2020

Matt-Yorkley Mar 19, 2020

sauloperez Mar 19, 2020

		@@ -0,0 +1,249 @@
		require 'spec_helper'

		describe Spree::Admin::ReportsController, type: :controller do

Inner join visible orders #4879

Inner join visible orders #4879

Conversation

sauloperez commented Mar 2, 2020 • edited

What? Why?

What should we test?

Release notes

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez commented Mar 3, 2020

sauloperez commented Mar 5, 2020 • edited

luisramos0 commented Mar 5, 2020

sauloperez commented Mar 5, 2020 • edited

sauloperez commented Mar 10, 2020 • edited

RachL commented Mar 10, 2020

sigmundpetersen commented Mar 10, 2020

RachL commented Mar 10, 2020

RachL commented Mar 10, 2020

sauloperez commented Mar 10, 2020

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sauloperez commented Mar 2, 2020 •

edited

sauloperez commented Mar 5, 2020 •

edited

sauloperez commented Mar 5, 2020 •

edited

sauloperez commented Mar 10, 2020 •

edited