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

Fix ~string.IsNullOrEmpty~ string.Length mapping for SQL CE/MSSQL #4177

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MaceWindu
Copy link
Contributor

Fix #4138

@MaceWindu MaceWindu added this to the 5.3.0 milestone Jun 24, 2023
@MaceWindu
Copy link
Contributor Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu
Copy link
Contributor Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@MaceWindu
Copy link
Contributor Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@linq2dbot
Copy link

Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed.

@MaceWindu MaceWindu marked this pull request as ready for review June 24, 2023 20:15
@MaceWindu
Copy link
Contributor Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu MaceWindu changed the title Fix string.IsNullOrEmpty mapping Fix -string.IsNullOrEmpty- string.Length mapping Jun 24, 2023
@MaceWindu MaceWindu changed the title Fix -string.IsNullOrEmpty- string.Length mapping Fix ~string.IsNullOrEmpty~ string.Length mapping for SQL CE/MSSQL Jun 24, 2023
@MaceWindu
Copy link
Contributor Author

/azp run test-all

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MaceWindu
Copy link
Contributor Author

I don't get why some DBs think they have right to mess with user data interpretation....

Copy link
Contributor

@viceroypenguin viceroypenguin left a 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?

@MaceWindu
Copy link
Contributor Author

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:

  • for .net mappings we try to emulate .net behavior when possible
  • for db-behavior users should use explicit mappings for database functions

@viceroypenguin
Copy link
Contributor

Perhaps wait until v6 then?

@MaceWindu
Copy link
Contributor Author

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)

@MaceWindu MaceWindu modified the milestones: 5.3.0, 6.0.0 Jun 26, 2023
@viceroypenguin
Copy link
Contributor

If waiting till v6, I'm good with it

@igor-tkachev
Copy link
Member

igor-tkachev commented Jul 14, 2023

I do not like this change at all as we fix very exotic case and break performance of all others.
Lets keep previous behavior by default and add some settings to switch it to this implementation.

/// 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>
Copy link
Contributor

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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"instead of 0"

@jods4
Copy link
Contributor

jods4 commented Jul 19, 2023

I'm with @igor-tkachev on this one, I don't like this change.

  1. This is a massive breaking change for people who have fixed-length string columns in their DB such as char(50). This is common in legacy schemas and is the motivation why LEN would not count trailing spaces.

  2. I see linq2db as a thin predictable mapper over SQL, even if in some specific cases the behaviour of SQL and C# differ.
    These differences cannot all be efficiently erased, users have to be conscious of them and SQL semantics. Examples include: three-valued logic, null comparisons, empty string is null in Oracle, different datatypes range (e.g. DateTime), etc.
    I don't like creating complex, inefficient expression for minor edge cases, and depriving users from an easy access to straightforward SQL code.

It's surprising that "abc".Length does not convert to LEN('abc').

I think the best way to proceed is to keep everything as-is and provide FullLength() as a SQL extension for SQL Server provider. If SQL Server users don't want the native behaviour, they can use this new function instead.

@igor-tkachev
Copy link
Member

Also I would add a method (configuration property) to switch default conversion to this new way.

@jods4
Copy link
Contributor

jods4 commented Jul 19, 2023

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 "abc".Length to the untrimmed way, I would make it an opt-in option in SQL Server provider-specific options, such as SqlServerOptions.TrimmedLength = false.

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 "".Length behaviour, so that regardless of your option you still have easy access to the other SQL code gen:

  • "".Length behavior is configurable (MSSQL provider-only) as you prefer (trimmed or non-trimmed);
  • Sql.Length() always predictably converts into LEN(..) so it's an escape hatch when TrimmedLength = false;
  • SqlServerExtensions.UntrimmedLength() always converts into an expression that doesn't trim, so it's an escape hatch when TrimmedLength = true.

@igor-tkachev
Copy link
Member

Did we get a lot of feedback on this? #4138 was opened recently but this behaviour has been in linq2db + Sql Server for years.

This is the point. I have been using Sql Server for 25 years now and have never experienced this before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants