Translate LIKE ANY expressions by austindrenski · Pull Request #430 · npgsql/efcore.pg

austindrenski · 2018-05-27T04:28:16Z

Translate LIKE ANY expressions

Work in progress. See #422 for more information.

Status

This PR currently has very brittle support for translation.

Examples

1. `Any` + `StartsWith`

string[] collection = new string[] { "a", "b", "c" };

source.Where(x => collection.Any(y => x.SomeString.StartsWith(y)));

SELECT x."SomeProperty"
FROM source AS x
WHERE x."SomeProperty" LIKE ANY ('{a%,b%,c%}');

2. `Any` + `EndsWith`

string[] collection = new string[] { "a", "b", "c" };

source.Where(x => collection.Any(y => x.SomeString.EndsWith(y)));

SELECT x."SomeProperty"
FROM source AS x
WHERE x."SomeProperty" LIKE ANY ('{%a,%b,%c}');

3. `Any` + `Like`

string[] collection = new string[] { "a", "b", "c%" };

source.Where(x => collection.Any(y => EF.Functions.Like(x.SomeString, y)));

SELECT x."SomeProperty"
FROM source AS x
WHERE x."SomeProperty" LIKE ANY ('{a,b,c%}');

4. `Any` + `ILike`

string[] collection = new string[] { "a", "b", "c%" };

source.Where(x => collection.Any(y => EF.Functions.ILike(x.SomeString, y)));

SELECT x."SomeProperty"
FROM source AS x
WHERE x."SomeProperty" ILIKE ANY ('{a,b,c%}');

5. `All` + `StartsWith`

string[] collection = new string[] { "a", "b", "c" };

source.Where(x => collection.All(y => x.SomeString.StartsWith(y)));

SELECT x."SomeProperty"
FROM source AS x
WHERE x."SomeProperty" LIKE ALL ('{a%,b%,c%}');

6. `All` + `EndsWith`

string[] collection = new string[] { "a", "b", "c" };

source.Where(x => collection.All(y => x.SomeString.EndsWith(y)));

SELECT x."SomeProperty"
FROM source AS x
WHERE x."SomeProperty" LIKE ALL ('{%a,%b,%c}');

7. `All` + `Like`

string[] collection = new string[] { "a", "b", "c%" };

source.Where(x => collection.All(y => EF.Functions.Like(x.SomeString, y)));

SELECT x."SomeProperty"
FROM source AS x
WHERE x."SomeProperty" LIKE ALL ('{a,b,c%}');

8. `All` + `ILike`

string[] collection = new string[] { "a", "b", "c%" };

source.Where(x => collection.All(y => EF.Functions.ILike(x.SomeString, y)));

SELECT x."SomeProperty"
FROM source AS x
WHERE x."SomeProperty" ILIKE ALL ('{a,b,c%}');

- Defines the `CustomArrayExpression` type. - Replaces `ArrayAnyExpression`. - Provides hooks for both `ANY` and `ALL` operations. Related: - https://www.postgresql.org/docs/current/static/functions-comparisons.html

roji

@austindrenski thanks for this great work, it's really nice to see the provider translating more complex expression types. This will definitely be a nice addition!

I know this is still WIP, but I've submitted a first review. Drop me a note when you want me to look again.

roji · 2018-05-27T07:34:53Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

            // Prefer the default EF Core translation if one exists
-            var result = base.VisitSubQuery(expression);
-            if (result != null)
+            if (base.VisitSubQuery(expression) is Expression result)


Why not just chain these with null coalescing:

=> base.VisitSubQuery(expression) ?? VisitLikeAny(expression) ?? VisitEqualsAny(expression)

roji · 2018-05-27T07:40:39Z

src/EFCore.PG/Query/Expressions/Internal/CustomArrayExpression.cs

+    /// <remarks>
+    /// See https://www.postgresql.org/docs/current/static/functions-comparisons.html
+    /// </remarks>
+    public class CustomArrayExpression : Expression, IEquatable<CustomArrayExpression>


Let's rename this to ArrayAnyAllExpression to make the naming more specific. After all there may be other array-related expressions out there.

roji · 2018-05-27T07:40:51Z

src/EFCore.PG/Query/Expressions/Internal/CustomArrayExpression.cs

+namespace Npgsql.EntityFrameworkCore.PostgreSQL.Query.Expressions.Internal
+{
+    /// <summary>
+    /// Represents a PostgreSQL ANY expression (e.g. 1 = ANY ('{0,1,2}'), 'cat' LIKE ANY ('{a%,b%,c%}')).


Update doc comment

roji · 2018-05-27T07:42:27Z

src/EFCore.PG/Query/Expressions/Internal/CustomArrayExpression.cs

+        /// <summary>
+        /// The collection of values or patterns to test for the <see cref="Operand"/>.
+        /// </summary>
+        public virtual Expression Collection { get; }


Rename to Array for precision

roji · 2018-05-27T07:43:51Z

src/EFCore.PG/Query/Expressions/Internal/CustomArrayExpression.cs

+        /// <summary>
+        /// True if the operator is modified by ANY; otherwise, false to modify with ALL.
+        /// </summary>
+        readonly bool _any;


A two-value enum would be slightly cleaner

roji · 2018-05-27T07:45:00Z

src/EFCore.PG/Query/Expressions/Internal/CustomArrayExpression.cs

+            => obj is CustomArrayExpression likeAnyExpression && Equals(likeAnyExpression);
+
+        /// <inheritdoc />
+        public bool Equals(CustomArrayExpression other)


Missing Operator and ANY/ALL

roji · 2018-05-27T07:46:33Z

src/EFCore.PG/Query/Expressions/Internal/CustomArrayExpression.cs

+        /// <summary>
+        /// The type of the operator expression (ANY or ALL).
+        /// </summary>
+        public virtual string OperatorType => _any ? "ANY" : "ALL";


Badly named, the operator is the comparison operator, rather than the ANY/ALL. I'd omit this entirely, it's the responsibility of NpgsqlQuerySqlGenerator to actually render this expression to SQL, so it should know how to translate your bool (or 2-value enum as I suggested) to SQL.

roji · 2018-05-27T07:47:43Z

src/EFCore.PG/Query/Sql/Internal/NpgsqlQuerySqlGenerator.cs

+        /// <summary>
+        /// Produces expressions like: 1 = ANY ('{0,1,2}') or 'cat' LIKE ANY ('{a%,b%,c%}').
+        /// </summary>
+        public Expression VisitArrayOperator(CustomArrayExpression arrayExpression)


Rename to VisitArrayAnyAll

roji · 2018-05-27T07:51:00Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

+                !(whereClause.Predicate is MethodCallExpression methodCallExpression))
+                return null;
+
+            if (!(Visit(methodCallExpression.Object ?? methodCallExpression.Arguments[1]) is Expression instance))


Doesn't this Visit() call actually generate SQL? Shouldn't it happen only after you've made sure you can translate the subquery (i.e. checking the specific methods below)?

… enum - Recognizes `AnyResultOperator` vs `AllResultOperator`

roji

I forgot to write the most important comment in my last review...

Unfortunately StartsWith and EndsWith are a bit more complicated than you'd think, because of escaping issues... the problem in your current implementation is that % and _ will be interpreted as wildcards. See NpgsqlStringStartsWithTranslator to see how regular StartsWith is translated. If it's a constant pattern we escape client-side and all is well, but if it isn't, then we have to resort to some trickery: we first do an imprecise LIKE which leverages indices but treats % and _ as wildcards, and then do LEFT() to extract the substring and do a precise comparison.

Unfortunately you can't duplicate all this complexity with LIKE ANY - it seems like we're a bit stuck on that. You can still implement the logic for constant/literal arrays of strings (e.g. .Where(x => new[] { "a", "b", "c" }.Any(y => x.Animal.StartsWith(y))), where you can escape client-side, and fall back to client-evaluation for other cases.

I'm not 100% sure this is worth the effort - tell me what you think.

roji · 2018-05-27T15:32:23Z

test/EFCore.PG.FunctionalTests/Query/LikeAnyQueryNpgsqlTest.cs

+            /// <summary>
+            /// The primary key.
+            /// </summary>
+            [Key]


nit: please remove, Id is a key by convention.

roji · 2018-05-27T15:32:36Z

test/EFCore.PG.FunctionalTests/Query/LikeAnyQueryNpgsqlTest.cs

+        /// <param name="sql">The SQL statement or fragment to search for in the logs.</param>
+        public void AssertContainsSql(string sql)
+        {
+            Assert.Contains(sql, Fixture.TestSqlLoggerFactory.Sql);


nit: expression-bodied method

roji · 2018-05-27T15:32:49Z

test/EFCore.PG.FunctionalTests/Query/LikeAnyQueryNpgsqlTest.cs

+            }
+
+            /// <inheritdoc />
+            public void Dispose()


nit: expression-bodied method

roji · 2018-05-27T15:32:52Z

test/EFCore.PG.FunctionalTests/Query/LikeAnyQueryNpgsqlTest.cs

+            /// <returns>
+            /// A <see cref="LikeAnyContext"/> for testing.
+            /// </returns>
+            public LikeAnyContext CreateContext()


nit: expression-bodied method

roji · 2018-05-27T15:36:47Z

src/EFCore.PG/Query/Expressions/Internal/ArrayAnyAllExpression.cs

+        /// <summary>
+        /// Represents an ALL array comparison.
+        /// </summary>
+        ALL = 1 << 0


I don't think there's any reason to give values to this enum's members, as they're not meant to be persisted or transferred anywhere (and I'm curious as to the << 0 - you're really used to flag enums? :)).

lol... bad habit of compulsively incorporating flag enums... but definitely irrelevant here.

I've dropped the assignments. I think I was considering some uninitialized condition when I drafted it, but simplified to avoid it.

roji · 2018-05-27T15:56:46Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

+        /// A 'LIKE ANY' or 'ILIKE ANY' expression or null.
+        /// </returns>
+        [CanBeNull]
+        protected virtual Expression VisitLikeAny([NotNull] SubQueryExpression expression)


Rename to VisitLikeAnyAll

roji · 2018-05-27T15:57:08Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

+                !(whereClause.Predicate is MethodCallExpression call))
+                return null;
+
+            Expression source = queryModel.MainFromClause.FromExpression;


nit: use var

austindrenski · 2018-05-27T17:24:50Z

Unfortunately StartsWith and EndsWith are a bit more complicated than you'd think, because of escaping issues...

I'm not 100% sure this is worth the effort - tell me what you think.

I'll need to spend some more time looking into this... For now, I'm okay punting on StartsWith and EndsWith and focusing on Like and ILike.

5b1f138 addresses your latest review, adds some fixes/tests, and disables support for StartsWith and EndsWith.

If possible, I would like to get a minimal level of LIKE ANY/LIKE ALL support added for the 2.1 release.

Does this sound feasible to you? If so, do you see any blocking issues with the current implementation?

- Details need to be worked out for `StartsWith(...)` and `EndsWith(...)`. - But this should not block standard `LIKE`/`ILIKE` `ANY`/`ALL` translation. - Added tests + fixes for `ALL` comparisons.

roji · 2018-05-27T20:31:32Z

Does this sound feasible to you? If so, do you see any blocking issues with the current implementation?

Yes, that sounds feasible. I'll review again and try to merge in the next couple of days. Note that 2.1.0 is supposed to come out this week, at which point the priority will be to release everything on the Npgsql side.

- Note: Includes type handler file renames from "Npgsq*" to "Npgsql*" - This could be split into another patch. - First goal: feature parity with mapped `T[]` operations. - 8 of 10 unit tests passing: - Passes - Roundtrip - SequenceEqual - Length/Count - Contains/Any - Issues: - Blocked by npgsql#430 - There should be a better system for these types of mappings. - Along the lines of `IMethodCallTranslator`.

- First goal: feature parity with mapped `T[]` operations. - 10 of 10 unit tests passing: - Passes - Roundtrip - SequenceEqual - Length/Count - Contains/Any - Indexers - Issues: - Blocked by npgsql#430 - There should be a better system for these types of mappings. - Along the lines of `IMethodCallTranslator`.

roji

Almost there, I'll be watching closely for your fixes so as to merge them as soon as they come.

roji · 2018-05-29T05:19:08Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

+            // ReSharper disable AssignNullToNotNullAttribute
+            switch (call.Method.Name)
+            {
+// TODO: Can StartsWith and EndsWith be implemented correctly?


nit: indent TODO comment and remove the commented-out cases (until implemented).

roji · 2018-05-29T05:20:49Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

+            var source = queryModel.MainFromClause.FromExpression;
+
+            // ReSharper disable AssignNullToNotNullAttribute
+            switch (call.Method.Name)


I think that a string comparison for the method name isn't the right approach here - it will capture any method named Like. Better to have a static readonly MethodInfo field for Like() and ILike(), looking it up via the GetRuntimeMethod() extension, and then comparing for actual equality (see the NpgsqlRegexIsMatchTranslator for an example).

We use string comparison in cases where we've also already made sure that the type containing the method is the correct one.

Completely agree. Got bit by this yesterday in the list operations PR, but didn't think to check back here...

roji · 2018-05-29T05:21:57Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

+        }
+
+        [CanBeNull]
+        static MethodCallExpression ComputeAny([NotNull] QueryModel queryModel)


nit: any reason to separate this out to a utility method? It's only called once...

roji · 2018-05-29T05:23:15Z

test/EFCore.PG.FunctionalTests/Query/LikeAnyAllQueryNpgsqlTest.cs

+        #region StartsWithTests
+
+        [Fact(Skip = "StartsWith not currently supported")]
+        public void Array_Any_StartsWith()


Please remove the skipped tests for now (i.e. keep them in your local branch where you'll be working on {Starts,Ends]With support.

Also removes future features that were commented/skipped.

austindrenski · 2018-05-29T14:10:56Z

@roji Just pushed some changes to address your review.

- First goal: feature parity with mapped `T[]` operations. - 10 of 10 unit tests passing: - Passes - Roundtrip - SequenceEqual - Length/Count - Contains/Any - Indexers - Issues: - Blocked by npgsql#430 - There should be a better system for these types of mappings. - Along the lines of `IMethodCallTranslator`.

roji · 2018-05-29T20:36:10Z

Thanks, merged for 2.1.

- First goal: feature parity with mapped `T[]` operations. - 10 of 10 unit tests passing: - Passes - Roundtrip - SequenceEqual - Length/Count - Contains/Any - Indexers - Issues: - Blocked by npgsql#430 - There should be a better system for these types of mappings. - Along the lines of `IMethodCallTranslator`.

Adds simple support to translates LIKE ANY expressions

ab16cc4

- Defines the `CustomArrayExpression` type. - Replaces `ArrayAnyExpression`. - Provides hooks for both `ANY` and `ALL` operations. Related: - https://www.postgresql.org/docs/current/static/functions-comparisons.html

roji requested changes May 27, 2018

View reviewed changes

Adjusts naming for precision and refactors to use ArrayComparisonType…

4ea4fba

… enum - Recognizes `AnyResultOperator` vs `AllResultOperator`

roji requested changes May 27, 2018

View reviewed changes

Disables (temporarily) StartsWith(...) and EndsWith(...) support

5b1f138

- Details need to be worked out for `StartsWith(...)` and `EndsWith(...)`. - But this should not block standard `LIKE`/`ILIKE` `ANY`/`ALL` translation. - Added tests + fixes for `ALL` comparisons.

austindrenski force-pushed the translate-like-any branch from 8105216 to 5b1f138 Compare May 27, 2018 19:25

austindrenski mentioned this pull request May 28, 2018

Map List<T> operations to PostgreSQL arrays (old) #431

Closed

roji requested changes May 29, 2018

View reviewed changes

Replaces name switch with MethodInfo comparison

cafe41f

Also removes future features that were commented/skipped.

roji merged commit 41678ef into npgsql:dev May 29, 2018

austindrenski deleted the translate-like-any branch May 29, 2018 20:38

austindrenski self-assigned this Aug 16, 2018

austindrenski added the enhancement label Aug 16, 2018

Conversation

austindrenski commented May 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Translate LIKE ANY expressions

Status

Examples

1. Any + StartsWith

2. Any + EndsWith

3. Any + Like

4. Any + ILike

5. All + StartsWith

6. All + EndsWith

7. All + Like

8. All + ILike

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austindrenski commented May 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roji commented May 27, 2018

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austindrenski commented May 29, 2018

Uh oh!

roji commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

austindrenski commented May 27, 2018 •

edited

Loading

1. `Any` + `StartsWith`

2. `Any` + `EndsWith`

3. `Any` + `Like`

4. `Any` + `ILike`

5. `All` + `StartsWith`

6. `All` + `EndsWith`

7. `All` + `Like`

8. `All` + `ILike`

austindrenski commented May 27, 2018 •

edited

Loading