#10582 - Translate SequenceEqual for Sqlite and SqlServer byte arrays #19594
Conversation
There was a problem hiding this comment.
@svengeance looks great, thanks! Aside from rebasing there's one small rename, but otherwise looks ready to merge.
src/Shared/EnumerableMethods.cs
Outdated
| @@ -18,6 +18,7 @@ internal static class EnumerableMethods | |||
| public static MethodInfo AnyWithoutPredicate { get; } | |||
| public static MethodInfo AnyWithPredicate { get; } | |||
| public static MethodInfo Contains { get; } | |||
| public static MethodInfo SequenceEquals { get; } | |||
There was a problem hiding this comment.
Please rename this to SequenceEqual (no s)
|
Regarding where to put this, I think it would make sense more as a separate RelationalByteArrayMethodCallTranslator, much like StringMethodTranslator or EnumHasFlagTranslator. These contain presumably "universal" relational translations, and we generally prefer implementing as method or member translator when possible, rather than modifying RelationalSqlTranslatingExpressionVisitor. @bricelam, does that sound like a good approach? |
|
Makes sense to me - otherwise Would you like to move the implementation to a relational method translator (as opposed to one each in sql/server method translator), or save that for another day? |
There was a problem hiding this comment.
This looks good to me, but as I wrote before we should consider moving this up to relational instead of reimplementing for each provider. Comparing blobs/byte arrays with the equality operators seems to be a pretty universal feature (at least it works on SQL Server, Sqlite, PG, MySQL), so this seems to be similar to translators like StringMethodTranslator, LikeTranslator which are RelationalMethodCallTranslatorProvider.
Any objections @ajcvickers @AndriySvyryd @bricelam @maumar?
test/EFCore.Sqlite.FunctionalTests/Query/GearsOfWarQuerySqliteTest.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace Microsoft.EntityFrameworkCore.Query.Internal | ||
| { | ||
| public class ByteArrayTranslator: IMethodCallTranslator |
There was a problem hiding this comment.
Rename to ByteArraySequenceEqualTranslator, to align with the other single-purpose translators. Other than that looks great.
|
Also needs a rebase. |
|
@roji should be good now. Let me know if there's anything else I can do to tidy up! |
This fixes #10582
Hopefully straightforward and simple implementation. I don't believe anything other than a direct call to
_sqlExpressionFactory.Equalis necessary since the method definition forSequenceEqualwith 2 args demands both be the same type.Would it be an improvement to place this in the
RelationalSqlTranslatingExpressionVisitor.VisitBinarymethod? Both implementations are the same for SqlServer/Sqlite, but I didn't want to make assumptions about any future inheritors.Cheers!