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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Updated SQL Server to consistently use OFFSET/FETCH #44458

Closed
wants to merge 2 commits into from
Closed

[10.x] Updated SQL Server to consistently use OFFSET/FETCH #44458

wants to merge 2 commits into from

Conversation

dunhamjared
Copy link
Contributor

@dunhamjared dunhamjared commented Oct 4, 2022

Overview

SQL Server on Laravel 8 was updated to use OFFSET/FETCH instead of the row_number() for offset and limit. Woo! 馃コ (#39863)

However, if an orderBy is not included, Laravel will fallback to old code that still uses row_number() -- which adds an unexpected column row_num to the results.

This unexpected column causes the following issues:

  • You cannot use row_num as a column name or alias unless you include a select
  • You cannot insert the offset results into a table unless you unset row_num

This PR updates the row_number() to OFFSET/FETCH, removing the need for the row_num column and increasing query performance. source

Changes

  • Updated row_number() logic to match the current use of OFFSET/FETCH
  • Consolidated the OFFSET/FETCH logic into built in functions

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.

@dunhamjared
Copy link
Contributor Author

dunhamjared commented Oct 4, 2022

Hey @staudenmeir

Looks like we are using OFFSET/FETCH -- but only when orderBy is used.
This may have been overlooked when switching to OFFSET/FETCH.

Let me know if you have any feedback!

@dunhamjared dunhamjared marked this pull request as ready for review October 5, 2022 20:03
@taylorotwell
Copy link
Member

As this has sat here for a while with no reactions and I honestly am not familiar with SQL Server - going to table this for now.

@dunhamjared dunhamjared deleted the master branch November 15, 2022 21:07
@dunhamjared dunhamjared restored the master branch November 15, 2022 21:07
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