Translate List<T> operations to PG array #1185
Conversation
| { | ||
| internal static class TypeExtensions | ||
| { | ||
| internal static bool IsGenericList(this Type type) |
There was a problem hiding this comment.
The name could be simplified:
| internal static bool IsGenericList(this Type type) | |
| internal static bool IsList(this Type type) |
There was a problem hiding this comment.
This was just to make it explicit that non-generic lists aren't covered.
| /// Note that mapping PostgreSQL arrays to .NET List{T} is also supported via <see cref="NpgsqlArrayListTypeMapping"/>. | ||
| /// See: https://www.postgresql.org/docs/current/static/arrays.html | ||
| /// </remarks> | ||
| public class NpgsqlArrayArrayTypeMapping : NpgsqlArrayTypeMapping |
There was a problem hiding this comment.
Let's rename this to NpgsqlArrayTypeMapping while NpgsqlArrayTypeMapping to NpgsqlCollectionTypeMapping. Otherwise, NpgsqlArrayArrayTypeMapping sounds like jagged array mapping.
There was a problem hiding this comment.
The problem is that this isn't about collections (which have a specific meaning in .NET)... The idea here was to include both the name of the .NET type and the PG type, since type mappings link the two. I wouldn't be too concerned about naming here - this is pretty internal types that users shouldn't ever encounter...
src/EFCore.PG/Storage/Internal/Mapping/NpgsqlArrayArrayTypeMapping.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| class SingleDimComparerWithEquals<TElem> : ValueComparer<TElem[]> |
There was a problem hiding this comment.
Why not to have only one comparer which internally will call EqualityComparer<TElem>.Defeault.Equals? The JIT can optimize some cases and inline method bodies (see dotnet/coreclr#14125).
There was a problem hiding this comment.
I agree there's probably some simplification to be done here, but let's do this in another PR, OK? This one is mostly about list translations and I don't want to go too far with other stuff etc...
src/EFCore.PG/Storage/Internal/Mapping/NpgsqlArrayTypeMapping.cs
Outdated
Show resolved
Hide resolved
roji
force-pushed
the
ListTranslations
branch
2 times, most recently
from
966929b to
3aa6b64
Compare
|
@YohDeadfall can you please give this another look? #1189 depends on this. |
src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlArrayTranslator.cs
Outdated
Show resolved
Hide resolved
| // TODO: Handle List<> | ||
| if (methodCall.Arguments.Count > 0 && methodCall.Arguments[0].Type.IsArray) | ||
| if (methodCall.Arguments.Count > 0 && ( | ||
| methodCall.Arguments[0].Type.IsArray || methodCall.Arguments[0].Type.IsGenericList())) |
There was a problem hiding this comment.
nit: Having all on a single line would be better, but it's up to you. Is it perf sensitive?
There was a problem hiding this comment.
I guess I'm starting to make more of an effort to respect 120 chars width :) Must be more bad EF influence...! But in this case it seems to express the logical structure better (or and and)...
Is it perf sensitive?
You mean because of the double methodCall.Arguments[0]? Definitely not perf sensitive to that degree :) This is part of "compilation", which happens once per LINQ query structure and the results are then cached.
* Match our List<T> translation capabilities to CLR array. * Improve some mapping and inference aspects. Closes #395
roji
force-pushed
the
ListTranslations
branch
from
3aa6b64 to
6759e00
Compare
Closes #395