-
Notifications
You must be signed in to change notification settings - Fork 453
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
Add Sql.Regexp
#4392
base: master
Are you sure you want to change the base?
Add Sql.Regexp
#4392
Conversation
in #1077 you wrote I should create an extension lib. I could do this, but could I also add this expression https://github.com/linq2db/linq2db/pull/4392/files#diff-2a6a9163861fbdc4015c6f32a3373c85469cb401552aea366b2d2c07ede63fb5R1198 I would like to have the support for Regex in Query. |
Source/LinqToDB/Sql/Sql.Strings.cs
Outdated
[Expression(PN.PostgreSQL, "{0} ~ {1}", ServerSideOnly = true, IsPredicate = true)] | ||
[Expression(PN.Oracle, "REGEXP_LIKE({0}, {1})", ServerSideOnly = true, IsPredicate = true)] | ||
[Expression(PN.SapHana, "{0} REGEXP {1}", ServerSideOnly = true, IsPredicate = true)] | ||
[Expression(PN.SqlServer, "dbo.regex({0}, {1})", ServerSideOnly = true, IsPredicate = true)] // supported via https://github.com/infiniteloopltd/SQLServerRegex |
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.
I would remove this line; sql server should throw an exception, and if consumers want to use regex, they can add the library specifically. Having this will create a false expectation for the consumer, if they look at the failed query and see dbo.regex()
in the querystring, and wonder why they don't have the regex
method.
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.
I tend to agree, I wouldn't commit core linq2db functions to custom libraries.
What if MS adds Regex support in the future (quite possible) and it's not exactly the same dialect / not 100% compatible with this lib?
If that lib is popular enough to be included in main linq2db lib, I'd put it on a specific static class: InfiniteLoop.Regex(..)
.
Source/LinqToDB/Sql/Sql.Strings.cs
Outdated
[Expression(PN.Oracle, "REGEXP_LIKE({0}, {1})", ServerSideOnly = true, IsPredicate = true)] | ||
[Expression(PN.SapHana, "{0} REGEXP {1}", ServerSideOnly = true, IsPredicate = true)] | ||
[Expression(PN.SqlServer, "dbo.regex({0}, {1})", ServerSideOnly = true, IsPredicate = true)] // supported via https://github.com/infiniteloopltd/SQLServerRegex | ||
public static bool RegExpIsMatch(string text, string expression) |
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.
Add xmldocs for this method.
I'm still against using Introduce only |
It's okay for me, if I could support it in a extension, so I could add my own "Expressions" so it works on my system with std regex |
@jogibear9988 this is a really good point in the context of source-generated regular expressions, which may not operate as expected in this context. Like sdanyliv said, it will create the expectation that the db will operate on the same regex rules that .net does, which is not true for any db engine. |
Yes, I wanted to now, is there a way for me, to add the line I did in Expressions.cs later in my usercode? So that in my code we still could use the "Regexp" object? We mostly use simple Regexes, wich would worknearly in all DBs. |
@@ -10,5 +10,13 @@ public static bool GenerateScopeIdentity | |||
get => SqlServerOptions.Default.GenerateScopeIdentity; | |||
set => SqlServerOptions.Default = SqlServerOptions.Default with { GenerateScopeIdentity = value }; | |||
} | |||
|
|||
/// <summary> | |||
/// You can specify a UserDefinedFunction wich is called to run a regex against your data. |
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.
typo in "wich".
/// Look at https://github.com/infiniteloopltd/SQLServerRegex for a sample | ||
/// in this case the value would be "dbo.regex({0}, {1})" | ||
/// </summary> | ||
public static string? RegexUserDefinedFunctionName { get; set; } |
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.
Do we really want to have such specific options? It's basically exactly the same level of abstraction as [Expression]
but for a single specific function.
We could imagine similar options for many other SQL libraries -- and I really don't think we want to do that.
Moreover, I'd rather reserve the "official" linq2db regex function to a possible future "native" regex function in SQL Server (should it ever happen).
I'd rather have:
- Docs/fully working example code to create such a static function with
[Expression("dbo.regex({0}, {1})")]
. - Shipping such functions in different namespaces that can be imported (
using LinqToDB.InfiniteLoop
) and/or different libraries.
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.
I'd move the complete code to an external library, if someone could tell me, how I could accomplish the add of the C# regex class to Expressions.cs, so I could use that in my queries
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.
@jogibear9988 This PR is currently doing a (string, string)
extension, isn't it?
Do you mean you'd like to be able to handle Regex.IsMatch(..)
?
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.
yes, I'd like to use regex.ismatch in my queries
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.
I'm gonna be honest, that's terrifying.
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.
But as I also saind, if we don't want to do this change, it is okay.
I'm only looking for a way, how I can extend Expressions.cs default... from outside.
Should we add a method? Problem is, that once initialized, adding something to it, does not have an effect. So it needs refactoring
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.
I think your approche is to limited.
Can you elaborate? Seems much more generic and flexible to me. In any case, it covers dbo.regex({1}, {2})
that you're wanting in this PR.
Here's why I think your approach is limited and worst:
- You introduce a new option
RegexUserDefinedFunctionName
that will only work forRegex.IsMatch
. No other function is supported. Right now there's PR Support for NodaTime #3624 that's adding a lot of ugly code to support NodaTime that would benefit from a generic way to do this. RegexUserDefinedFunctionName
is static. What if I want a different config for two different DB instances? How could we easily run tests in parallel?
IMHO this design does not scale to more functions/libraries and is not easily maintainable long-term.
It moves inside linq2db core something that could trivially be done in user-land.
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.
I'm only looking for a way, how I can extend Expressions.cs default... from outside.
Should we add a method? Problem is, that once initialized, adding something to it, does not have an effect. So it needs refactoring
I think this is the way to go and it's basically my proposal above.
Yes, it's a non-trivial refactoring but I think it is the best option long-term. Only issue right now is that there's the large @sdanyliv parser refactoring going on. We need to merge this before doing other large changes.
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.
okay for me
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.
BTW will gladly help with this. It's been on my mind for a while.
efcore seem to support this for postgres: https://www.npgsql.org/efcore/mapping/translations.html#string-functions |
They also seem to automaticly map a regex function for sqlite (if you'd like to) |
It is Roji's decision because he created PostgreSQL provider, and probably ported to SQLite. And I think he is wrong here. |
I don't think so ;-) , but I'll wait till we have a possibility to add my own mappings for Standard methods. |
23025cd
to
935c8b5
Compare
So, I now removed the functionality from Linq2db and added a sample in the Tests. |
@jogibear9988 looks like you remove both the expression and the |
No, I moved everything to an extra file. Cause for me it only makes sense if SqlServer is also supported. So I only added it to the tests as a sample. If someone likes to use it, he could copy the file from the tests. Or maybe I create a extra Nuget for Linq2db -> For Example Linq2DB.Regex, wich then maybe also contains the assembly for sqlserver |
So we can merge this, as a sample, or close the pull req. |
Given that the code doesn't build without changes to the |
@viceroypenguin now I understand, it should build. This was wrong, I'll fix it |
935c8b5
to
0dc6676
Compare
[LinqToDB.Sql.Expression(ProviderName.Oracle, "REGEXP_LIKE({0}, {1})", ServerSideOnly = true, IsPredicate = true)] | ||
[LinqToDB.Sql.Expression(ProviderName.SapHana, "{0} REGEXP {1}", ServerSideOnly = true, IsPredicate = true)] | ||
[LinqToDB.Sql.Expression(ProviderName.SqlServer, "dbo.regex({0}, {1})", ServerSideOnly = true, IsPredicate = true)] | ||
public static bool IsMatch(string text, string expression) |
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.
DB2 LUW : https://www.ibm.com/docs/en/db2/11.5?topic=functions-regexp-like
DB2 zOS: https://www.ibm.com/docs/en/db2-for-zos/12?topic=functions-regexp-like
DB2 i: https://www.ibm.com/docs/en/i/7.5?topic=predicates-regexp-like-predicate
Firebird: https://firebirdsql.org/file/documentation/html/en/refdocs/fblangref30/firebird-30-language-reference.html#fblangref30-commons-predsimilarto
INFORMIX: https://www.ibm.com/docs/en/informix-servers/14.10?topic=routines-regex-match-function
ClickHouse: https://clickhouse.com/docs/en/sql-reference/functions/string-search-functions#match
[LinqToDB.Sql.Expression(ProviderName.PostgreSQL, "{0} ~ {1}", ServerSideOnly = true, IsPredicate = true)] | ||
[LinqToDB.Sql.Expression(ProviderName.Oracle, "REGEXP_LIKE({0}, {1})", ServerSideOnly = true, IsPredicate = true)] | ||
[LinqToDB.Sql.Expression(ProviderName.SapHana, "{0} REGEXP {1}", ServerSideOnly = true, IsPredicate = true)] | ||
[LinqToDB.Sql.Expression(ProviderName.SqlServer, "dbo.regex({0}, {1})", ServerSideOnly = true, IsPredicate = true)] |
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.
don't map non-standard 3rd-party functions
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 only a sample class in the tests. but I can remove
[LinqToDB.Sql.Expression(ProviderName.Oracle, "REGEXP_LIKE({0}, {1})", ServerSideOnly = true, IsPredicate = true)] | ||
[LinqToDB.Sql.Expression(ProviderName.SapHana, "{0} REGEXP {1}", ServerSideOnly = true, IsPredicate = true)] | ||
[LinqToDB.Sql.Expression(ProviderName.SqlServer, "dbo.regex({0}, {1})", ServerSideOnly = true, IsPredicate = true)] | ||
public static bool IsMatch(string text, string expression) |
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.
missing overloads for additional parameters, supported by some DBs (e.g. options)
/// For SQLServer support you need to register the function from | ||
/// https://github.com/infiniteloopltd/SQLServerRegex | ||
/// </summary> | ||
public static void AddRegexSupport() |
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.
I would prefer to avoid explicit registrations. Wether we register it automatically or don't map members at all
EnableSqliteRegex(connection.Connection); | ||
} | ||
|
||
public static void EnableSqliteRegex(DbConnection connection) |
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.
we don't need it. User could register function from his code, if he needs it
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.
look at efcore, there EF does the registration for you
} | ||
|
||
[Test] | ||
public void Test698([IncludeDataSources(TestProvName.AllSQLite, TestProvName.AllPostgreSQL, TestProvName.AllOracle)] string context) |
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.
all supported databases and APIs should be tested
@MaceWindu whats the advantage to use this instead of Linq.Expressions.MapMember? |
Check some basic notes here https://github.com/linq2db/linq2db/wiki/Linq-To-DB-6#member-translators |
supersedes #1077
Fixes #698