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

Support for translating Regex.Replace and Regex.Count #3059

Open
WhatzGames opened this issue Jan 15, 2024 · 7 comments · May be fixed by #3062
Open

Support for translating Regex.Replace and Regex.Count #3059

WhatzGames opened this issue Jan 15, 2024 · 7 comments · May be fixed by #3062
Labels
enhancement New feature or request
Milestone

Comments

@WhatzGames
Copy link
Contributor

Recently I came across a case where the regex_replace function would have been helpful.
Also the regex_count function could have come in handy at that time.

Though it was easy to workaround the missing features, I thought I'd create an issue here.
I have been tinkering on my own fork already, so if there's interest for it I will gladly create a PR for it.

@roji roji added the enhancement New feature or request label Jan 15, 2024
@roji
Copy link
Member

roji commented Jan 15, 2024

Putting in the backlog.

You're certainly welcome to submit a PR - though it may take me a while to get around to reviewing it. If you want to take a stab at it, take a look at the existing support for regex searching in the provider to see how it's done overall.

@WhatzGames
Copy link
Contributor Author

After getting your feedback for #2936 I looked over my PR for this one.
I noticed during some more tinkering in SQL, that regex_count() also returns NULL when NULL ist provided for any of it's arguments.
This means, that there is a discrepancy between the sql return value and the .NET Regex.Count().
regex_count() being int? and Regex.Count() being plain int.

would you say in such a case, that Regex.Count() should not be used and a custom RegexCount(this DbFunctions _, ...) extension instead?
Or is there another preferred viable approach you have in mind?

@roji
Copy link
Member

roji commented Mar 31, 2024

@WhatzGames sorry for taking some time to get back to you.

Yeah, the question of nullability is a tricky one, since .NET and SQL behave very differently around nulls. In many cases, a .NET function throws an exception when given a null argument, where in SQL that causes the corresponding function to return null instead (aka "null propagation" behavior). EF function translations are unfortunately not always fully consistent around how they deal with nulls.

As a general rule, it seems more useful to approximate regular .NET method signatures for EF.Functions we introduce. That is, if a parameter really is optional (and and omitting it by passing in a null) provides a useful return value (which usually would mean it isn't null), then it makes sense for that parameter to be nullable in .NET. However, if the parameter would only be nullable for SQL null propagation, and conceptually you'd expect the .NET function to throw instead, then it's better to not have nullable parameters, since that makes for a worse .NET experience. If we have nullable return values, that forces the user to check for null (in C#) every time they call this function, even when they pass non-null inputs.

If you look at the EF.Functions for SQL Server, you can see that most parameters and return values are non-nullable. Specifically for the DateDiff* functions, there are overloads both for nullable and non-nullable; this is where EF isn't fully consistent, and note that this is only possible with value type parameters (it's not possible to overload on nullability with .NET reference types).

To summarize... If all the above makes sense, I think we should probably go back to #2936 and amend ToDate/ToTimestamp to have non-nullable parameters - is that something you can do please? Similarly, regex_count() accepts and returns NULL as part of the standard SQL NULL propagation behavior - this is a case where in .NET we'd actually accept an exception. As such, I'd expect the EF.Functions method to also return a non-nullable value.

@WhatzGames
Copy link
Contributor Author

@roji no worries, busy times take their toll.

First of all, thanks for the helpful explanation.

Considering your feedback regarding regex_count, I take it that using Regex.Count() is a suitable option then.
The .NET method itself throws when given a null value.
Translating this should have the same effect in the end.
Therefore no extra EF.Function is needed.

@roji
Copy link
Member

roji commented Apr 3, 2024

@WhatzGames yep, that makes sense - and it's always better to just translate a built-in function when possible (I'd definitely not expect us to introduce EF.Functions for such a nullability difference).

@WhatzGames
Copy link
Contributor Author

@roji I hope it's okay, that I ping you.
Just wanted to let you know that the PR is ready for review.

@roji
Copy link
Member

roji commented May 15, 2024

Thanks @WhatzGames - unfortunately I'm very busy in this period, it may take me a while before I can review your PR... I'll do my best to do this for 9.0 though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants