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

On mysql, compare CONCAT'd values rather than tuple of values #14

Closed
wants to merge 1 commit into from

Conversation

joealcorn
Copy link
Contributor

When ordering by two or more fields as well as supplying a cursor argument to the CurorPaginator.page function, the ORM will generate a query something along the lines of this

SELECT `project`.`id`,
       `project`.`name`,
       `project`.`date_created`,
       (`project`.`date_created`, `project`.`id`) AS `_cursor`
FROM `project`
WHERE ((`project`.`date_created`, `project`.`id`) > (2017-02-16 17:41:42+00:00, 1739438))
ORDER BY `project`.`date_created` DESC,
         `project`.`id` DESC

On mysql this breaks at the AS '_cursor' part - OperationalError: (1241, 'Operand should contain 1 column(s)').

It seems mysql doesn't allow you to select a tuple. To get around this, I'm concatenating the values instead.

With this change, the query generated looks more like this (as_mysql logic was taken from here)

SELECT `project`.`id`,
       `project`.`name`,
       `project`.`date_created`,
       CONCAT_WS('', `project`.`date_created`, `project`.`id`) AS `_cursor`
FROM `project`
WHERE (CONCAT_WS('', `project`.`date_created`, `project`.`id`) < (CONCAT_WS('', 2017-02-16 17:41:42+00:00, 1739438)))
ORDER BY `project`.`date_created` DESC,
         `project`.`id` DESC

From my limited testing, this does seem to work (I'm using django 1.9.13), but I'm not 100% certain of the performance impact or potential comparison differences of using concat rather than comparing tuples.

@mjtamlyn
Copy link
Contributor

mjtamlyn commented Nov 1, 2017

That sounds very fragile to me, not all types will sort in the correct way when they are concatenated as strings. (e.g. 20 < 3).

If the tuple approach doesn't work on myself, then the project would need a lot more work to manually reconstruct the cursor. Personally I'd rather document this as a limitation of the project.

@joealcorn
Copy link
Contributor Author

Fair enough, you are right it is fragile, I've run into an edge case already (and expecting more). I may have to put some work in to get it creating proper where queries on mysql, if I do I'll see about contributing those changes upstream. In the mean time I'll close this PR. :)

@joealcorn joealcorn closed this Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants