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

[10.x] Update SQL Server to FETCH and OFFSET for queries that do not include an order by #44937

Merged
merged 2 commits into from Nov 15, 2022

Conversation

dunhamjared
Copy link
Contributor

@dunhamjared dunhamjared commented Nov 14, 2022

Overview

SQL Server 2012 introduced the fetch and offset feature.

Laravel started using FETCH and OFFSET in 2021 -- but only when you include an order by.
See PR: #39863

Laravel also only officially supports 2017 and up:
https://laravel.com/docs/9.x/database#introduction

This PR updates the query builder to also use FETCH and OFFSET for queries that do not include an order by.

Others have voiced support for this PR:
https://www.reddit.com/r/laravel/comments/ypooqe

This update improves the speed by 33% and requires less execution steps!
https://gist.github.com/dunhamjared/cb40bbf294ed6f8e48bd60010a31b4f3

Changes

  • Updated row_number() logic to match the current use of OFFSET/FETCH
  • Consolidated the OFFSET/FETCH logic into built in functions
  • Ordered the $selectComponents in the correct order for SQL Server

Example

Currently, if you include an order by, it uses offset and fetch:

$builder->select('*')->from('users')->skip(11)->take(10)->orderBy('email', 'desc');
select * from [users] order by [email] desc offset 11 rows fetch next 10 rows only

However, if order by is not included, it falls back to the old method of offsetting the results:

$builder->select('*')->from('users')->skip(10)->take(10);
select * from (select *, row_number() over (order by (select 0)) as row_num from [users]) as temp_table where row_num between 11 and 20 order by row_num

This PR would instead produce:

select * from [users] order by (SELECT 0) offset 10 rows fetch next 10 rows only

Edge cases

  • There could be people that actually use the row_num in their applications when order by is not used, so this could be considered a breaking change.

@joelharkes
Copy link
Contributor

@dunhamjared this introduced a big bug!

Page 1 has no offset and thus is not explicitly sorted by column 0 and thus has another sort order, this must be fixed!!

@dunhamjared
Copy link
Contributor Author

dunhamjared commented Jul 17, 2023

Hi @joelharkes -- that sounds very odd! I haven't experienced that issue myself, also tried quickly replicating the issue you described with no success.

Could you create an issue for this and provide a query example?

@joelharkes
Copy link
Contributor

@dunhamjared
The problem arrives when you don't use incrementing ID's

We use guids as primary key and generating the guids in laravel (creates alphabetically data time sorted guis).

But SQL server orders differently.

This means the inserted record might be on the last 'page' in the databse but when you sort on ID it is actually not the last record in the the list.

see also: https://stackoverflow.com/questions/7810602/sql-server-guid-sort-algorithm-why

@joelharkes
Copy link
Contributor

I will make a PR to fix this issue ASAP.

It should also sort on Select 0 when there is a limit set to avoid this issue.

@joelharkes
Copy link
Contributor

Opened PR to fix it: #47763

@dunhamjared
Copy link
Contributor Author

Nice, glad to see it was a simple fix. :)

@joelharkes
Copy link
Contributor

joelharkes commented Jul 17, 2023

@dunhamjared i broke distincts + limits but i can't get it to work, see: #47766

could you perhaps check what im missing?

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

3 participants