-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
TrimStart and TrimEnd with optional char array implementation for SQL Server #33715
Conversation
@dotnet-policy-service agree |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
# Conflicts: # src/EFCore.SqlServer/Query/Internal/SqlServerMethodCallTranslatorProvider.cs
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.
Thanks, here are some comments from me.
Note that there's also #22927 for Trim (as opposed to TrimStart/End) - it would be good to add this translation too (though it doesn't have to happen in the same PR).
src/EFCore.SqlServer/Query/Internal/Translators/SqlServerStringMethodTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/Translators/SqlServerStringMethodTranslator.cs
Outdated
Show resolved
Hide resolved
…RTRIM. Handling compatibility level to low." This reverts commit e1a73e7.
…d end in a consistent way.
# Conflicts: # src/EFCore.SqlServer/Query/Internal/SqlServerMethodCallTranslatorProvider.cs
Yes, I saw the Trim, but since it has a different syntax in the new version, it is unlikely that it can be done in the same way. So, @roji, if you agree, I would prefer to address it after finishing these. For some reason, my SQLite tests are failing again. Maybe I'm doing something wrong in the merge or something? |
Commenter does not have sufficient privileges for PR 33715 in repo dotnet/efcore |
src/EFCore.SqlServer/Query/Internal/SqlServerMethodCallTranslatorProvider.cs
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/Translators/SqlServerStringMethodTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/Translators/SqlServerStringMethodTranslator.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
src/EFCore.SqlServer/Query/Internal/Translators/SqlServerStringMethodTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/Translators/SqlServerStringMethodTranslator.cs
Outdated
Show resolved
Hide resolved
…lidated by compatibility level.
Head branch was pushed to by a user without write access
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.
Thanks, this is very close - see note below. I'll just go ahead and fix the remaining issue plus some cleanup.
Thanks for your contribution!
|| (TrimStartMethodInfoWithCharArrayArg.Equals(method) | ||
// SqlServer LTRIM does not take arguments | ||
&& ((arguments[0] as SqlConstantExpression)?.Value as Array)?.Length == 0)) | ||
|| (_sqlServerSingletonOptions.CompatibilityLevel >= 160 |
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.
It's a bit of an annoying little detail, but the previous code did translate TrimStartMethodInfoWithCharArrayArg as long as the first argument is an empty array constant. I don't think it was an important thing to do, but once it's there I don't think we should be removing it (it's a very minor but also needless breaking change). So can we please preserve the previous behavior, and only put the new translation inside the CompatibilityLevel condition?
dotnet#33715 and dotnet#34002 were developed concurrently. Their merge does not build because of some changes in the types returned by `ISqlExpressionFactory`.
Fixes #22924