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
Fix ~string.IsNullOrEmpty~ string.Length mapping for SQL CE/MSSQL #4177
base: master
Are you sure you want to change the base?
Conversation
/azp run test-all |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run test-all |
Azure Pipelines failed to run 1 pipeline(s). |
/azp run test-all |
Azure Pipelines successfully started running 1 pipeline(s). |
Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed. |
/azp run test-all |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run test-all |
Azure Pipelines successfully started running 1 pipeline(s). |
I don't get why some DBs think they have right to mess with user data interpretation.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change? Should we be concerned with people who have already written code based on string.Length
and adapted to the differences between c# and their database on how string.Length
is interpreted?
If people rely on old behavior of Length, it could be breaking change for them (MSSQL/SQLCE users only) Still I think it is a bug that should be fixed as:
|
Perhaps wait until v6 then? |
I'm fine with it (I will need this fix at some point, but currently I'm stuck with 4.3.0, so it will not help me anyways if we release it now) |
If waiting till v6, I'm good with it |
I do not like this change at all as we fix very exotic case and break performance of all others. |
/// E.g. see LEN function notes for SQL CE or MSSQL databases. | ||
/// Known issues with empty string: | ||
/// <list type="bullet"> | ||
/// <item>Oracle treats '' as NULL and returns NULL instead of 1</item> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"instead of 0"
/// Known issues with empty string: | ||
/// <list type="bullet"> | ||
/// <item>Oracle treats '' as NULL and returns NULL instead of 1</item> | ||
/// <item>Sybase ASE treats '' as ' ' and returns 1 instead of 1</item> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"instead of 0"
I'm with @igor-tkachev on this one, I don't like this change.
It's surprising that I think the best way to proceed is to keep everything as-is and provide |
Also I would add a method (configuration property) to switch default conversion to this new way. |
Yeah, if it is desirable for SQL Server users to change the default behavior of Did we get a lot of feedback on this? #4138 was opened recently but this behaviour has been in linq2db + Sql Server for years. If there is a global option, I would make it change only the
|
This is the point. I have been using Sql Server for 25 years now and have never experienced this before. |
Fix #4138