[10.x] Possible fix for unexpected column in SQL Server offset

[dunhamjared]

Looking for feedback on this PR for resolving this issue:

Currently, if offset is used for SQL Server:

1. You cannot use row_num as a column name or alias
2. An unexpected column row_num is included in the results

This PR helps reduce query select conflicts and removes unexpected data generated by laravel.

Changes:

• Renamed temporary column row_num to temp_row_num to help reduce conflicts
• Removes the temp_row_num column from the results if offset used

Fixes #44354

Examples

Scenario 1

If you have a row_num column or alias, SQL Server will throw:

SQLSTATE[42000]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The column 'row_num' was specified multiple times for 'temp_table'

Scenario 2

If you try to insert the offset results into the same table by using chunk:

db()->connection('db_1')->table('example')->chunk(500, function($chunk){
    db()->connection('db_2')->table('example')->insert($chunk);
});

row_num is added to the results and is an unexpected column, causing SQL server to throw:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'row_num' in 'field list'

Original PR: #44355

[dunhamjared]

Hi @staudenmeir, @yansern and @garygreen -- do you both have any input on this issue and solution?

Or should we instead use OFFSET and FETCH instead of the current row_number() implementation?
This should remove the need for a row_num column as well.

From the community:

• SQL Server issue when ordering by count column with pagination (workaround included). ideas#1006
• 'chunk loop' loop is adding a 'row_num' property #20814 - Same issue given in example "Scenario 2".
• Problem when used with chunk method staudenmeir/laravel-cte#35 - soultion it so only select columns from the table the person is trying to query so row_num will not be returned
• [Proposal] SqlServer2012 grammar file #7568 -

[staudenmeir]

HI @dunhamjared,

Renamed temporary column row_num to temp_row_num to help reduce conflicts
Removes the temp_row_num column from the results if offset used

Renaming the column definitely makes sense. Not sure if we do it consistently, but we use laravel_ as a prefix for internal columns in other places.

I would say that this is a breaking change and needs to go into the next major release. There could be people that actually use the row_num in their applications. Definitely an edge case, but we can't rule it out completely.

Or should we instead use OFFSET and FETCH instead of the current row_number() implementation?

What's the minimum required SQL Server version for that?

[dunhamjared]

@staudenmeir

I would say that this is a breaking change and needs to go into the next major release. There could be people that actually use the row_num in their applications. Definitely an edge case, but we can't rule it out completely.

Completely agree, I will create a new PR to master.

What's the minimum required SQL Server version for that?

2012 (source), below our minimum support version. Woo!

Renaming the column definitely makes sense. Not sure if we do it consistently, but we use laravel_ as a prefix for internal columns in other places.

Good to know, thanks!

It also seems that OFFSET/FETCH is generally faster and less expensive than row_number() - source

[staudenmeir]

2012 (source), below our minimum support version. Woo!

That's good. Laravel only supports 2017+, but I’m sure that quite a few people use Laravel with older versions (since almost all query features still work there). So Laravel is technically “allowed” to rely on 2017+ features, but it still needs to be considered that people aren't happy when pagination no longer works on their SQL Server 2008/2012 app (even though it happened because of a major update).

[dunhamjared]

Working on new PR for master branch.