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.

Sign up for GitHub

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

Jump to bottom

Fix for #33702 Translation for the SQL Server PATINDEX function #33868

Merged
merged 14 commits into from
Jun 14, 2024
Merged

Fix for #33702 Translation for the SQL Server PATINDEX function #33868

merged 14 commits into from
Jun 14, 2024

Conversation

smnsht
Copy link
Contributor

@smnsht smnsht commented Jun 1, 2024

Fix for #33702 Translation for the SQL Server PATINDEX function

  • [*] 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
  • [*] The code builds and tests pass locally (also verified by our automated build checks)
  • [*] Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2
  • [*] Tests for the changes have been added (for bug fixes / features)
  • [*] Code follows the same patterns and style as existing code in this repo

@smnsht smnsht marked this pull request as ready for review June 1, 2024 15:21
roji
roji requested changes Jun 2, 2024
View reviewed changes
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated 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
@smnsht
Copy link
Contributor Author

smnsht commented Jun 2, 2024

I will try to fix this as soon as possible, but it likely won't happen before Friday.

smnsht added 2 commits June 7, 2024 11:42
@smnsht
* Fixed argumentsPropagateNullability
2ac63d2 
* The return value of EF.Functions method shouldn't be nullable
* Method will need to always return long.
* Moved out of the "full text" region
* Removed overload with collation parameter
* Removed check for SqlParameterExpression || SqlConstantExpression
* Removed check for ColumnExpression
@smnsht
remove unnecessary using
871d802
roji
roji requested changes Jun 7, 2024
View reviewed changes
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs Outdated Show resolved Hide resolved
src/EFCore.SqlServer/Extensions/SqlServerDbFunctionsExtensions.cs Outdated
@@ -2137,6 +2137,23 @@ public static class SqlServerDbFunctionsExtensions
string timeZone)
=> throw new InvalidOperationException(CoreStrings.FunctionOnClient(nameof(AtTimeZone)));

/// <summary>
/// A DbFunction method stub that can be used in LINQ queries to target the SQL Server <c>PATINDEX</c> store function.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please align these doc comments to what's on the other functions? The summary simply summarizes what the function actually does (no mention of "DbFunction method stub" - just a copy-paste from the function docs:

Returns the starting position of the first occurrence of a pattern in a specified expression, or zero if the pattern is not found, on all valid text and character data types.

PATINDEX should be referenced in a <seealso> element, as elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roji
fixed

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
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs Outdated Show resolved Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs Outdated Show resolved Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs Outdated Show resolved Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs Outdated Show resolved Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs Outdated Show resolved Hide resolved
roji
roji approved these changes Jun 13, 2024
View reviewed changes
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.

@smnsht thanks, LGTM!

@smnsht
Copy link
Contributor Author

smnsht commented Jun 13, 2024

@roji
thanks for your patience

@cincuranet cincuranet merged commit 17b1deb into dotnet:main Jun 14, 2024
7 checks passed
@cincuranet
Copy link
Contributor

@smnsht Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cincuranet cincuranet cincuranet left review comments

@ranma42 ranma42 ranma42 left review comments

@roji roji roji approved these changes

@maumar maumar maumar approved these changes

Assignees

@roji roji

Labels
community-contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smnsht @cincuranet @ranma42 @maumar @roji