Regex replace and count #3062
Regex replace and count #3062
Conversation
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Thanks for the good work here @WhatzGames. I'm submitting a review, but in the interest of time etc. have pushed a commit to do the suggested fixes/improvements. I'll go ahead and merge - but please take a look and tell me if you see anything problematic/improvable, we can always do more work here.
| } | ||
|
|
||
| var (input, pattern) = (arguments[0], arguments[1]); | ||
| var typeMapping = ExpressionExtensions.InferTypeMapping(input, pattern); |
There was a problem hiding this comment.
I know the code was this way before, but it's incorrect - the type of the arguments/operands isn't related, and PG doesn't infer one from the other (for example, a varchar(8)-typed pattern doesn't mean that the input also has that type).
| MethodInfo method, | ||
| IReadOnlyList<SqlExpression> arguments, | ||
| IDiagnosticsLogger<DbLoggerCategory.Query> logger) | ||
| => TranslateIsMatch(instance, method, arguments, logger) |
There was a problem hiding this comment.
nit: no need to have parameters for instance, logger if they're unused (this is all private code, which we can change in the future if we need to)
| passingArguments, | ||
| nullable: true, | ||
| Enumerable.Repeat(true, passingArguments.Count), | ||
| typeof(int?), |
There was a problem hiding this comment.
This should be int, not int? - in SQL tree expressions, basically anything is nullable (due to SQL null propagation rules), so we have non-nullable types only.
| "regexp_replace", | ||
| passingArguments, | ||
| nullable: true, | ||
| Enumerable.Repeat(true, passingArguments.Count), |
|
|
||
| return result; | ||
| } | ||
|
|
| if (options.HasFlag(RegexOptions.Multiline)) | ||
| { | ||
| result = "n"; | ||
| }else if(!options.HasFlag(RegexOptions.Singleline)) |
|
|
||
| string? result; | ||
|
|
||
| if (options.HasFlag(RegexOptions.Multiline)) |
There was a problem hiding this comment.
nit: this is a conceptual switch over options
| /// </remarks> | ||
| public class NpgsqlRegexTranslator : IMethodCallTranslator | ||
| { | ||
| private static readonly MethodInfo IsMatch = |
There was a problem hiding this comment.
FYI we're starting to move away from this way of matching things in method/member translators; this lookup of MethodInfos via reflection causes them to always get preserved when using trimming, regardless of whether the user actually needs them or not (and therefore bloats the final binary). Instead, it's better to check the declaring type, and then simply check the method name as a string.
I'll rewrite to do this.
|
|
||
| AssertSql( | ||
| """ | ||
| SELECT regexp_replace(c."CompanyName", '^A', 'B', 'p') |
There was a problem hiding this comment.
nit: this should be right-aligned like all the other SQL baselines (generally please try following existing style practices in an existing codebase).
Took the time, but as far as I can tell nothing seems off. Thanks for the review and final changes. |
Closes npgsql#3059 Co-authored-by: Shay Rojansky <roji@roji.org>
2 participants
closes #3059
I currently do not translate Regex.Count for Postgres versions < 15, but it should be possible to provide a subquery for older version if desired.