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

TrimStart and TrimEnd with optional char array implementation for SQL Server #33715

Merged
merged 14 commits into from
Jun 20, 2024

Conversation

abdielcs
Copy link
Contributor

@abdielcs abdielcs commented May 14, 2024

Fixes #22924

  • [X ] I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • [ X] The code builds and tests pass locally (also verified by our automated build checks)
  • [X ] Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • [X ] Tests for the changes have been added (for bug fixes / features)
  • [X ] Code follows the same patterns and style as existing code in this repo

@abdielcs
Copy link
Contributor Author

@dotnet-policy-service agree

@roji
Copy link
Member

roji commented May 17, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

# Conflicts:
#	src/EFCore.SqlServer/Query/Internal/SqlServerMethodCallTranslatorProvider.cs
@cincuranet
Copy link
Contributor

@maumar or @roji can I get 2nd set of eyes. Otherwise LGTM.

@cincuranet cincuranet requested review from maumar and roji May 24, 2024 15:36
Copy link
Member

@roji roji left a 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).

…RTRIM. Handling compatibility level to low."

This reverts commit e1a73e7.
# Conflicts:
#	src/EFCore.SqlServer/Query/Internal/SqlServerMethodCallTranslatorProvider.cs
@abdielcs
Copy link
Contributor Author

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).

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?

Copy link

Commenter does not have sufficient privileges for PR 33715 in repo dotnet/efcore

@abdielcs abdielcs requested a review from roji May 31, 2024 15:38
@cincuranet
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@abdielcs abdielcs requested a review from cincuranet June 14, 2024 17:28
@cincuranet cincuranet enabled auto-merge (squash) June 19, 2024 07:39
auto-merge was automatically disabled June 19, 2024 15:30

Head branch was pushed to by a user without write access

@abdielcs abdielcs requested a review from roji June 19, 2024 16:26
@cincuranet cincuranet self-requested a review June 20, 2024 18:53
Copy link
Member

@roji roji left a 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
Copy link
Member

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?

@roji roji enabled auto-merge (squash) June 20, 2024 20:29
@maumar maumar disabled auto-merge June 20, 2024 20:44
@maumar maumar enabled auto-merge (squash) June 20, 2024 21:07
@maumar maumar merged commit 9e15699 into dotnet:main Jun 20, 2024
7 checks passed
@abdielcs abdielcs deleted the TrimStartSql branch June 20, 2024 23:52
ranma42 added a commit to ranma42/efcore that referenced this pull request Jun 24, 2024
dotnet#33715 and dotnet#34002 were developed concurrently. Their merge does not build because of some changes in the types returned by `ISqlExpressionFactory`.
maumar pushed a commit that referenced this pull request Jun 24, 2024
Fix build regression from #34002

#33715 and #34002 were developed concurrently. Their merge does not build because of some changes in the types returned by `ISqlExpressionFactory`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL Server: Translate string.TrimStart/End overloads accepting characters to trim
5 participants