Switch order search to using starts over cont #1660

gmacdougall · 2016-12-20T17:20:01Z

Due to the way that standard indexes work in modern databases. Use of
contains to find a string is very inefficient. Instead, if we find
things that start with the string, the index can find the records it is
looking for very efficiently.

This turns a table sequential scan into a simple index lookup
signifncantly speeding things up, or avoiding timeouts on stores with
millions of orders.

Example:

mysql> SELECT COUNT(`spree_orders`.`id`) FROM `spree_orders` WHERE (`spree_orders` .`email` LIKE '%foo@bar\\.com%' AND `spree_orders`.`completed_at` IS NULL);
+----------------------------+
| COUNT(`spree_orders`.`id`) |
+----------------------------+
|                          1 |
+----------------------------+
1 row in set (20.27 sec)

mysql> SELECT COUNT(`spree_orders`.`id`) FROM `spree_orders` WHERE (`spree_orders` .`email` LIKE 'foo@bar\\.com%' AND `spree_orders`.`completed_at` IS NULL);
+----------------------------+
| COUNT(`spree_orders`.`id`) |
+----------------------------+
|                          1 |
+----------------------------+
1 row in set (0.00 sec)

jhawthorn

👍

I think the order number and shipment number might as well be a plain ==, but let's ship this change first and see if this causes anyone any hardships. The only field here I think might be useful to do a "contains" instead of a "start" is email.

Can this get a CHANGELOG entry?

gmacdougall · 2016-12-20T19:16:36Z

Sure thing. I agree that having contains would be better than starts with on e-mail, but the reason that this came up is that we're getting timeouts on our site with large numbers of orders when attempting to search via e-mail.

gmacdougall · 2016-12-20T23:44:39Z

Changelog added in an amendment.

stewart · 2016-12-21T22:47:50Z

I believe the contains is also useful for promotion codes.

cbrunsdon · 2016-12-30T18:49:50Z

I think this is probably worth reaching out to a few places CS departments and do a quick ask to see if they ever use email addresses with "contains" like that. I know people could override the view and switch the query param back, but its potentially a disruptive thing. It would be nice if we could support wildcards ourselves and let people decide when to use them.

jhawthorn · 2017-02-17T21:17:42Z

@cbrunsdon @gmacdougall Can anything be done to move this forward? One of our promises with Solidus is to work well on large stores, which this change is necessary for.

Maybe we should start by changing the less controversial searches, like number and shipment_number to _start

gmacdougall · 2017-02-17T21:46:29Z

I feel that all of the things listed here are large enough that non-indexed searches are inappropriate. It's not that hard to get millions of orders or promotion codes.

bbuchalter · 2017-05-26T17:44:05Z

@gmacdougall can we get this one rebased on master and review this work again?

houndci-bot · 2017-07-02T12:57:52Z

backend/spec/features/admin/orders/listing_spec.rb

+=======
+          click_on 'Filter'
+          fill_in "q_promotions_codes_value_start", with: promotion.codes.first.value
+>>>>>>> e13e004... Switch order search to using starts over cont


unexpected token tRSHFT
unexpected token tIDENTIFIER

houndci-bot · 2017-07-02T12:57:52Z

backend/spec/features/admin/orders/listing_spec.rb

+||||||| parent of e13e004... Switch order search to using starts over cont
+          click_on 'Filter'
+          fill_in "q_promotions_codes_value_cont", with: promotion.codes.first.value
+=======


unexpected token tEQQ

houndci-bot · 2017-07-02T12:57:52Z

backend/spec/features/admin/orders/listing_spec.rb

          click_on "Filter Results"
          fill_in "q_order_promotions_promotion_code_value_cont", with: promotion.codes.first.value
+||||||| parent of e13e004... Switch order search to using starts over cont


unexpected token tOROP
unexpected token tIDENTIFIER

houndci-bot · 2017-07-02T12:57:52Z

backend/spec/features/admin/orders/listing_spec.rb

@@ -160,8 +160,16 @@
        end

        it "only shows the orders with the selected promotion" do
+<<<<<<< HEAD


unexpected token tLSHFT

gmacdougall · 2017-07-02T12:58:02Z

Change rebased and shall rise again!

kennyadsl

I'm ok with this change, it just needs a rebase that fixes CHANGELOG (again) and the conflict error.

Going to request changes to point out that it needs some action, I'll approve once fixed! 👍

kennyadsl · 2017-07-03T10:31:15Z

backend/app/views/spree/admin/orders/index.html.erb

+=======
+                <%= label_tag :q_number_start, Spree.t(:order_number, :number => '') %>
+                <%= f.text_field :number_start %>
+>>>>>>> e13e004... Switch order search to using starts over cont


@gmacdougall I think this has been introduced rebasing and fixing the CHANGELOG conflict.

My bad. I'll fix that up.

gmacdougall · 2017-07-03T17:22:27Z

This is now rebased (properly)!

mamhoff · 2017-10-04T12:13:46Z

Resolved conflict with the current master branch so that the changelog entry is in the right place. I still think this is a worthwhile change.

tvdeyen · 2017-10-04T12:28:42Z

@gmacdougall would you mind to rebase again so we can merge this?

Due to the way that standard indexes work in modern databases. Use of contains to find a string is very inefficient. Instead, if we find things that start with the string, the index can find the records it is looking for very efficiently. This turns a table sequential scan into a simple index lookup signifncantly speeding things up, or avoiding timeouts on stores with millions of orders.

gmacdougall · 2017-10-04T20:33:15Z

Rebased the CHANGELOG out of this. Should be good to go.

mtomov · 2017-10-05T09:29:00Z

Thank you!

jhawthorn approved these changes Dec 20, 2016

View reviewed changes

gmacdougall force-pushed the stop-using-contains-on-order-search branch from 2cb4ac5 to e13e004 Compare December 20, 2016 23:44

stewart mentioned this pull request Dec 21, 2016

Switch order search to using starts over cont TommyJohnWear/solidus#9

Merged

mtomov mentioned this pull request Jun 16, 2017

Fix admin SQL issues with DISTINCT products #2025

Merged

gmacdougall force-pushed the stop-using-contains-on-order-search branch from e13e004 to 227439b Compare July 2, 2017 12:57

gmacdougall added the Code review needed label Jul 2, 2017

houndci-bot reviewed Jul 2, 2017

View reviewed changes

gmacdougall force-pushed the stop-using-contains-on-order-search branch from 227439b to 847bb0a Compare July 2, 2017 12:58

kennyadsl requested changes Jul 3, 2017

View reviewed changes

gmacdougall force-pushed the stop-using-contains-on-order-search branch 2 times, most recently from 51507cc to e6458c8 Compare July 3, 2017 17:21

gmacdougall force-pushed the stop-using-contains-on-order-search branch from e6458c8 to 2979d6f Compare July 4, 2017 20:38

kennyadsl approved these changes Jul 5, 2017

View reviewed changes

tvdeyen assigned gmacdougall Oct 4, 2017

tvdeyen removed the Code review needed label Oct 4, 2017

gmacdougall force-pushed the stop-using-contains-on-order-search branch from 1d9679b to d565006 Compare October 4, 2017 20:32

gmacdougall merged commit 0b9cb14 into solidusio:master Oct 4, 2017

gmacdougall deleted the stop-using-contains-on-order-search branch October 4, 2017 21:25

gmacdougall restored the stop-using-contains-on-order-search branch October 4, 2017 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch order search to using starts over cont #1660

Switch order search to using starts over cont #1660

gmacdougall commented Dec 20, 2016

jhawthorn left a comment

gmacdougall commented Dec 20, 2016 •

edited

Loading

gmacdougall commented Dec 20, 2016

stewart commented Dec 21, 2016

cbrunsdon commented Dec 30, 2016

jhawthorn commented Feb 17, 2017

gmacdougall commented Feb 17, 2017

bbuchalter commented May 26, 2017

houndci-bot Jul 2, 2017

houndci-bot Jul 2, 2017

houndci-bot Jul 2, 2017

houndci-bot Jul 2, 2017

gmacdougall commented Jul 2, 2017

kennyadsl left a comment

kennyadsl Jul 3, 2017

gmacdougall Jul 3, 2017

gmacdougall commented Jul 3, 2017

mamhoff commented Oct 4, 2017

tvdeyen commented Oct 4, 2017

gmacdougall commented Oct 4, 2017

mtomov commented Oct 5, 2017

Switch order search to using starts over cont #1660

Switch order search to using starts over cont #1660

Conversation

gmacdougall commented Dec 20, 2016

jhawthorn left a comment

Choose a reason for hiding this comment

gmacdougall commented Dec 20, 2016 • edited Loading

gmacdougall commented Dec 20, 2016

stewart commented Dec 21, 2016

cbrunsdon commented Dec 30, 2016

jhawthorn commented Feb 17, 2017

gmacdougall commented Feb 17, 2017

bbuchalter commented May 26, 2017

houndci-bot Jul 2, 2017

Choose a reason for hiding this comment

houndci-bot Jul 2, 2017

Choose a reason for hiding this comment

houndci-bot Jul 2, 2017

Choose a reason for hiding this comment

houndci-bot Jul 2, 2017

Choose a reason for hiding this comment

gmacdougall commented Jul 2, 2017

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl Jul 3, 2017

Choose a reason for hiding this comment

gmacdougall Jul 3, 2017

Choose a reason for hiding this comment

gmacdougall commented Jul 3, 2017

mamhoff commented Oct 4, 2017

tvdeyen commented Oct 4, 2017

gmacdougall commented Oct 4, 2017

mtomov commented Oct 5, 2017

gmacdougall commented Dec 20, 2016 •

edited

Loading