Skip to content

Conversation

@ajcvickers
Copy link
Contributor

To allow mapping types like ranges, which have an element type, but don't behave as collections.

@ajcvickers ajcvickers requested a review from a team September 10, 2023 12:38
@ajcvickers
Copy link
Contributor Author

/cc @roji I haven't changed any places in query to call IsPrimitiveCollection instead of just looking at the element type, but we might want to do this before you introduce mappings that have an element type but are not primitive collections.

To allow mapping types like ranges, which have an element type, but don't behave as collections.
@roji
Copy link
Member

roji commented Sep 11, 2023

@ajcvickers I'm not sure I'll actually be introducing non-collection mappings which make use of the core "element type mapping" concept... The core concept really is about unlocking queryability (composing LINQ operators), so non-queryable types don't really have anything to gain by using this over simply having a provider-specific reference to their "element" type...

But regardless of that, yeah, we should do a quick pass and change places where we currently look at the element type mapping instead of the property. The relevant ones are probably this one in nav expansion, and this one in translation, if you want to just change them in this PR.

Note that for ranges specifically, I'm thinking that it may actually be possible (and useful) to introduce queryability for some of these: see npgsql/efcore.pg#2867.

@roji
Copy link
Member

roji commented Sep 11, 2023

(leaving it to @AndriySvyryd to review these metadata changes)

@roji
Copy link
Member

roji commented Sep 11, 2023

(ah, I see this is partially done in https://github.com/dotnet/efcore/pull/31680/files#diff-6c8d087179e9bd9496dd5122b69f7ccffa83e5b4b191862cecb14ca840ef69bcR2313 - consider also doing the nav expansion change there)


SetAnnotation(CoreAnnotationNames.ElementType, elementType);

IsPrimitiveCollection = ClrType.TryGetElementType(typeof(IEnumerable<>))?.IsAssignableFrom(clrType) == true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a parameter to avoid duplicate logic (it will require generating different code for compiled model though)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiled model isn't currently working for primitive collections anyway. I'm starting to look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do it in #31680

@ajcvickers ajcvickers merged commit 88edb55 into release/8.0 Sep 11, 2023
@ajcvickers ajcvickers deleted the 230910_ItsAHorse branch September 11, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants