Bug 21622 - Incorrect GROUP BY clause in acqui/ scripts
Summary: Incorrect GROUP BY clause in acqui/ scripts
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 21718 22260 (view as bug list)
Depends on:
Blocks: 17258
  Show dependency treegraph
 
Reported: 2018-10-19 15:18 UTC by Jonathan Druart
Modified: 2019-04-30 12:28 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21622: Remove incorrect GROUP BY clauses in acqui script (1.45 KB, patch)
2018-10-19 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21622: Remove incorrect GROUP BY clauses in acqui script (1.51 KB, patch)
2018-10-29 22:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script (1.45 KB, patch)
2018-10-31 13:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script (2.15 KB, patch)
2018-10-31 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script (2.22 KB, patch)
2019-01-16 22:20 UTC, Jasmine Amohia
Details | Diff | Splinter Review
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script (2.27 KB, patch)
2019-02-03 10:35 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21622: (RM follow-up) adjust for changes from bug 21282 (1.21 KB, patch)
2019-03-25 12:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21622: (RM follow-up) adjust for changes from bug 21282 (1.50 KB, patch)
2019-03-25 13:05 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-10-19 15:18:14 UTC
acqui/ordered.pl:    GROUP BY aqorders.ordernumber                                                                                                        
acqui/spent.pl:    GROUP BY aqorders.ordernumber  

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY
Comment 1 Jonathan Druart 2018-10-19 15:20:32 UTC
Created attachment 80943 [details] [review]
Bug 21622: Remove incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch
Comment 2 Jonathan Druart 2018-10-29 21:34:58 UTC
*** Bug 21718 has been marked as a duplicate of this bug. ***
Comment 3 Katrin Fischer 2018-10-29 22:09:14 UTC
Created attachment 81596 [details] [review]
Bug 21622: Remove incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Julian Maurice 2018-10-30 16:30:27 UTC
The SQL queries have a JOIN clause with aqorders_items. Without the GROUP BY it can return multiple rows for the same order. It shouldn't be removed.

On master, ordered.pl displays a table of 15 rows
With the patch applied, it displays a table of 22 rows
Same thing on spent.pl with different numbers.

Failed QA
Comment 5 Katrin Fischer 2018-10-30 17:55:16 UTC
Thx Julian! It didn't happen on mine, probably not enough data!
Comment 6 Jonathan Druart 2018-10-31 13:38:29 UTC
Created attachment 81724 [details] [review]
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch
Comment 7 Jonathan Druart 2018-10-31 13:56:09 UTC
Created attachment 81725 [details] [review]
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch
Comment 8 Jasmine Amohia 2019-01-16 22:20:47 UTC
Created attachment 84101 [details] [review]
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch

Signed-off-by: Jasmine Amohia <jasmineamohia.student@wegc.school.nz>
Comment 9 Katrin Fischer 2019-02-03 10:35:20 UTC
Created attachment 84679 [details] [review]
Bug 21622: Adjust incorrect GROUP BY clauses in acqui script

acqui/ordered.pl:    GROUP BY aqorders.ordernumber
acqui/spent.pl:    GROUP BY aqorders.ordernumbe

'koha_kohadev.aqorders.biblionumber' isn't in GROUP BY

Test plan:
- Create orders for different basket and using different funds
- Receive some of them
- Hit the ordered and spent pages (from the acqui home page)
=> The tables must contain the same data with and without this patch

Signed-off-by: Jasmine Amohia <jasmineamohia.student@wegc.school.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Nick Clemens 2019-02-04 13:10:24 UTC
I missed this one and filed 22260

I am wondering if one situation is preferable to the other (subquery vs adding all fields to group by)

Bug 20182 and 21723 imply there may be some issues here - can we get some opinions here on the best way to fix these situations?
Comment 11 Jonathan Druart 2019-02-17 14:45:15 UTC
(In reply to Nick Clemens from comment #10)
> I missed this one and filed 22260
> 
> I am wondering if one situation is preferable to the other (subquery vs
> adding all fields to group by)
> 
> Bug 20182 and 21723 imply there may be some issues here - can we get some
> opinions here on the best way to fix these situations?

Could you benchmark the 2 queries on a big production DB?
Comment 12 Nick Clemens 2019-02-21 16:40:39 UTC
*** Bug 22260 has been marked as a duplicate of this bug. ***
Comment 13 Nick Clemens 2019-02-21 16:45:11 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Nick Clemens from comment #10)
> > I missed this one and filed 22260
> > 
> > I am wondering if one situation is preferable to the other (subquery vs
> > adding all fields to group by)
> > 
> > Bug 20182 and 21723 imply there may be some issues here - can we get some
> > opinions here on the best way to fix these situations?
> 
> Could you benchmark the 2 queries on a big production DB?

~.1 seconds for this solution vs ~4 seconds for the other

Deprecated in favor of this one, we should invesitgate 21723 on its own I think
Comment 14 Nick Clemens 2019-02-22 15:05:57 UTC
Awesome work all!

Pushed to master for 19.05
Comment 15 Martin Renvoize 2019-02-26 09:22:42 UTC
Pushed to 18.11.x for 18.11.04
Comment 16 Lucas Gass 2019-03-06 23:43:45 UTC
this one fails qa tool in 18.05.x AND i cannot successfully run through the test plan. not backporting to 18.05.x. let me know if it is needed
Comment 17 Nick Clemens 2019-03-25 12:59:37 UTC
Created attachment 86973 [details] [review]
Bug 21622: (RM follow-up) adjust for changes from bug 21282
Comment 18 Nick Clemens 2019-03-25 13:05:28 UTC
Created attachment 86974 [details] [review]
Bug 21622: (RM follow-up) adjust for changes from bug 21282
Comment 19 Nick Clemens 2019-03-29 11:06:55 UTC
(In reply to Nick Clemens from comment #18)
> Created attachment 86974 [details] [review] [review]
> Bug 21622: (RM follow-up) adjust for changes from bug 21282

Followup pushed to master
Comment 20 Martin Renvoize 2019-04-01 10:41:06 UTC
Followup pushed to 18.11.x series.