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

Jump to bottom

Fix bugs in table sorting query builder #12154

Merged
merged 2 commits into from Oct 6, 2017
Merged

Fix bugs in table sorting query builder #12154

merged 2 commits into from Oct 6, 2017

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Oct 5, 2017

See tests where I had an input array and it ended sorting it like (only on PHP5.X)

array (
  0 => 'log_visit',
  1 => 
  array (
    'table' => 'log_action',
    'tableAlias' => 'log_action_idaction_name',
    'joinOn' => 'log_link_visit_action.idaction_name = log_action_idaction_name.idaction',
  ),
  2 => 
  array (
    'table' => 'log_action',
    'tableAlias' => 'log_action_visit_exit_idaction_name',
    'joinOn' => 'log_visit.visit_exit_idaction_name = log_action_visit_exit_idaction_name.idaction',
  ),
  3 => 'log_link_visit_action',

which does not work obviously. It took me couple of hours to test lots of different cases and making sure they all still work so I'm hoping for the best. Looks like the simplest solution will work best.

Right now I am running locally some tests and will comment once they pass as well

@tsteur tsteur added Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review labels Oct 5, 2017
@tsteur tsteur added this to the 3.2.0 milestone Oct 5, 2017
@tsteur
Copy link
Member Author

tsteur commented Oct 5, 2017

Local tests pass. Hoping build tests pass as well.

@mattab
Copy link
Member

mattab commented Oct 5, 2017

This looks very complex!

There were 3 failures in the build on PHP 5.6


There were 3 failures:

1) Piwik\Tests\Integration\SegmentTest::test_getSelectQuery_whenJoiningManyCustomTablesItShouldKeepTheOrderAsDefined
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'sql' => ' SELECT log_link_visit_action.custom_dimension_1, log_action.name as url, sum(log_link_visit_action.time_spent) as `13`, sum(case log_visit.visit_total_actions when 1 then 1 when 0 then 1 else 0 end) as `6` FROM log_link_visit_action AS log_link_visit_action LEFT JOIN log_link_visit_action AS log_link_visit_action_foo ON log_link_visit_action.idvisit = log_link_visit_action_foo.idvisit LEFT JOIN log_action AS log_action_foo ON log_link_visit_action_foo.idaction_url = log_action_foo.idaction LEFT JOIN log_link_visit_action AS log_link_visit_action_bar ON log_link_visit_action.idvisit = log_link_visit_action_bar.idvisit LEFT JOIN log_action AS log_action_bar ON log_link_visit_action_bar.idaction_url = log_action_bar.idaction LEFT JOIN log_link_visit_action AS log_link_visit_action_baz ON log_link_visit_action.idvisit = log_link_visit_action_baz.idvisit LEFT JOIN log_action AS log_action_baz ON log_link_visit_action_baz.idaction_url = log_action_baz.idaction LEFT JOIN log_action AS log_action ON log_link_visit_action.idaction_url = log_action.idaction WHERE ( log_link_visit_action.server_time >= ? AND log_link_visit_action.server_time <= ? AND log_link_visit_action.idsite = ? ) AND ( log_action.type = ? )'
+    'sql' => ' SELECT log_link_visit_action.custom_dimension_1, log_action.name as url, sum(log_link_visit_action.time_spent) as `13`, sum(case log_visit.visit_total_actions when 1 then 1 when 0 then 1 else 0 end) as `6` FROM log_link_visit_action AS log_link_visit_action LEFT JOIN log_action AS log_action ON log_link_visit_action.idaction_url = log_action.idaction LEFT JOIN log_link_visit_action AS log_link_visit_action_foo ON log_link_visit_action.idvisit = log_link_visit_action_foo.idvisit LEFT JOIN log_action AS log_action_foo ON log_link_visit_action_foo.idaction_url = log_action_foo.idaction LEFT JOIN log_link_visit_action AS log_link_visit_action_bar ON log_link_visit_action.idvisit = log_link_visit_action_bar.idvisit LEFT JOIN log_action AS log_action_bar ON log_link_visit_action_bar.idaction_url = log_action_bar.idaction LEFT JOIN log_link_visit_action AS log_link_visit_action_baz ON log_link_visit_action.idvisit = log_link_visit_action_baz.idvisit LEFT JOIN log_action AS log_action_baz ON log_link_visit_action_baz.idaction_url = log_action_baz.idaction WHERE ( log_link_visit_action.server_time >= ? AND log_link_visit_action.server_time <= ? AND log_link_visit_action.idsite = ? ) AND ( log_action.type = ? )'
     'bind' => Array (...)
 )

/home/travis/build/piwik/piwik/tests/PHPUnit/Integration/SegmentTest.php:512

2) Piwik\Tests\Integration\SegmentTest::test_getSelectQuery_whenJoinLogLinkVisitActionOnActionOnVisit
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'sql' => ' SELECT log_link_visit_action.custom_dimension_1, actionAlias.name as url, sum(log_link_visit_action.time_spent) as `13`, sum(case visitAlias.visit_total_actions when 1 then 1 when 0 then 1 else 0 end) as `6` FROM log_link_visit_action AS log_link_visit_action LEFT JOIN log_visit AS visitAlias ON visitAlias.idvisit = log_link_visit_action.idvisit LEFT JOIN log_action AS actionAlias ON log_link_visit_action.idaction_url = actionAlias.idaction LEFT JOIN log_action AS log_action ON log_link_visit_action.idaction_url = log_action.idaction WHERE ( log_link_visit_action.server_time >= ? AND log_link_visit_action.server_time <= ? AND log_link_visit_action.idsite = ? ) AND ( log_action.type = ? )'
+    'sql' => ' SELECT log_link_visit_action.custom_dimension_1, actionAlias.name as url, sum(log_link_visit_action.time_spent) as `13`, sum(case visitAlias.visit_total_actions when 1 then 1 when 0 then 1 else 0 end) as `6` FROM log_link_visit_action AS log_link_visit_action LEFT JOIN log_action AS log_action ON log_link_visit_action.idaction_url = log_action.idaction LEFT JOIN log_visit AS visitAlias ON visitAlias.idvisit = log_link_visit_action.idvisit LEFT JOIN log_action AS actionAlias ON log_link_visit_action.idaction_url = actionAlias.idaction WHERE ( log_link_visit_action.server_time >= ? AND log_link_visit_action.server_time <= ? AND log_link_visit_action.idsite = ? ) AND ( log_action.type = ? )'
     'bind' => Array (...)
 )

/home/travis/build/piwik/piwik/tests/PHPUnit/Integration/SegmentTest.php:723

3) Piwik\Tests\Integration\SegmentTest::test_getSelectQuery_whenLimit_withCustomJoinsAndSameColumns
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'sql' => ' SELECT log_inner.name AS 'EntryPageTitle', log_inner.name02fd90a35677a359ea5611a4bc456a6f AS 'EventAction', count(distinct log_inner.idvisit) AS 'nb_uniq_visits', count(distinct log_inner.idvisitor) AS 'nb_uniq_visitors', sum(case log_inner.visit_total_actions when 1 then 1 when 0 then 1 else 0 end) AS 'bounce_count', sum(log_inner.visit_total_actions) AS 'sum_actions', sum(log_inner.visit_goal_converted) AS 'sum_visit_goal_converted' FROM ( SELECT log_action_visit_entry_idaction_name.name, log_action_idaction_event_action.name as name02fd90a35677a359ea5611a4bc456a6f, log_visit.idvisit, log_visit.idvisitor, log_visit.visit_total_actions, log_visit.visit_goal_converted FROM log_visit AS log_visit LEFT JOIN log_action AS log_action_visit_entry_idaction_name ON log_visit.visit_entry_idaction_name = log_action_visit_entry_idaction_name.idaction LEFT JOIN log_link_visit_action AS log_link_visit_action ON log_link_visit_action.idvisit = log_visit.idvisit LEFT JOIN log_action AS log_action_idaction_event_action ON log_link_visit_action.idaction_event_action = log_action_idaction_event_action.idaction ORDER BY nb_uniq_visits, log_action_idaction_event_action.name LIMIT 0, 33 ) AS log_inner GROUP BY log_inner.name, log_inner.name02fd90a35677a359ea5611a4bc456a6f ORDER BY nb_uniq_visits, log_inner.name02fd90a35677a359ea5611a4bc456a6f'
+    'sql' => ' SELECT log_inner.name AS 'EntryPageTitle', log_inner.name02fd90a35677a359ea5611a4bc456a6f AS 'EventAction', count(distinct log_inner.idvisit) AS 'nb_uniq_visits', count(distinct log_inner.idvisitor) AS 'nb_uniq_visitors', sum(case log_inner.visit_total_actions when 1 then 1 when 0 then 1 else 0 end) AS 'bounce_count', sum(log_inner.visit_total_actions) AS 'sum_actions', sum(log_inner.visit_goal_converted) AS 'sum_visit_goal_converted' FROM ( SELECT log_action_visit_entry_idaction_name.name, log_action_idaction_event_action.name as name02fd90a35677a359ea5611a4bc456a6f, log_visit.idvisit, log_visit.idvisitor, log_visit.visit_total_actions, log_visit.visit_goal_converted FROM log_visit AS log_visit LEFT JOIN log_link_visit_action AS log_link_visit_action ON log_link_visit_action.idvisit = log_visit.idvisit LEFT JOIN log_action AS log_action_visit_entry_idaction_name ON log_visit.visit_entry_idaction_name = log_action_visit_entry_idaction_name.idaction LEFT JOIN log_action AS log_action_idaction_event_action ON log_link_visit_action.idaction_event_action = log_action_idaction_event_action.idaction ORDER BY nb_uniq_visits, log_action_idaction_event_action.name LIMIT 0, 33 ) AS log_inner GROUP BY log_inner.name, log_inner.name02fd90a35677a359ea5611a4bc456a6f ORDER BY nb_uniq_visits, log_inner.name02fd90a35677a359ea5611a4bc456a6f'
     'bind' => Array (...)
 )

/home/travis/build/piwik/piwik/tests/PHPUnit/Integration/SegmentTest.php:963

@tsteur
Copy link
Member Author

tsteur commented Oct 5, 2017

Fixed tests and improved them (by making sure the generated queries actually execute and don't break because of an invalid join order etc)

@mattab mattab merged commit 43e14ed into 3.x-dev Oct 6, 2017
@mattab mattab deleted the querybuildertables branch October 6, 2017 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tsteur @mattab