Map List<T> operations to PostgreSQL arrays (old) #431
Conversation
| /// <summary> | ||
| /// Provides unit tests for list operator translations. | ||
| /// </summary> | ||
| public class ListQueryNpgsqlTest : IClassFixture<ListQueryNpgsqlTest.ListQueryNpgsqlFixture> |
There was a problem hiding this comment.
You may want to consider having a single test suite for both array and list. After all, the operations are logically the same and we map to the same PostgreSQL type. Let me know what you think.
| @@ -194,7 +200,7 @@ class SingleDimComparerWithEquals<TElem> : ValueComparer<List<TElem>> | |||
| public SingleDimComparerWithEquals() : base( | |||
| (a, b) => Compare(a, b), | |||
| o => o.GetHashCode(), // TODO: Need to get hash code of elements... | |||
| source => DoSnapshot(source)) {} | |||
| source => DoSnapshot(source)) { } | |||
There was a problem hiding this comment.
nit: have seen these added spaces before, can you please configure your IDE not to do this? It makes PRs harder to review (and I prefer the space-less version).
There was a problem hiding this comment.
Ah, yes -- should be fixed going forward.
austindrenski
force-pushed
the
list-operator-translation
branch
5 times, most recently
from
7086bcb to
470e8fc
Compare
|
@roji Parity with the already-mapped array operations is coming together. However, this is running into changes pending in #430, specifically, the changes around Also, I noticed some file names were prefixed with Npgsq instead of Npgsql. I've fixed them in this PR, but I can split those out to a separate patch if you'd prefer. (If you want to fix them directly on dev, that works too.) |
|
Splitting that out to a separate PR is always best, even if it's a small one. I'll try to merge #430 (or at least to review again) ASAP. |
austindrenski
force-pushed
the
list-operator-translation
branch
from
470e8fc to
d9af112
Compare
|
@roji I just pushed d9af112 which completes feature parity with the array operation tests in Next steps
|
Supported:
Planned
|
austindrenski
force-pushed
the
list-operator-translation
branch
from
bc0f0d5 to
2efbbbd
Compare
There was a problem hiding this comment.
Here's a first batch of comments - this isn't a simple thing to get exactly right.
Also don't forget to update the array docs page.
| /// <summary> | ||
| /// The default member translators registered by the Npgsql provider. | ||
| /// </summary> | ||
| static readonly IMemberTranslator[] MemberTranslators = |
There was a problem hiding this comment.
I'm totally fine with these refactorings, but in the future please try to separate them into a different commit (even if it's in the same PR), just a matter of cleanly separating changes and making reviewing easier.
There was a problem hiding this comment.
That's my bad. This refactoring was part of a larger substance change that I later reverted. (Had hoped the EF Core model might let me snag property calls as member translations...)
I'll take care to split out pure refactorings in the future.
| [CanBeNull] | ||
| public Expression Translate(MethodCallExpression expression) | ||
| { | ||
| if (expression.Object != null && !typeof(IList).IsAssignableFrom(expression.Object.Type)) |
There was a problem hiding this comment.
Be careful here, we only want to translate arrays and List<T>, and not IList<T>...
| // TODO: use #430 to map @> to source.All(x => other.Contains(x)); | ||
| // TODO: use #430 to map && to soucre.Any(x => other.Contains(x)); | ||
|
|
||
| switch (expression.Method.Name) |
There was a problem hiding this comment.
Make sure that the declaring type of the method is actually Enumerable.
|
|
||
| switch (expression.Method.Name) | ||
| { | ||
| // TODO: Currently handled in NpgsqlSqlTranslatingExpressionVisitor. |
There was a problem hiding this comment.
Not sure what this comment refers too... I don't think there's currently any Append/Concat support for arrays.
There was a problem hiding this comment.
I added a case to handle ConcatResultOperator in NpgsqlSqlTranslatingExpressionVisitor.
The TODO is there because I would like to eventually pull the *ResultOperator handling out of VisitSubQuery and into some pluggable class like IMethodCallTranslator.
| { | ||
| // TODO: Currently handled in NpgsqlSqlTranslatingExpressionVisitor. | ||
| case nameof(Enumerable.Append): | ||
| case nameof(Enumerable.Concat): |
There was a problem hiding this comment.
I think we need to be more careful here with regards to the exact type of the other argument. It's fine if it's an array or list, but if it's an actual .NET IEnumerable<T> we shouldn't translate it. Same for Prepend below.
There was a problem hiding this comment.
Hmmm... I hadn't consider that risk. I'll add some guards clauses.
Though this actually sounds like it could be a fun case to test in the next release. (E.g. could a subquery be appended into a Postgres array?)
| case nameof(Enumerable.SequenceEqual): | ||
| return Expression.MakeBinary(ExpressionType.Equal, expression.Arguments[0], expression.Arguments[1]); | ||
|
|
||
| case nameof(ToString): |
There was a problem hiding this comment.
We should only translate methods where the server evaluation yields the exact same results as the client evaluation, but calling ToString() in .NET generates a completely different thing than PostgreSQL (which actually generates a useful representation...). If you really think there's value in allowing users to call PostgreSQL array_to_string, then that needs to be done via something like EF.Functions.ArrayToString()...
There was a problem hiding this comment.
Good points. To be fair, I'm not sure how much value there is, but its cheap to include...so I added an extension class with NpgsqlArrayExtensions.ArrayToString(...).
This is actually an improvement overall, because it also lets us provide arguments for that ToString() wouldn't handle.
| // Translate someArray.Length | ||
| if (subQueryModel.ResultOperators.First() is CountResultOperator) | ||
| return Expression.ArrayLength(Visit(fromExpression)); | ||
| return new SqlFunctionExpression("array_length", typeof(int), new[] { Visit(fromExpression), Expression.Constant(1) }); |
There was a problem hiding this comment.
In principle I'd like to maintain the separation of responsibility between NpgsqlSqlTranslatingExpressionVisitor and NpgsqlQuerySqlGenerator, which this change compromises. This class should output expressions that are as close as possible to .NET expressions, and which will be translated later to raw SQL by NpgsqlQuerySqlGenerator. This is why I prefer for us to output an ArrayLength expression for later translation.
There was a problem hiding this comment.
That makes sense...but the trouble here is that Expression.ArrayLength throws when the expression isn't an array. The goal here was to limit the amount of branching for the same intent.
That said, I can revert and handle array and list length separately if you'd prefer to maintain separation.
I could also define another expression type along the lines of ArrayOrListLengthExpression that's a little more forgiving during instantiation.
| if (containsItem != null) | ||
| return new ArrayAnyExpression(containsItem, Visit(fromExpression)); | ||
| } | ||
| if (Visit(contains.Item) is Expression containsItem && Visit(fromExpression) is Expression source) |
There was a problem hiding this comment.
Thanks for these little improvements, I need to get more used to writing this way.
| @@ -65,24 +74,27 @@ protected override Expression VisitSubQuery(SubQueryExpression expression) | |||
| if (properties.Count == 0) | |||
| return null; | |||
| var lastPropertyType = properties[properties.Count - 1].ClrType; | |||
| if (lastPropertyType.IsArray && lastPropertyType.GetArrayRank() == 1 && subQueryModel.ResultOperators.Count > 0) | |||
| if (typeof(IList).IsAssignableFrom(lastPropertyType) && subQueryModel.ResultOperators.Count > 0) | |||
There was a problem hiding this comment.
As I wrote above, this is too wide. We definitely don't want to translate any IList<T> out there, only arrays and List<T>. I know this means there'll be some duplicated/separate handling for these two types, but I don't think we have a choice.
| protected override Expression VisitUnary(UnaryExpression expression) | ||
| { | ||
| // TODO: I don't think this is called any longer. |
There was a problem hiding this comment.
See comment in NpgsqlSqlTranslatingExpressionVisitor about reverting this
austindrenski
force-pushed
the
list-operator-translation
branch
from
2efbbbd to
07aa199
Compare
|
@roji Just pushed changes to address your review. Supported:
|
austindrenski
force-pushed
the
list-operator-translation
branch
from
c0a17fc to
88a57c7
Compare
|
I'm in the process of rebasing to the latest dev. Should have a new version pushed in the next 15 minutes. |
austindrenski
force-pushed
the
list-operator-translation
branch
2 times, most recently
from
5e0b2c9 to
1a4d96f
Compare
austindrenski
force-pushed
the
list-operator-translation
branch
from
fa50b65 to
d2fc9bd
Compare
Updatesd2fc9bd introduces some (fragile) support for the array operators I think Contains:src.Where(x => x.SomeList.All(y => x.SomeArray.Contains(y)));WHERE x."SomeArray" @> x."SomeList"Overlaps:src.Where(x => x.SomeArray.Any(y => x.SomeList.Contains(y)));WHERE x."SomeArray" && x."SomeList" |
Updates579cbae attempts to address the growing complexity in SubqueriesI'm working on a delegation approach for subquery pattern translation:
Switching
|
austindrenski
force-pushed
the
list-operator-translation
branch
3 times, most recently
from
ad5c0af to
ac28fc6
Compare
Creates Npgsql implementations of `IExpressionFragmentTranslator` and `IQueryOptimizer` to support more complex translations. These classes make it easier to inject new fragment translations and expression rewrites into the EF Core translation process. This also includes documentation and cleanup of the composite method call and member translators. This is split out from npgsql#431. Working examples can be found there.
Creates Npgsql implementations of `IExpressionFragmentTranslator` and `IQueryOptimizer` to support more complex translations. These classes make it easier to inject new fragment translations and expression rewrites into the EF Core translation process. This also includes documentation and cleanup of the composite method call and member translators. This is split out from npgsql#431. Working examples can be found there.
austindrenski
force-pushed
the
list-operator-translation
branch
from
6cbdb2c to
03086e4
Compare
- 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`.
| C# expression | SQL generated by Npgsql | |----------------------------------|-------------------------| | `Array.Length` | `array_length(1, 1)` | `IList.Count` | `array_length(a, 1)` | `Enumerable.Count(a)` | `array_length(a, 1)` | `Array[0]` | `a[1]` | `IList[0]` | `a[1]` | `Enumerable.ElementAt(a, 0)` | `a[1]` | `Enumerable.Append(a, b)` | `a || b` | `Enumerable.Concat(a, b)` | `a || b` | `Enumerable.Prepend(a, b)` | `b || a` | `Object.Equals(a, b)` | `a = b` | `Enumerable.SequenceEqual(a, b)` | `a = b` | `a.ToString()` | `array_to_string(a, ',')` | `Enumerable.IndexOf(a, 0)` | `COALESCE(array_position(a, 1), -1)` | `Enumerable.Contains(a, b)` | `b = ANY (a)` | C# expression | SQL generated by Npgsql | |----------------------------|-------------------------| | `a.All(x => b.Contains(x)` | `b <@ a` | `a.Any(x => b.Contains(x)` | `a && b` - Relational operators could also be supported via extensions on `EF.Functions`. - https://www.postgresql.org/docs/current/static/functions-array.html#ARRAY-OPERATORS-TABLE
- Improves how we verify that: - an array or list is supported - the function for an array or list is supported. - Renames `NpgsqlListTranslator` to `NpgsqlArrayTranslator` - Adds `NpgsqlArrayExtensions`
- This support is fragile: - Tests are passing...but more complex tests are needed. Contains (@>): - src.Where(x => x.SomeList.All(y => x.SomeArray.Contains(y))) - WHERE x."SomeArray" @> x."SomeList" Overlaps (&&): - src.Where(x => x.SomeArray.Any(y => x.SomeList.Contains(y))) - WHERE x."SomeArray" && x."SomeList"
This refactors `NpgsqlSqlTranslatingExpressionVisitor` to use `VisitSubQuery` to delegate to `VisitArraySubQuery` which itself delegates to more specialized virtual methods based on the subquery's result operator. The goal is to allow the subquery visits to be called recursively to handle more complex patterns. For example, translating the array operator `<@` involves identifying a subquery nested inside of a subquery. Rather than duplicating identification logic, we want to use the standard visitor approach and send a nested subquery back up to `Visit`. Another consideration is the ability to compose result operators within an `Any` or `All` result. These result operators are almost exclusively designed to have a nested condition or subquery.
The plan is to use an expression fragment translator to offload some of the work currently done in `NpgsqlSqlTranslatingExpressionVisitor` and make complex pattern translations more approachable. Specifically, this makes it tractable to handle `Array.Exists<T>` and `List<T>.Exists` for PostgreSQL array columns. This commit includes new tests in various stages: - Passing: - List_Exists_equals_with_literal_constant - List_Exists_less_than_with_literal_constant - Failing (simple): - List_Exists_less_than_with_literal_constant - Failing (complicated): - Array_Exists_equals_with_literal_constant - Array_Exists_equals_with_column_list_element - Array_Exists_less_than_with_literal_constant The simple failure is related to sign inversion when switching from C# to SQL patterns (e.g. 'y < 1' becomes '1 = ANY (...)`. The complicated failures are due to missing rewrite support for `Array.Exists<T>` and arrays generally. There is a built-in `ExistsToAnyRewritingExpressionVisitor` which handles lists, but it will need to be extended to support the same rewrite for arrays. Solving the above will provide exist translations for basic query shapes. But this support is fragile at best until the `NpgsqlArrayExpressionFragmentTranslator` class has a more thorough treatment for array identification. Currently, we just look for a `QuerySourceReferenceExpression` and assume its a `MainFromClause`. This could be handled better to allow for literal arrays, and possibly other set returning functions that PostgreSQL would consider valid in an array expression.
- Organizing the table. - Outlining sections to explain the pattern translations.
- Exposes a hook for adding rewriting expression visitors.
- Adds `NpgsqlExistsToAnyRewritingExpressionVisitor`.
- Modified from `ExistsToAnyRewritingExpressionVisitor` to
rewrite `Array.Exists<T>(T[], Predicate<T>)` as an expression
of `Any` and `Contains`.
Changed the private constructor to internal to make it available for expression visitors. Specifically, the following commit needs to visit `PgFunctionExpression` instances that may have been constructed in an early visitor. The constructor pass-through logic was a bit tangled, so each public constructor now passes directly to the internal one. This saves a few redundant null checks, but mostly it makes it clear which arguments are set to an internal default. The `ToString` and `GetHashCode` logic has been cleaned up, as well as adding an `IEquatable<T>` constraint. The thinking here is that since the methods were already there, we might as well let callers know. Something to consider for future use would be whether or not this expression should "own" its own SQL representation. Basically, the `ToString` logic duplicates a lot of the same code as `NpgsqlQuerySqlGenerator` (just without the instance/argument visits).
!# Result operator handling
Result operator handling is being moved (when possible)
from `NpgsqlSqlTranslatingVisitor` to expression fragment
visitors. Right now, this only affects array types with
specialized result operator methods.
- Moved:
- `ConcatResultOperator`
- `CountResultOperator`
-Planned:
- `ContainsResultOperator`
- `AnyResultOperator`
- `AllResultOperator`
!# Array indexers
This commit also changes the way that array indexes are handled.
Because both `T[]` and `List<T>` map to PostgreSQL arrays, we
should handle array access with the same code. We can do this by
rewriting `ExpressionType.ArrayIndex` as `ExpressionType.Index`.
This should open up support on things like substring translation
for `IEnumerable<char>` methods in addition to simple indexers.
Another benefit is cleaner SQL. We don't need to worry about
wrapping indexers in parentheses because PostgreSQL oeprator
precedence is well-documented.
```sql
-- before
SELECT (x."SomeArray"[2]);
-- after
SELECT x."SomeArray"[2];
```
!# String indexers
String indexers are translated to `substr` in PostgreSQL.
However, `substr` returns `text` not `char`. Even if `substr`
did return `char`, C# expression trees lift `char` types to
`int`. So this form of translation occurs:
```c#
someString[0] == 'T'
```
```sql
-- fails with 'operator does not exist: text = integer'
substr("someString", 1, 1) = 84
```
To avoid that failure, we wrap the `substr` call in `ascii`
to get the ASCII or UTF8 integer value. An interesting
alternative would be to cast the `text` to `bytea` and
then call `get_byte` which reverts to zero-based indexes
and saves us from specifying the unitary length of `substr`.
```sql
-- current
ascii(subsr("someString", 1, 1)) = 84
-- possible alternative
get_byte("someString"::bytea, 0) = 84
```
Opening 450 to gather feedback on this.
!# Visitor dispatching
This commit begins to change methods from public to protected on
`NpgsqlQuerySqlGenerator` and moves the responsibility for
dispatching from each expression's `Accept` method to the
generator's `VisitExtensions` method.
- `CompiledQueryNpgsqlTests.Query_with_array_parameter()` - `CompiledQueryNpgsqlTests.Query_with_array_parameter_async()` Both throw: ```c# System.InvalidCastException : Can't write CLR type System.String[] with handler type TextHandler ``` Tests are overridden for now with an example showing the non-compiled form does not throw. Needs further investigation.
austindrenski
force-pushed
the
list-operator-translation
branch
3 times, most recently
from
913ddf3 to
24017c4
Compare
Something to do with `MemberAccessBindingExpressionVisitor` happens under some specific circumstances that coincide with a compiled query. Added an `IsSafeToVisit` helper to `NpgsqlSqlTranslating...Visitor` for an additional guard when handling arrays that could be some type of replaced parameter.
austindrenski
force-pushed
the
list-operator-translation
branch
from
24017c4 to
a917e00
Compare
austindrenski
changed the title
Creates Npgsql implementations of `IExpressionFragmentTranslator` and `IQueryOptimizer` to support more complex translations. These classes make it easier to inject new fragment translations and expression rewrites into the EF Core translation process. This is split out from npgsql#431. Working examples can be found there.
Creates Npgsql implementations of `IExpressionFragmentTranslator` and `IQueryOptimizer` to support more complex translations. These classes make it easier to inject new fragment translations and expression rewrites into the EF Core translation process. This is split out from npgsql#431. Working examples can be found there.
|
Closing in favor of #541. |
Map
List<T>operations to PostgreSQL arraysWork in progress. See #395 for more information.
Objectives
Feature parity with previously mapped
T[]operations forList<T>.Map array operators for
T[]andList<T>.Translate array expressions
{operand} {operator} {ANY|ALL} ({array})Related
#356