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

SqlServer: string.IsNullOrWhiteSpace translation is overly complex #22916

Closed
bricelam opened this issue Oct 7, 2020 · 5 comments · Fixed by #23035
Closed

SqlServer: string.IsNullOrWhiteSpace translation is overly complex #22916

bricelam opened this issue Oct 7, 2020 · 5 comments · Fixed by #23035
Labels
area-query area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. type-enhancement
Milestone

Comments

@bricelam
Copy link
Contributor

bricelam commented Oct 7, 2020

Today, we translate this as @value IS NULL OR LTRIM(RTRIM(@value)) = N''. But I have a few questions:

  1. Why do we trim both the start and the end? Just one would produce the same result with less processing.
  2. More importantly, why do we trim at all? As we know from our StartsWith research, N' ' = N'' returns true in SQL Server.

IMHO, the translation should be @value IS NULL OR @value = N''

@bricelam
Copy link
Contributor Author

bricelam commented Oct 8, 2020

In a similar vein, IsNullOrEmpty should use LIKE.

@AndriySvyryd AndriySvyryd added this to the Backlog milestone Oct 9, 2020
@AndriySvyryd AndriySvyryd added the good first issue This issue should be relatively straightforward to fix. label Oct 9, 2020
@bradmarder
Copy link

bradmarder commented Oct 12, 2020

@bricelam Possible to save a few bytes with COALESCE(@value, N'')= N''? Or ISNULL if strictly SQL Server

@roji
Copy link
Member

roji commented Oct 12, 2020

@bradmarder IIRC COALESCE prevents index use, so @value IS NULL OR @value = N'' should be preferred. I don't know anything about IS NULL vs. ISNULL through.

@bradmarder
Copy link

@roji You are totally right about COALESCE not using an index, I missed that aspect. I did test ISNULL([@value], N'') = N'' on SQL Server 2017 and it did use an index seek (same exact execution plan as the @value IS NULL OR @value = N''.

@roji
Copy link
Member

roji commented Oct 18, 2020

Split out changing IsNullOrEmpty to #23037.

@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 20, 2020
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-preview1 Jan 27, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview1, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. good first issue This issue should be relatively straightforward to fix. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants