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

Draft: SQL Server support #414

Draft
wants to merge 4 commits into
base: 1.x
Choose a base branch
from
Draft

Conversation

stevenobird
Copy link

@stevenobird stevenobird commented Oct 22, 2024

This one still needs a bit of work to do.

  1. Since SQL Server upsert works with the merge statement in Laravel, we need to rely on a physical key_hash column with pre-generated hashes like in sqlite and cannot go the mysql, mariadb and postgres way by using a virtual column - even if SQL Server supports this by using computed columns. A quick example on that:

    merge [pulse_values] using (values ('key', timestamp, 'type', value)) [laravel_source] ([key], [timestamp], [type], [value])
    on [laravel_source].[type] = [pulse_values].[type] and [laravel_source].[key_hash] = [pulse_values].[key_hash]

    This issue lies in laravel_source (the source table to be compared in the merge statement) not having the key_hash column
    - this makes the join condition [laravel_source].[key_hash] = [pulse_values].[key_hash] invalid.

    We could create computed columns for the key_hash...:

        'sqlsrv' => $table->computed('key_hash', 'cast(hashbytes(\'md5\', [key]) as uniqueidentifier)'),

    ... but doing that, we will get a SQL Error because the merge query will try to insert or update the key_hash column - which is
    not possible, because this one is a non-writable, computed value. That's why I went with creating a string with the length of
    32, which is the md5 string representation.

  2. I am using [laravel_source] as hard coded alias for the upserts, since the current implementation in Laravel also does that. See as seen in SqlGrammar::compileUpsert()


This one still needs a bit of work with some digging being involved. Another issue that I've been facing is the problem of the sqlsrv driver returning values as a string - that results in failing test with messages like...

Failed asserting that '946782245' is identical to 946782245.

and

Failed asserting that '1000' is identical to 1000.

or

Failed asserting that '1729617300' is identical to 1729617300

As you can see, the values match, but the types don't.

The official SQL Server PHP Driver gives us the PDO::SQLSRV_ATTR_FETCHES_NUMERIC_TYPE attribute which should give us numeric casts. I just don't know how to enable them during testing and casting the expected values explicitly to integer feels "wrong" to me.

Calling DB::connection('sqlsrv')->getPdo()->setAttribute(PDO::SQLSRV_ATTR_FETCHES_NUMERIC_TYPE, true) in the beforeEach closure didn't help.

I've also faced a weird error regarding transactions, but that might be because I'm still using msodbcsql17 instead of the current msodbcsql18.

This happened to me in the one or more aggregates for a single type test:

[Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Cannot roll back trans2. No transaction or savepoint of that name was found.

At least it is something in the current state:
image

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

stevenobird and others added 3 commits October 23, 2024 00:00
Since SQL Server uses merge instead of insert, we have to filter for this to satisfy the expectations.
Also, since sqlsrv has one more physical column like sqlite, adjust the expectations here.
This should make all tests pass nonetheless which Storage driver is being used
@stevenobird
Copy link
Author

stevenobird commented Oct 26, 2024

Right now I'm facing an exception that drives me nuts.
This origins from the one or more aggregates for a single type test in DatabaseStorageTest.php:

SQLSTATE[22018]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Conversion failed when converting the nvarchar value '233.33333333333' to data type int.

It seems that the merge statement(?) doesn't like that the float value 233.33333333333 is explicitly cast to a string and provided as that.

When executing the query directly in SMSS, I get the same error:
image

It works in SMSS when I leave out the quotes from the values:
image

When I cast the value explicitly as decimal(20, 2) (like in the migration/table definition), the query also succeeds:
image

At this point, I don't know how we could implement SQL Server support without changing too much in Laravel and Pulse code.
We could cast values to integer, but that could take away some required precision - and even that would require some drawbacks. And unfortunately, there is no PDO::PARAM_FLOAT, PDO::PARAM_DECIMAL or something like that.

It seems to be somehow related to the query execution plan, but I couldn't find anything that proves this point.

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.

1 participant