Bug 23982 - Count/pagination broken for reports with duplicated column names
Summary: Count/pagination broken for reports with duplicated column names
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: additional_work_needed
Depends on: 23624
Blocks:
  Show dependency treegraph
 
Reported: 2019-11-06 20:01 UTC by Nick Clemens
Modified: 2019-11-20 08:50 UTC (History)
4 users (show)

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


Attachments
Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table (2.25 KB, patch)
2019-11-18 10:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table (2.31 KB, patch)
2019-11-19 12:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table (2.37 KB, patch)
2019-11-19 13:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23982: Fix the failing test (904 bytes, patch)
2019-11-20 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-11-06 20:01:52 UTC
bug 23624 simplified how we get the count of rows in a report, however, it breaks under certain circumstances.

A report like:
SELECT * FROM issues JOIN borrowers USING (borrowernumber)

will have two borrowernumber columns - SQL will give us there rsults, but if we try to wrap them in a SELECT COUNT(*) FROM (report) it throws a duplicated column error.

I don't know an easy fix except to state that one must name the columns they want to avoid duplication
Comment 1 Jonathan Druart 2019-11-07 09:42:51 UTC
It seems that we should revert
  commit bca4453c50d8b5b1d3c7029ded2b522a9a2bf868
  Bug 23624: (QA follow-up) Optimize even more


From https://bugs.mysql.com/bug.php?id=6709:
"""
This is in agreement with the SQL Standard, which says:
[...]
11) No <column name> shall be specified more than once 
in a <derived column> list.
"""
Comment 2 Martin Renvoize 2019-11-13 16:48:07 UTC
This is horrible.. I really don't like the idea of reverting that followup as it fixes an important performance hole.

Feels like MySQL is getting it wrong to me.. grr

Contemplating if we have any other options.
Comment 3 Nick Clemens 2019-11-13 18:36:44 UTC
I think if we can make a sensible error back to the user about deduplicating the names we would be okay.

Ideally we would also make basic pagination work, just assumign 10 pages, and again having a friendly error if they page out of bounds
Comment 4 Jonathan Druart 2019-11-18 10:23:50 UTC
Created attachment 95479 [details] [review]
Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table

Caused by
  commit bca4453c50d8b5b1d3c7029ded2b522a9a2bf868
  Bug 23624: (QA follow-up) Optimize even more

A report like:
SELECT * FROM issues JOIN borrowers USING (borrowernumber)

will have two borrowernumber columns - SQL will give us there rsults,
but if we try to wrap them in a SELECT COUNT(*) FROM (report) it throws
a duplicated column error.

This patch suggests to execute the query the old way if the derived
table optimization failed.
Comment 5 Jonathan Druart 2019-11-18 10:24:47 UTC
This impacts stable releases and will cause reports to fail. I think it's a critical one.
Comment 6 Martin Renvoize 2019-11-19 12:47:41 UTC
Created attachment 95545 [details] [review]
Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table

Caused by
  commit bca4453c50d8b5b1d3c7029ded2b522a9a2bf868
  Bug 23624: (QA follow-up) Optimize even more

A report like:
SELECT * FROM issues JOIN borrowers USING (borrowernumber)

will have two borrowernumber columns - SQL will give us there rsults,
but if we try to wrap them in a SELECT COUNT(*) FROM (report) it throws
a duplicated column error.

This patch suggests to execute the query the old way if the derived
table optimization failed.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-11-19 12:48:11 UTC
Thanks for jumping on this one Jonathan, looks reasonable and in a quick test it works for me.. Signing off.
Comment 8 Nick Clemens 2019-11-19 13:18:23 UTC
Created attachment 95548 [details] [review]
Bug 23982: (bug 23624 follow-up) Handle SQL error caused by derived table

Caused by
  commit bca4453c50d8b5b1d3c7029ded2b522a9a2bf868
  Bug 23624: (QA follow-up) Optimize even more

A report like:
SELECT * FROM issues JOIN borrowers USING (borrowernumber)

will have two borrowernumber columns - SQL will give us there rsults,
but if we try to wrap them in a SELECT COUNT(*) FROM (report) it throws
a duplicated column error.

This patch suggests to execute the query the old way if the derived
table optimization failed.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Martin Renvoize 2019-11-19 15:10:47 UTC
Nice work!

Pushed to master for 19.11.00
Comment 10 Jonathan Druart 2019-11-20 08:50:21 UTC
Created attachment 95593 [details] [review]
Bug 23982: Fix the failing test

#   Failed test 'nb_rows returns 0 on bad queries'
    #   at t/db_dependent/Reports/Guided.t line 441.
    #          got: undef
    #     expected: '0'

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>